Closed Bug 777680 Opened 7 years ago Closed 4 years ago

"Bookmark This Page" in context menu should be highlighted

Categories

(Firefox :: Menus, defect)

17 Branch
defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 41
Iteration:
41.3 - Jun 29
Tracking Status
firefox32 --- affected
firefox36 --- affected
firefox41 --- verified

People

(Reporter: fx4waldi, Assigned: jaws)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 9 obsolete files)

Attached image Mockup (obsolete) —
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.
Component: Untriaged → Menus
OS: Windows 7 → All
Hardware: x86 → All
Attached image Mockup 2 (obsolete) —
the same is when you click Firefox Button > Bookmarks > Bookmark This Page
Attached patch Patch 0.1 (obsolete) — Splinter Review
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.
Attachment #647185 - Flags: review?(dao)
Attachment #647185 - Attachment is patch: true
Attached patch Patch 0.2 (obsolete) — Splinter Review
patch to the context menu, bookmarkmenu and appmenu
Attachment #647185 - Attachment is obsolete: true
Attachment #647185 - Flags: review?(dao)
Attachment #647501 - Flags: review?(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.
Attachment #647501 - Flags: review?(dao) → review-
(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.
Edit bookmark this page doesn't sound right in English. We should go for something simple like "Edit this bookmark" or "Edit bookmarked page".
Attached patch Patch 0.3 (obsolete) — Splinter Review
no icon in the context menu
better sound
works in web-panels
Attachment #647501 - Attachment is obsolete: true
Attachment #654183 - Flags: review?(dao)
Attached patch Patch 0.3.1 (obsolete) — Splinter Review
no icon in the context menu
better sound
works in web-panels
Attachment #654183 - Attachment is obsolete: true
Attachment #654183 - Flags: review?(dao)
Attachment #654184 - Flags: review?(dao)
Attached patch Patch 0.4 (obsolete) — Splinter Review
no icon in the context menu (class="menuitem-iconic")
support for menubar
Attachment #654184 - Attachment is obsolete: true
Attachment #654184 - Flags: review?(dao)
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.
Blocks: 1000513
Status: UNCONFIRMED → NEW
Ever confirmed: true
fx4waldi, would you like to pick this back up?
Flags: needinfo?(fx4waldi)
Attached patch bug 777680.diff (obsolete) — Splinter Review
I think I can try.
I have prepared a patch.
Attachment #646092 - Attachment is obsolete: true
Attachment #647084 - Attachment is obsolete: true
Attachment #657697 - Attachment is obsolete: true
Attachment #8433459 - Flags: review?(jaws)
Flags: needinfo?(fx4waldi)
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;
Attachment #8433459 - Flags: review?(jaws) → feedback?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(mak77)
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. :-)
Attachment #8433459 - Flags: feedback?(gijskruitbosch+bugs) → feedback-
(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.
Flags: needinfo?(mak77)
fx4waldi, is this enough information for you to take and push forward with the patch?
Assignee: nobody → fx4waldi
Status: NEW → ASSIGNED
Flags: needinfo?(fx4waldi)
Assignee: fx4waldi → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(fx4waldi)
Duplicate of this bug: 1061949
Duplicate of this bug: 1128160
Depends on: 1061949
Attached patch WIP Patch (obsolete) — Splinter Review
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.
Assignee: nobody → jaws
Attachment #8433459 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8616541 - Flags: feedback?(gijskruitbosch+bugs)
The above patch also fixes bug 1061949.
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?
Attachment #8616541 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
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?
(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.
Attached patch PatchSplinter Review
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.
Attachment #8616541 - Attachment is obsolete: true
Attachment #8616765 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8616765 [details] [diff] [review]
Patch

I think Marco would make a better reviewer. :-)
Attachment #8616765 - Flags: review?(gijskruitbosch+bugs) → review?(mak77)
Iteration: --- → 41.3 - Jun 29
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
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() {
Attachment #8616765 - Flags: review?(mak77) → review+
Depends on: 1174434
https://hg.mozilla.org/mozilla-central/rev/538f0581a56b
https://hg.mozilla.org/mozilla-central/rev/3a69292cf6b4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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.
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce
Won't it be implemented into current release (v40) ??
(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.
Depends on: 1248927
You need to log in before you can comment on or make changes to this bug.