855805 - Create the Bookmarks widget with subview

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

This should be something along the lines of the History widget seen here:

http://people.mozilla.com/~shorlander/panel-experiment-03/panel-experiment.html

Will probably need UX direction for the actual layout, however.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

During today's Australis meeting, we talked a bit about how the Bookmarks widget was supposed to work.

Tentative decisions:

a) The bookmarks widget, when in the panel, should open a subview
b) The subview should list X number of top-level bookmarks.

So, the questions are:

1) Which bookmarks? Most recent? Most used?
2) Are there other functions here? The ability to add a bookmark to the current page, like in the Bookmarks Alt menu?
3) How large is X?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bookmarks subview notes and ideas

These are some notes and sketches I made after looking at mak's patch in bug 748894 (which merges the Bookmarks button and the Star button).

I'm a little concerned that the subview has too little horizontal space, which will force it truncate long URLs and bookmark titles.

Anyhow, looking for feedback on this so we can solidify a direction to proceed with.

Comment 3

[Zhenshuo Fang (:fang) - Firefox UX Team]

(In reply to Mike Conley (:mconley) from comment #2)

I have some feedback and thoughts about this, might be a long reply, so tl;dr: I think the general direction looks good, the main concern is what to show in the subview & concerns on inconsistency of behavior when bookmarks button is in the toolbar.

1. About the fall back (click to open the library): I agree that this is acceptable, at least for v1. Then we can test and see how many users actually move the bookmarks button into menu panel and why, and then make adjustment. (I have some concern about losing the one-click bookmark ability, see next comment)

2. What to show in the subview: Can we show "Most visited" (and even another section of "recently bookmarked") instead of a list of bookmarks? The Pros: we don't have to deal with scrolling since we can determine to show X number of most visited bookmarks, and we don't have to show any folders. It would be weird to show folders but when user click on it we open the library. The cons: inconsistency of behavior when bookmarks button is in the toolbar, which we might have to just accept it since it's hard to avoid...

3. The limited horizontal space: I don't think we should remove the icons cause a long list of texts is just too hard to scan. Maybe we can just make the default width a little bit wider? or make the menu wider only when there's a subview? I don't think it'll make the menu much too uglier, unless there's other issues with the width.

Comment 4

[Zhenshuo Fang (:fang) - Firefox UX Team]

Attachment: One Click bookmark in Menu panel

Another concern with the fall back option (show library when bookmarks button is in menu panel) and the subview approach above, is that it loses the ability of one-click quick bookmarking.

One related question is do we show different style of the bookmarks button (grey vs. yellow/blue) based on whether the page is bookmarked when opening menu panel? it makes sense to show status of the page in the toolbar but menu is kind of browser-level... but I think we should show it anyway, which means when on an already bookmarked page, we should a yellow/blue star in stead of a grey one.

So if the user want to bookmark a page when the bookmarks button is in menu panel, assuming he doesn't use right click, he has to click menu->click "bookmark this page", and I assume we show the "bookmark edit panel" after that so the he has to click "done" or outside the panel, which is three clicks compare to when bookmarks button is in the toolbar. 

One crazy (or not?) idea is use the same behavior for the combined bookmarks button (see attachment) but with a different visual element. This is not ideal but just put it out there for discussion...

- when click the bookmarks button once, bookmark the page (show yellow star) and show tooltip "bookmark added" for 2 seconds.
- when click the yellow star again, show the "edit panel" in a hanger thingy under the bookmarks button.
- user access the submenu by clicking on the "arrow towards right"

pros: similar enough behavior with bookmarks button in toolbar, still has the quick bookmark ability
cons: visually unacceptable? too much engineering effort for the hanger thingy? unique behavior that no other menu item has.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

So fang and I talked about this in IRC, and here's where we're heading:

1) Bug 748894 (which mak hopes to land soon) makes the Bookmark button a split button, where the left-half of the button (the star) bookmarks the page or brings up the Edit Bookmark panel. The right-half of the button brings up the Bookmarks menu popup.

2) That button can be dragged into the panel. When it is in the panel, it does not appear as a split button. It appears as a normal panel button. The star icon still indicates whether or not the current page is bookmarked, but other than that, it does not behave the same as when it is in the nav-bar.

Clicking on that button will bring up the "bookmarks subview".

