777680 - "Bookmark This Page" in context menu should be highlighted

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[fx4waldi]

Attachment: Mockup

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20120724040203

Steps to reproduce:

In the context menu item "Bookmark This Page" should be highlighted when the page is added to your bookmarks. Currently, this element are the same. I created a simple mockup.

Comment 1

[fx4waldi]

Attachment: Mockup 2

the same is when you click Firefox Button > Bookmarks > Bookmark This Page

Comment 2

[fx4waldi]

Attachment: Patch 0.1

I did the patch, I tested the Windows 7 and it works. In Mac OS and Linux are not working icons.
This is a patch for the context menu.

Comment 3

[fx4waldi]

Attachment: Patch 0.2

patch to the context menu, bookmarkmenu and appmenu

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 647501 [details] [diff] [review]
Patch 0.2

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -3733,6 +3733,10 @@ var XULBrowserWindow = {
>     delete this.reloadCommand;
>     return this.reloadCommand = document.getElementById("Browser:Reload");
>   },
>+  get starButton () {

This seems to be unrelated to XULBrowserWindow.

>--- a/browser/base/content/nsContextMenu.js
>+++ b/browser/base/content/nsContextMenu.js
>@@ -257,9 +257,13 @@ nsContextMenu.prototype = {
>     var isTextSelected = this.isTextSelected;
> 
>     // Use "Bookmark This Link" if on a link.
>-    this.showItem("context-bookmarkpage",
>+	var starred = XULBrowserWindow.starButton.getAttribute("starred") == "true";

This code also runs in side bars (web-panels.xul), where no star button exists.

>--- a/browser/locales/en-US/chrome/browser/browser.dtd
>+++ b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!ENTITY bookmarkThisPageCmd2.label "Edit Bookmark This Page"> 

This doesn't sound like correct English.

Last but not least, context menus usually don't have icons, which I think invalidates most of this bug.

Comment 5

[u428464]

(In reply to Dão Gottwald [:dao] from comment #4)
> Last but not least, context menus usually don't have icons, which I think
> invalidates most of this bug.
But this bug is still interesting for the star in the bookmark menu. The name of the contextual action could also be changed.

Comment 6

[u428464]

Edit bookmark this page doesn't sound right in English. We should go for something simple like "Edit this bookmark" or "Edit bookmarked page".

Comment 7

[fx4waldi]

Attachment: Patch 0.3

no icon in the context menu
better sound
works in web-panels

Comment 8

[fx4waldi]

Attachment: Patch 0.3.1

no icon in the context menu
better sound
works in web-panels

Comment 9

[fx4waldi]

Attachment: Patch 0.4

no icon in the context menu (class="menuitem-iconic")
support for menubar

Comment 10

[fx4waldi]

Do you want the icons in Mac OS X and Linux. Where icons should be and where not? I do not have these systems and I do not know exactly how it looks on them.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

fx4waldi, would you like to pick this back up?

Comment 12

[fx4waldi]

Attachment: bug 777680.diff

I think I can try.
I have prepared a patch.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8433459 [details] [diff] [review]
bug 777680.diff

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

::: browser/base/content/nsContextMenu.js
@@ +269,5 @@
>      // Use "Bookmark This Link" if on a link.
> +    var shouldShow = !(this.isContentSelected || this.onTextInput || this.onLink ||
> +                       this.onImage || this.onVideo || this.onAudio || this.onSocial ||
> +                       this.onCanvas);
> +    var starred = PlacesUtils.getBookmarksForURI(this.browser.currentURI).length;	

I think we should try to use the BookmarkingUI object if possible. Gijs, what do you think? (asking because you worked on bug 964887).

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +438,5 @@
>  <!ENTITY viewBGImageCmd.accesskey     "w">
>  <!ENTITY setDesktopBackgroundCmd.label      "Set As Desktop Background…">
>  <!ENTITY setDesktopBackgroundCmd.accesskey  "S">
>  <!ENTITY bookmarkPageCmd2.label       "Bookmark This Page">
> +<!ENTITY bookmarkEditCmd.label        "Edit This Bookmark">

We don't need to introduce a new string here, we can reuse &editThisBookmarkCmd.label;

Comment 14

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> Comment on attachment 8433459 [details] [diff] [review]
> bug 777680.diff
> 
> Review of attachment 8433459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/nsContextMenu.js
> @@ +269,5 @@
> >      // Use "Bookmark This Link" if on a link.
> > +    var shouldShow = !(this.isContentSelected || this.onTextInput || this.onLink ||
> > +                       this.onImage || this.onVideo || this.onAudio || this.onSocial ||
> > +                       this.onCanvas);
> > +    var starred = PlacesUtils.getBookmarksForURI(this.browser.currentURI).length;	
> 
> I think we should try to use the BookmarkingUI object if possible. Gijs,
> what do you think? (asking because you worked on bug 964887).

Hmm. That's specifically meant to provide the UI just for the star button. It doesn't currently expose this API correctly, and even if we expose an API for the private _items collection, that won't be updated unless the star button is on a toolbar and visible.

However, we shouldn't use getBookmarksForURI because it does a sync places query (see bug 964887 comment #5 ). 

For now, the easy way to fix this is to make the context menu item observe the same broadcaster the star button is using. That will be strictly better than the current state, and mostly right, I think? Not 100% sure, but definitely no worse than the current state.


As for perfect solutions, we should check with Marco. IMO it would make sense to create an object in browser-places.js (let's say "CurrentPageBookmarkTracker", but bikeshed away) that keeps track of this state, which means we can get it synchronously and cheaply if possible, and synchronously and expensively if it isn't (ie the async queries haven't yet caught up with the actual gBrowser.currentURI).

We can file a separate bug for that refactoring, however. Marco, does the above idea sound right?

Comment 15

[:Gijs (he/him)]

Comment on attachment 8433459 [details] [diff] [review]
bug 777680.diff

Hrm, although I suppose because it's a label vs. aria-label... that might complicate things? Not sure. :-)

Comment 16

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #14)
> As for perfect solutions, we should check with Marco. IMO it would make
> sense to create an object in browser-places.js (let's say
> "CurrentPageBookmarkTracker", but bikeshed away) that keeps track of this
> state, which means we can get it synchronously and cheaply if possible, and
> synchronously and expensively if it isn't (ie the async queries haven't yet
> caught up with the actual gBrowser.currentURI).

I don't think there should be a way to get that synchronously and expensively, we will remove all ways to do that and burn with fire any code trying to do that :) I hope you can live with a cached-or-async solution.

I think it might make sense to share some object that tracks bookmarking status of the current page and re-use that object both here and in BookmarkingUI. Basically split BookmarkingUI into a functional part and a UI part. That should also make that object saner.

Btw, as you said the broadcaster should cover us here, notice that it's updated only when _updateBookmarkPageMenuItem is invoked, you likely need to add a currentPageContextPopupShowing method to invoke that (see onMainMenuPopupShowing or onPanelMenuViewShowing) otherwise you might get an outdated value. We basically update the broadcaster on request today.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

fx4waldi, is this enough information for you to take and push forward with the patch?

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch

Hey Gijs, this patch should match pretty close to what you described in comment #14 and then was later expanded on by Marco in comment #16.

One gripe about this patch: The context menu UI is updated via updateEditUIVisibility which isn't semantically correct. I'm using it now because it is called in the onpopupshowing of the contentAreaContextMenu. What do you think we should do here? We could add a call to a separate function in the onpopupshowing/onpopuphiding attribute but that seems like a slippery slope to me.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

The above patch also fixes bug 1061949.

Comment 22

[:Gijs (he/him)]

Comment on attachment 8616541 [details] [diff] [review]
WIP Patch

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

::: browser/base/content/browser-places.js
@@ +1350,4 @@
>      }
>      else {
>        this.star.removeAttribute("disabled");
> +      this.broadcaster.removeAttribute("stardisabled");

Maybe this is a dumb question, but why can't this set the disabled attribute on the broadcaster?

::: browser/base/content/browser.js
@@ +3927,5 @@
>      goSetCommandEnabled("cmd_switchTextDirection", true);
>    }
>  #endif
> +
> +  BookmarkingUI.onCurrentPageContextPopupShowing(); // XXXjaws this should be moved out of updateEditUIVisibility().

Why not call this from within the nsContextMenu constructor, if and only if shouldDisplay is true and the star is shown?

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 8616541 [details] [diff] [review]
WIP Patch

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

::: browser/base/content/nsContextMenu.js
@@ +10,5 @@
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
>    "resource:///modules/CustomizableUI.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +  "resource://gre/modules/PlacesUtils.jsm");

what are you using this for? leftover?

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #22)
> Comment on attachment 8616541 [details] [diff] [review]
> WIP Patch
> 
> Review of attachment 8616541 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-places.js
> @@ +1350,4 @@
> >      }
> >      else {
> >        this.star.removeAttribute("disabled");
> > +      this.broadcaster.removeAttribute("stardisabled");
> 
> Maybe this is a dumb question, but why can't this set the disabled attribute
> on the broadcaster?

The toolbarbutton is observing this broadcaster. If we set the "disabled" attribute on the broadcaster, then the button to open the bookmarks-menu will be disabled when we only want the star disabled (it's also why the previous code was only setting "disabled" on the star whereas the other attributes were set on the button).

(In reply to Marco Bonardo [::mak] from comment #23)
> what are you using this for? leftover?

Yes, leftover. Thanks for catching that.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Thanks for the quick feedback Gijs and Marco. This patch should be ready for review now.

I'm no longer setting the overflow label on the broadcaster because the label was specific to the overflowed state of the button, which shouldn't get propagated to the context menu.

Also, the toolbarbutton is only observing the "starred" and "buttontooltiptext" attributes because the observer also has a "label" attribute that we don't want to get propagated to the button if it is placed in the panel. The panel should always read "Bookmarks" regardless of the starred state, since it opens the subview.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8616765 [details] [diff] [review]
Patch

I think Marco would make a better reviewer. :-)

Comment 27

[Marco Bonardo [:mak]]

Comment on attachment 8616765 [details] [diff] [review]
Patch

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

::: browser/base/content/browser-context.inc
@@ +32,5 @@
>                    command="Browser:Stop"/>
>          <menuitem id="context-bookmarkpage"
>                    class="menuitem-iconic"
> +                  observes="bookmarkThisPageBroadcaster"
> +                  defaulttooltiptext="&bookmarkPageCmd2.label;"

what's this defaulttooltiptext and what's its scope?

::: browser/base/content/browser-places.js
@@ +1691,5 @@
>        PlacesCommandHook.bookmarkCurrentPage(isBookmarked);
>      }
>    },
>  
> +  onCurrentPageContextPopupShowing: function BUI_onCurrentPageContextPopupShowing() {

nit: just
onCurrentPageContextPopupShowing() {

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/538f0581a56b

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3a69292cf6b4

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/538f0581a56b
https://hg.mozilla.org/mozilla-central/rev/3a69292cf6b4

Comment 31

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on Windows 7 32-bit, Windows 10 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 64-bit using Firefox 40.0b1, build ID: 20150811185633.

Comment 32

[[:Towkir] Ahmed]

Won't it be implemented into current release (v40) ??

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to [:Towkir] Ahmed from comment #32)
> Won't it be implemented into current release (v40) ??

It wasn't fixed in time for v40 and it wasn't high enough priority + severity for it to get expedited to v40. It will be released with v41.