Closed Bug 855805 Opened 7 years ago Closed 6 years ago

Create the Bookmarks widget with subview

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: mak)

References

(Blocks 1 open bug, )

Details

(Keywords: icon, Whiteboard: [Australis:M8][Australis:P1])

Attachments

(2 files, 16 obsolete files)

31.26 KB, patch
mak
: checkin+
Details | Diff | Splinter Review
375.03 KB, patch
Details | Diff | Splinter Review
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.
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?
Flags: needinfo?(zfang)
Flags: needinfo?(shorlander)
Attached image Bookmarks subview notes and ideas (obsolete) —
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.
Attachment #731888 - Flags: feedback?(zfang)
Attachment #731888 - Flags: feedback?(shorlander)
(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.
Flags: needinfo?(zfang)
Attached image One Click bookmark in Menu panel (obsolete) —
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.
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.
(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).
Flags: needinfo?(shorlander)
Status: NEW → ASSIGNED
Attached patch WIP Patch 2 (obsolete) — Splinter Review
Some cleanups
Attachment #734823 - Attachment is obsolete: true
Could clear feedback on attachement here too I think.
Attached patch WIP Patch 3 (obsolete) — Splinter Review
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.
Attachment #734830 - Attachment is obsolete: true
Attachment #731888 - Flags: feedback?(zfang)
Attachment #731888 - Flags: feedback?(shorlander)
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
Attachment #742543 - Flags: feedback?(bmcbride)
Attachment #742543 - Attachment is obsolete: true
Attachment #742543 - Flags: feedback?(bmcbride)
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.
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.
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
(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.
(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.
Depends on: 867224
No longer blocks: 867585
Depends on: 867585
Attachment #731888 - Attachment is obsolete: true
No longer blocks: 855287
Whiteboard: [Australis:M6]
Attachment #731973 - Attachment is obsolete: true
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #755420 - Attachment is obsolete: true
Attached patch Patch v1.1 (obsolete) — Splinter Review
Getting rid of some debugging cruft.
Attachment #755580 - Attachment is obsolete: true
Attached patch Patch v1.2 (obsolete) — Splinter Review
More debugging cruft!
Attachment #755583 - Attachment is obsolete: true
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
Attachment #755585 - Flags: feedback?(mano)
Sadly, this is slipping to M7.
Whiteboard: [Australis:M6] → [Australis:M7]
Attached patch Patch v1.3 (obsolete) — Splinter Review
Updated for latest trunk.
Attachment #755585 - Attachment is obsolete: true
Attachment #755585 - Flags: feedback?(mano)
Attached patch Experiment patch (obsolete) — Splinter Review
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).
Reassigning to myself, to work on the interim design I got by UX.
Assignee: mconley → mak77
Attached patch wip (obsolete) — Splinter Review
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.
Attachment #758346 - Attachment is obsolete: true
Attachment #758347 - Attachment is obsolete: true
Flags: needinfo?(mconley)
Was the needinfo? on me for whether or not to use this for bug 880792? Or something else?
Flags: needinfo?(mconley)
yep, just for you to evaluate whether this is usable for bug 880792
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.
Flags: needinfo?(madhava)
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
Attached patch widget and menus v1 (obsolete) — Splinter Review
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.
Attachment #762393 - Attachment is obsolete: true
Attachment #765671 - Flags: review?(mconley)
Attached patch arrow bookmarks menu v1 (obsolete) — Splinter Review
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.
Attachment #765672 - Flags: review?(mconley)
Keywords: icon
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P1]
Blocks: 883141
Duplicate of this bug: 885080
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?
Attachment #765671 - Flags: review?(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.
Attachment #765672 - Flags: review?(mconley) → review+
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
Flags: needinfo?(shorlander)
(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.
(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.
Flags: needinfo?(shorlander)
(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.
Flags: needinfo?(madhava)
Do you mean something different than toolbarbutton-dropdown-arrow.png / toolbarbutton-dropdown-arrow-inverted.png?
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?
No, I just didn't understand what you meant when you wrote "an icon we can use for the dropmarker."
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.
Duplicate of this bug: 865483
Hey Marco, any updates on this?
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
(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.
Attached patch widget and functionality (obsolete) — Splinter Review
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.
Attachment #765671 - Attachment is obsolete: true
Attachment #778840 - Flags: review?(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.
Attachment #778840 - Flags: review?(mconley) → review+
Attached patch Styling (obsolete) — Splinter Review
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.
Attachment #765672 - Attachment is obsolete: true
Attachment #780059 - Flags: review?(mconley)
(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!
Blocks: 897268
filed bug 897268 to replace my crappy images with proper icons.
(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.
Addressed comments
Attachment #778840 - Attachment is obsolete: true
Comment on attachment 780069 [details] [diff] [review]
widget and functionality

Landed the first part
https://hg.mozilla.org/projects/ux/rev/e6cceb960b18
Attachment #780069 - Flags: checkin+
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 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.
Attachment #780059 - Flags: review?(mconley) → review+
Depends on: 898838
(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.
Attached patch StylingSplinter Review
Attachment #780059 - Attachment is obsolete: true
Depends on: 899530
https://hg.mozilla.org/projects/ux/rev/81ce85168be1
Whiteboard: [Australis:M?][Australis:P1] → [Australis:M?][Australis:P1][fixed-in-ux]
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.
(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.
what are you dragging, a link, the whole url from the locationbar?
(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.
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.
Whiteboard: [Australis:M?][Australis:P1][fixed-in-ux] → [Australis:M8][Australis:P1][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/e6cceb960b18
https://hg.mozilla.org/mozilla-central/rev/81ce85168be1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][Australis:P1][fixed-in-ux] → [Australis:M8][Australis:P1]
Target Milestone: --- → Firefox 28
No longer depends on: 992684
Depends on: 992684
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.
actually, I dropped IE and chrome because of the minimalistic approach to UI design. it is historically why I have liked ff.
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.
the first entry of the bookmarks panel is opening the sidebar (what ctrl+b does)
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.
(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.
You need to log in before you can comment on or make changes to this bug.