3) The bookmarks subview allows the user to bookmark the page (or edit the bookmark if one already exists). It also shows some number of most frequently visited bookmarks.

Comment 6

[u428464]

(In reply to Mike Conley (:mconley) from comment #5)
> So fang and I talked about this in IRC, and here's where we're heading:
> 
> 1) Bug 748894 (which mak hopes to land soon) makes the Bookmark button a
> split button, where the left-half of the button (the star) bookmarks the
> page or brings up the Edit Bookmark panel. The right-half of the button
> brings up the Bookmarks menu popup.
> 
> 2) That button can be dragged into the panel. When it is in the panel, it
> does not appear as a split button. It appears as a normal panel button. The
> star icon still indicates whether or not the current page is bookmarked, but
> other than that, it does not behave the same as when it is in the nav-bar.

So a new icon is needed here (maybe the classic one with the little star blue or yellow if bookmarked or not).

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 2

Some cleanups

Comment 9

[u428464]

Could clear feedback on attachement here too I think.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 3

De-bitrotted for latest jamun.

But this might benefit instead from the API that Blair described in bug 866245.

I'm going to see how far I get with that, and then come back to this.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 742543 [details] [diff] [review]
WIP Patch 3

Hey Blair,

I noticed that in bug 861702, the PanelUI-subView class starts to be added programmatically when building widgets with subviews.

That's OK, but it doesn't cover the case where a subview is supposed to be triggered by an old style XUL widget (which doesn't get created).

How do we reconcile this? Do we need a mechanism for old XUL widgets to indicate that they too have subviews, or is manually adding the PanelUI-subView class to the node acceptable?

-Mike

Comment 12

[Mike Conley (:mconley) (:⚙️)]

So a good chunk of this work is slightly moot. The UX team has proposed something new:

1) Back out the bookmark star change before it gets built into Aurora (bug 867343)
2) Replace the bookmark split menu button with something more akin to this:

http://shorlander.dropmark.com/118810

The good news is that I don't have to jump through so many hoops to keep the old split-button behaviour when the button is in a toolbar. I can create the new widget more or less from scratch, re-use the subview stuff I had here in this bug, and also maybe leverage the work jaws is doing in bug 861088.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

More notes:

The panel that the bookmark widget should open should display the toplevel bookmarks and bookmark folders.

When the widget is in the nav-bar, the user can go down a single folder level via a subview effect similar to the one experienced in the menu panel.

When the widget is in the menu panel, traveling down a single folder level is not allowed. We still show the folders, but clicking on them should open that folder in the Library instead.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Because it sounds like both the menu panel, and (at the very least) the bookmarks panel will be having the capability to show subviews a la this demo[1], I think I'm going to try to abstract out the subview stuff from PaneLUI.js into a re-usable XBL binding that extends panel.

[1]: http://people.mozilla.com/~shorlander/panel-experiment-03/panel-experiment.html

Comment 15

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) from comment #14)
> I think I'm going to try to abstract out the subview stuff from
> PaneLUI.js into a re-usable XBL binding that extends panel.
> 

Correction - extends arrowpanel.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike Conley (:mconley) from comment #12)
> http://shorlander.dropmark.com/118810

Sounds good - I like this much better.

(In reply to Mike Conley (:mconley) from comment #15)
> (In reply to Mike Conley (:mconley) from comment #14)
> > I think I'm going to try to abstract out the subview stuff from
> > PaneLUI.js into a re-usable XBL binding that extends panel.
> > 
> 
> Correction - extends arrowpanel.

Ditto.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

I think I'm going to break this up into a few bugs and patches, since I think getting all of the bookmark widget's functionality into a single patch is probably a review nightmare.

So here's what this patch does:

1) Adds a Bookmarks widget that when in the menu panel, opens a subview that shows "Bookmark this page", "Show Bookmark Toolbar", a list of at maximum 500 bookmarks and folders, and then "Show All Bookmarks".
2) "Bookmark this page" currently continues to anchor to the star in the URL bar. I'm not going to deal with the new functionality in this patch.
3) If the widget is in the menu, clicking on a folder currently brings up the Library showing All Bookmarks. This is not ideal - we'd like to be able to show the contents of a particular folder, however the Library doesn't really support this yet. I'll have to add that support in a separate bug.
4) If the widget is in a toolbar, clicking on it opens the subview in its own panel. Clicking on a folder in that panel opens *another* subview showing the contents of that folder.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1

Getting rid of some debugging cruft.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.2

More debugging cruft!

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 755585 [details] [diff] [review]
Patch v1.2

Hey Mano,

So this is the start of the new bookmarking widget that we're aiming to ship with Australis.

Just wanted to know if everything looks OK in browser-places.js and browserPlacesViews.js. Is what I'm doing making sense, or is there a better way of going about all of this?

-Mike

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Sadly, this is slipping to M7.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.3

Updated for latest trunk.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Attachment: Experiment patch

This patch shows me trying to delve into browser-places.js and pick it apart. It also removes the URL bar star.

I've only ever tested this on Ubuntu, so it'll likely not look quite right on Windows or OSX (although I can't say it looked all that great on Ubuntu either).

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Reassigning to myself, to work on the interim design I got by UX.

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: wip

This builds on top of the patch in bug 877748

Basically it implements the subview, when the button is in the appmenu panel. It's incomplete and likely has bugs, though if you want to put these 2 patches in the UR build I think it would do the job.

I will try to cleanup the thing in the next days, but the functionality is there.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Was the needinfo? on me for whether or not to use this for bug 880792? Or something else?

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yep, just for you to evaluate whether this is usable for bug 880792

Comment 29

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, I have most of the interaction working as expected in the various panels/toolbars and I'm working to "fake" the arrow panel aspect for the dropdown button. I cannot use a real arrow panel there cause it breaks most of the common interaction we have with menupopups, though I can  make a menupopup appear as an arrow panel, I have this partly done and I'm working on its regressions.

Now, I'd need the graphic for the dropdown part of the button, if possible.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block landing on m-c.

Comment 31

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: widget and menus v1

This seems to work in an acceptable way, apart the missing graphic that I replaced with existing menu graphic.
There is some small glitch here and there but nothing we can't handle at a later time.
I think it's ok to start reviewing it.

Note it still requires bug 877748 for proper functionality.

Comment 32

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: arrow bookmarks menu v1

and this applies on top of the previous patch, implementing the arrow panel look for the bookmarks menu.
ah, the missing graphic is in this part, not the previous one.

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 765671 [details] [diff] [review]
widget and menus v1

Review of attachment 765671 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly sane - thanks so much again for taking this on. :)

Just a few nits, and some general questions (some for UX too).

::: browser/base/content/browser-places.js
@@ +1013,4 @@
>    },
>  
>    get anchor() {
> +    //TODO (bug 873712): handle this properly once there's a better way to

Nit - space before TODO

@@ +1016,5 @@
> +    //TODO (bug 873712): handle this properly once there's a better way to
> +    // figure when the button is in the overflow panel.
> +    let button = this.button;
> +    if (button && button.classList.contains("overflowedItem")) {
> +      return  CustomizableUI.getWidget("bookmarks-menu-button")

Nit - only one space after return

@@ +1102,5 @@
>      this._updateToolbarStyle();
>    },
>  
>    _updateToolbarStyle: function BUI__updateToolbarStyle() {
> +    let button = this.btton;

Ruh roh - this.btton is undefined. Should be this.button.

@@ +1230,5 @@
> +    let widget = CustomizableUI.getWidget("bookmarks-menu-button")
> +                               .forWindow(window);
> +    if (widget.areaType == CustomizableUI.TYPE_MENU_PANEL) {
> +      let view = document.getElementById("PanelUI-bookmarks");
> +      view.addEventListener("ViewShowing", this.onAppMenuViewShowing);

Nomenclature nit - AppMenu has, historically, referred to the old-style Firefox button. We haven't been 100% perfect in removing references to it (I think some strings, for example, still have appMenu in their keys), but I'd like to avoid adding more.

So maybe PanelMenu is a better choice.

@@ +1245,5 @@
>    },
>  
> +  onAppMenuViewShowing: function BUI_onViewShowing(aEvent) {
> +    // Update checked status of the toolbar toggle.
> +    let viewToolbar = document.getElementById("appmenu_viewBookmarksToolbar");

Same with the IDs - maybe call it panel_viewBookmarksToolbar. Or panelMenu_viewBookmarksToolbar.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +44,5 @@
> +      <label value="&appMenuBookmarks.label;"/>
> +      <toolbarbutton id="appMenuBookmarkThisPage"
> +                     label="&bookmarkThisPageCmd.label;"
> +                     command="Browser:AddBookmarkAs"
> +                     onclick="PanelUI.hide();"/>

Not sure if I'm a huge fan of all of these onclick="PanelUI.hide();" inline event handlers. I guess listening to command events doesn't work though, since the events will get fired from the <command> element that the command attribute refers to. Argh.

We don't have a great solution for panel closing though, so I guess until we come up with something better, this'll have to do...

@@ +73,5 @@
> +                   onclick="if (event.button == 1) BookmarkingUI.onAppMenuViewCommand(event, this._placesView);"
> +                   oncommand="BookmarkingUI.onAppMenuViewCommand(event, this._placesView);"
> +                   flatList="true"
> +                   tooltip="bhTooltip">
> +        <arrowscrollbox id="appmenu_bookmarksMenuItems"

I think I'd like UX's blessing on this arrowscrollbox before r+'ing this. I'm *preeeeety* sure we'd prefer a standard vertical scrollbar instead, over the arrowscrollbox. But I'll double-check with shorlander.

::: browser/components/places/content/browserPlacesViews.js
@@ +1893,5 @@
> +    while (this._rootElt.hasChildNodes()) {
> +      this._rootElt.removeChild(this._rootElt.firstChild);
> +    }
> +
> +    for (let i = 0; i < this._resultNode.childCount; ++i) {

Should we cap this at some point? Is it really useful to have hundreds and hundreds of items in this list?

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 765672 [details] [diff] [review]
arrow bookmarks menu v1

Review of attachment 765672 [details] [diff] [review]:
-----------------------------------------------------------------

I'm happy with this with the debugging reportError gone and my other nit fixed.

::: browser/base/content/browser.css
@@ +698,5 @@
>  }
> +
> +/* Give this menupopup an arrow panel styling */
> +#BMB_bookmarksPopup {
> +  -moz-binding: url("chrome://browser/content/places/menu.xml#places-popup-arrow");

Nit - -moz-appearance before -moz-binding

::: browser/components/places/content/menu.xml
@@ +585,5 @@
> +        if (anchorClass && this.anchorNode) {
> +          let anchor =
> +            document.getAnonymousElementByAttribute(this.anchorNode, "class",
> +                                                    anchorClass);
> +          Components.utils.reportError(anchor);

I'm guessing this is a debugging leftover? We should remove it.

::: browser/themes/linux/browser.css
@@ +1383,5 @@
>    opacity: .4;
>  }
>  
> +#bookmarks-menu-button.toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon {
> +  /* FIXME FIXME FIXME: this is a placeholder */

So I guess we need an icon here... I'll needinfo? shorlander.

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Hey Stephen,

1) Do you have an icon we can use for the dropmarker for the bookmarks menu button when it's in a toolbar? Or should we file a separate bug for that asset?
2) Am I correct in remembering that arrowscrollbox is a thing-to-avoid and that we should just use the good ol' vertical scrollbar for long lists?

-Mike

Comment 37

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mike Conley (:mconley) from comment #34)
> I think I'd like UX's blessing on this arrowscrollbox before r+'ing this.
> I'm *preeeeety* sure we'd prefer a standard vertical scrollbar instead, over
> the arrowscrollbox. But I'll double-check with shorlander.

ok, I didn't try the scrollbar, I thought we wanted to keep it coherent with the menu that uses the arrowscrollbox.  The scrollbar should be feasible I guess. Regardless, that is something that could be changed later too, in a polish phase.

> ::: browser/components/places/content/browserPlacesViews.js
> @@ +1893,5 @@
> > +    while (this._rootElt.hasChildNodes()) {
> > +      this._rootElt.removeChild(this._rootElt.firstChild);
> > +    }
> > +
> > +    for (let i = 0; i < this._resultNode.childCount; ++i) {
> 
> Should we cap this at some point? Is it really useful to have hundreds and
> hundreds of items in this list?

no idea, if this is intended as a replacement to the menu, I guess we should have everything. UX may have a different opinion, I don't have strong feelings.

Comment 38

[Stephen Horlander [:shorlander]]

(In reply to Mike Conley (:mconley) - mostly gone until July 8 from comment #36)
> Hey Stephen,
> 
> 1) Do you have an icon we can use for the dropmarker for the bookmarks menu
> button when it's in a toolbar? Or should we file a separate bug for that
> asset?

Should I add this to the toolbar sprite?

> 2) Am I correct in remembering that arrowscrollbox is a thing-to-avoid and
> that we should just use the good ol' vertical scrollbar for long lists?

Correct.

Comment 39

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Stephen Horlander [:shorlander] from comment #38)
> (In reply to Mike Conley (:mconley) - mostly gone until July 8 from comment
> #36)
> > Hey Stephen,
> > 
> > 1) Do you have an icon we can use for the dropmarker for the bookmarks menu
> > button when it's in a toolbar? Or should we file a separate bug for that
> > asset?
> 
> Should I add this to the toolbar sprite?

Imho would be easier, for inverted colors support.

Comment 40

[Dão Gottwald [:dao]]

Do you mean something different than toolbarbutton-dropdown-arrow.png / toolbarbutton-dropdown-arrow-inverted.png?

Comment 41

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

it won't be a dropdown arrow, but an actual button icon, I thought having it in the palette was preferred to avoid special rules to replace the icons, according to comments I saw in the past in other bugs. We will need a small version for Linux a Lion version for Mac, anything that a button requires. Would you prefer a separate graphic file?

Comment 42

[Dão Gottwald [:dao]]

No, I just didn't understand what you meant when you wrote "an icon we can use for the dropmarker."

Comment 43

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

technically it's still a menu-button, but the dropmarker button will be handled as a common button, so it will be as large as the main button and will have an icon instead of the dropmarker glyph.

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Hey Marco, any updates on this?

Comment 46

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I still lack the graphics, the css can't be completed without the updated Toolbar.png and friends.
But I can improve the code by fixing your comments, will do that.

Comment 47

[Mike Conley (:mconley) (:⚙️)]

(In reply to Marco Bonardo [:mak] from comment #46)
> I still lack the graphics, the css can't be completed without the updated
> Toolbar.png and friends.
> But I can improve the code by fixing your comments, will do that.

For Australis, we've been OK landing changes with temporary / placeholder graphic assets until the real ones can land. So waiting on graphics should not block this.

Comment 48

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: widget and functionality

This has all of the functionality, while the double-button and arrow-panel styling is left for the other part.
I think I fixed all of the comments, included the change from arrowscrollbox to common scrollbar, and also used the recent improvements to the australis code.
If you're satisfied with the functionality part, we may land it and leave-open while I work on the styling part, so this can be tested earlier and won't bitrot more.

Comment 49

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 778840 [details] [diff] [review]
widget and functionality

Review of attachment 778840 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Marco,

This looks great, and thanks for all of your hard work. :) I think this is landable, and issues that are found with it can be addressed in follow-ups.

-Mike

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +60,5 @@
> +                     type="checkbox"
> +                     toolbarId="PersonalToolbar"
> +                     oncommand="onViewToolbarCommand(event); PanelUI.hide();"/>
> +      <toolbarseparator/>
> +      <toolbarbutton id="panelMenu_bookmarksToolbar"

These two toolbar buttons need to close the panel as well.

@@ +67,5 @@
> +      <toolbarbutton id="panelMenu_unsortedBookmarks"
> +                     label="&unsortedBookmarksCmd.label;"
> +                     oncommand="PlacesCommandHook.showPlacesOrganizer('UnfiledBookmarks');"/>
> +      <toolbarseparator/>
> +      <toolbaritem id="panelMenu_bookmarksMenu"

This item should probably have a min-height, otherwise it can get too small for the scrolling to be much use at all. Just choose some reasonable em, I suppose.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ -178,5 @@
>  
>  <!ENTITY bookmarksMenuButton.label          "Bookmarks">
>  <!ENTITY bookmarksMenuButton.tooltip        "Display your bookmarks">
>  <!ENTITY bookmarksMenuButton.unsorted.label "Unsorted Bookmarks">
> -<!ENTITY viewBookmarksSidebar.label         "Show in Sidebar">

We've been asked not to remove strings, to reduce pain if/when we do a back-out. Instead, strings to be removed should go at the bottom in a "to be deleted" block (See bug 889500, bug 896161)

@@ -324,5 @@
>  <!ENTITY showAllHistoryCmd2.label "Show All History">
>  <!ENTITY showAllHistoryCmd.commandkey "H">
>  
>  <!ENTITY appMenuCustomize.label "Customize">
> -<!ENTITY appMenuBookmarks.label "Bookmarks">

Same as above.

Comment 50

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Styling

This is the styling part.
There are issues clearly:
- the graphics are "homemade", I didn't spend time optimizing Toolbar.png files since shorlander will have to replace them regardless
- on osx the "open" state is missing, I was not brave enough to also try designing those. Probably there's also some polishing to do, the star turns blue when the popup opens, that's quite strange but was there already.

I think this is a good starting point, polishing can be done easily on top of this.

Comment 51

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mike Conley (:mconley) from comment #49)
> ::: browser/components/customizableui/content/panelUI.inc.xul
> @@ +60,5 @@
> > +                     type="checkbox"
> > +                     toolbarId="PersonalToolbar"
> > +                     oncommand="onViewToolbarCommand(event); PanelUI.hide();"/>
> > +      <toolbarseparator/>
> > +      <toolbarbutton id="panelMenu_bookmarksToolbar"
> 
> These two toolbar buttons need to close the panel as well.

Probably yes, I was unsure since they are toggle buttons, but probably makes sense.

> @@ +67,5 @@
> > +      <toolbarbutton id="panelMenu_unsortedBookmarks"
> > +                     label="&unsortedBookmarksCmd.label;"
> > +                     oncommand="PlacesCommandHook.showPlacesOrganizer('UnfiledBookmarks');"/>
> > +      <toolbarseparator/>
> > +      <toolbaritem id="panelMenu_bookmarksMenu"
> 
> This item should probably have a min-height, otherwise it can get too small
> for the scrolling to be much use at all. Just choose some reasonable em, I
> suppose.

I'm not sure I understand this request, the scrollbar should exist only when needed.

> We've been asked not to remove strings, to reduce pain if/when we do a
> back-out.

ok!

Comment 52

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

filed bug 897268 to replace my crappy images with proper icons.

Comment 53

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Marco Bonardo [:mak] from comment #51)
> (In reply to Mike Conley (:mconley) from comment #49)
> > These two toolbar buttons need to close the panel as well.
> 
> Probably yes, I was unsure since they are toggle buttons, but probably makes
> sense.

nevermind, I read the comment wrongly, those should indeed close the panel.

Comment 54

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: widget and functionality

Addressed comments

Comment 55

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 780069 [details] [diff] [review]
widget and functionality

Landed the first part
https://hg.mozilla.org/projects/ux/rev/e6cceb960b18

Comment 56

[Tim Nguyen :ntim]

Great work !

Problems remaining :
- If the page is starred and the toolbar button is located in the menu, then the panel menu icon would still stay the same (it should changed to starred state)
- Visual styling for panel subview (currently looks horrible)
- The font size is too big when the toolbar button is located in the menu (only tested on Windows)
- When clicking bookmark this page in the subview, the "page bookmarked" panel should show near the subview, or even integrated to the subview.
- Animation is too fast (at least different for me) when opening the subview, you should see the difference between opening the history subview and the bookmarks subview

Comment 57

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 780059 [details] [diff] [review]
Styling

Review of attachment 780059 [details] [diff] [review]:
-----------------------------------------------------------------

I've noticed that when dragging an item to the button when it's in the toolbar, the panel will open, but I only have a split second to move my cursor down to the panel or else it closes again. I don't think that should block this work from landing, but could you please file a follow-up for it?

Anyhow, I'm happy with this. Thanks Marco!

::: browser/base/content/browser.xul
@@ +681,5 @@
>            <menupopup id="BMB_bookmarksPopup"
>                       placespopup="true"
>                       context="placesContext"
>                       openInTabs="children"
> +                     anonymousAnchorClass="toolbarbutton-menubutton-dropmarker"

I think there's a convention that element attributes should always be all lower-case.

However, seeing as how we're already violating this with openInTabs, and restricting this change to BMB_bookmarksPopup, I guess this isn't so bad...

::: browser/themes/osx/browser.css
@@ +1849,5 @@
> +#BMB_bookmarksPopup > menuitem,
> +#BMB_bookmarksPopup > menu,
> +#BMB_bookmarksPopup > menuseparator {
> +  -moz-appearance: none;
> +}

Nit - newline after this closing brace.

Comment 58

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mike Conley (:mconley) from comment #57)
> I've noticed that when dragging an item to the button when it's in the
> toolbar, the panel will open, but I only have a split second to move my
> cursor down to the panel or else it closes again. I don't think that should
> block this work from landing, but could you please file a follow-up for it?

yes that's why I increased the closing timer, but could be not enough yet. I will file the bug, we may have to completely change that code.
I will address your comments and land later today.

Comment 59

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Styling

Comment 60

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/projects/ux/rev/81ce85168be1

Comment 61

[Sean Smith]

Trying to replicate the issue of the menu closing too fast, I gave up trying to make the bookmark.  I then noticed that the marker that indicates where the bookmark would be created when dropped stayed in the menu during subsequent opening and closing.  It disappears after a restart.

And, for some reason, I can only create bookmarks using drag and drop from this site, but no others that I have tried.  The cursor becomes the "action unavailable" cursor.

I can file new bugs for you if you want.

Comment 62

[Sean Smith]

(In reply to Sean Smith from comment #61)
>  
> And, for some reason, I can only create bookmarks using drag and drop from
> this site, but no others that I have tried.  The cursor becomes the "action
> unavailable" cursor.

I've discovered that I can only create bookmarks using drag and drop on the widget menu with https sites, but not http sites.

Comment 63

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

what are you dragging, a link, the whole url from the locationbar?

Comment 64

[Sean Smith]

(In reply to Marco Bonardo [:mak] from comment #63)
> what are you dragging, a link, the whole url from the locationbar?

I click in the URL bar to highlight the entire address/URL, then drag that to the menu side of the widget to get the menu to open.  If it is an https site, the URL can be dropped in the menu to create a bookmark; if it is an http site, the cursor becomes the non-available cursor style when I reach the widget.  The widget will still open, but I cannot drop the URL on the menu to create the bookmark.

Dragging a link from the page works as expected, I can create the bookmark, just not if I drag the URL from the address bar.  If I grab the site identity icon, then it works.

Comment 65

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

that's not a regression, the old menu works the same.
The problem is that trimURL removes http:// making what you are dragging an invalid url... Try dragging the identity icon instead (the icon on the left of the url)

Surely that's something we may try to fix, you may file a bug about it, still not a regression.

Comment 66

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/e6cceb960b18
https://hg.mozilla.org/mozilla-central/rev/81ce85168be1

Comment 67

[Jim Michaels]

I find that star button is not working for me as a replacement for the bookmarks button, I make regular use of the bookmarks button for productivity. other people may use the star thing, but I for speed just use ctrl-d, although I would use both probably if the buttons were there, but I personally am fast with the keyboard, I just have not memorized all the kb hotkeys. ctrl-b is hard to reach and thus error-prone, but it is mnemonic. so I tend to use a button for that to eliminate mistakes and hand-stretching.

Comment 68

[Jim Michaels]

actually, I dropped IE and chrome because of the minimalistic approach to UI design. it is historically why I have liked ff.

Comment 69

[Jim Michaels]

if you must stay only with the star thing, at least add to it a dropdown that does the ctrl-b thing. it's clunky at best, but at least users can get to the bookmarks pane and are not cut off from managing their bookmarks.

Comment 70

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the first entry of the bookmarks panel is opening the sidebar (what ctrl+b does)

Comment 71

[Jim Michaels]

there are no entries. only ctrl-b opens the sidebar.
there is no dropdown in ff29. star button adds to bookmarks only. button that looks like bookmarks list to right of star (what the star "adds to") does nothing when clicked.

Comment 72

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Jim Michaels from comment #71)
> there are no entries. only ctrl-b opens the sidebar.
> there is no dropdown in ff29. star button adds to bookmarks only. button
> that looks like bookmarks list to right of star (what the star "adds to")
> does nothing when clicked.

so, looks like you have some serious issue with your profile. Please try Safe mode (http://support.mozilla.org/kb/Safe+Mode), I guess some of your add-ons is creating you more issues than it's solving.