964887 - Bookmark submenu says "Bookmark this page" even for bookmarked pages

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Madhava Enros [:madhava]]

When the bookmarking button is in the menu panel rather than the toobar, the first item in its submenu is "Bookmark this page." Unfortunately, it remains saying "Bookmark this page" even when the current page is bookmarked (clicking it brings up the bookmarking doorhanger with the correct "Edit this bookmark" header).

We could change the text to either
(a) Remove bookmark  OR
(b) Edit this bookmark

I think the latter makes sense, for now, in that we already have that string and that's the doorhanger that comes up when you click on the menu item. The doorhanger itself provides the ability to remove the bookmark.

Comment 1

[:Gijs (he/him)]

NB: I noticed that this behaviour also happens with the 'old' menu bar's bookmark menu, at least on OS X.

Comment 2

[:Gijs (he/him)]

Attachment: fix menu in Australis to say 'Edit this bookmark' for pages which have already been bookmarked,

So this would work if it weren't for the fact that this.itemIds is a lie when the star button is in the panel from the start, because then we never register a bookmarks observer and we never update the information. I'll go and fix that in another patch, which will basically be an update of the one for bug 964218. For now, when testing this, as long as you start with the bookmarks button in the toolbar, this should let you test the functionality. I've gone ahead and also updated the main menu to behave the same way.

Comment 3

[:Gijs (he/him)]

Attachment: fix menu in Australis to say 'Edit this bookmark' for pages which have already been bookmarked,

Forgot to update the onMainBookmarksMenuShowing method. This should be the correct patch.

Comment 4

[:Gijs (he/him)]

Attachment: fix menu in Australis to say 'Edit this bookmark' for pages which have already been bookmarked,

... however, reading the comments in bug 964218, it seems that we *intend* not to update this._itemIds unless we're in the toolbar. I've updated the code to always use another mechanism of checking whether the current page is bookmarked.

Comment 5

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

Comment on attachment 8368283 [details] [diff] [review]
fix menu in Australis to say 'Edit this bookmark' for pages which have already been bookmarked,

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

This is causing main-thread IO :(

::: browser/base/content/browser-menubar.inc
@@ -371,5 @@
>                  command="Browser:ShowAllBookmarks"
>                  key="manBookmarkKb"/>
>        <menuseparator id="organizeBookmarksSeparator"/>
>        <menuitem id="menu_bookmarkThisPage"
> -                label="&bookmarkThisPageCmd.label;"

why changing this and not touching panelMenuBookmarkThisPage? Was not this bug about the menupanel view?
You should probably do both. At that point bookmarkThisPageCmd.label is unused and should be removed... though, what about using a broadcaster, with bookmarkThisPageCmd.label as the default label value, and just swap label value in browser-places.js?

::: browser/base/content/browser-places.js
@@ +1007,5 @@
>      return this._unstarredTooltip =
>        gNavigatorBundle.getString("starButtonOff.tooltip");
>    },
>  
> +  get _menuBookmarkThisPage()

add Label suffix please

@@ +1014,5 @@
> +    return this._menuBookmarkThisPage =
> +      gNavigatorBundle.getString("menuBookmarkThisPage.label");
> +  },
> +
> +  get _menuEditBookmark()

ditto

@@ +1034,5 @@
>    },
>  
> +  onMainBookmarksMenuShowing: function PCH_onMainBookmarksMenuShowing(event) {
> +    // There's nothing to do in non-browser windows, or for submenus
> +    if (window.location.href != getBrowserURL() ||

this means the menubar on mac will retain the wrong label, or even worse no label at all (since this is always skipped and you removed the default label)?

while there is a valid reason to not updateBookmarkAllTabsCommand in that case (it's a permanently disabled command in non-browser windows), I think you can remove this check, just please test on Mac native menubar the menu is working properly.

@@ +1041,5 @@
> +    }
> +
> +    let menuItem = document.getElementById("menu_bookmarkThisPage");
> +    let uri = gBrowser.selectedBrowser.currentURI;
> +    let isStarred = PlacesUtils.getMostRecentBookmarkForURI(uri) != -1;

no, no, no.
This is a synchronous method that will hang the browser every time you open the menu.

Rather than this, I prefer removing the optimization where we don't keep _itemIds updated, after all, when the star was in the locationbar, it was always visible.
Moreover, we didn't have reasons to keep _itemIds updated, so it was correct to optimize it, now we DO have reasons (updating this label and according to what I saw on irc, we may want to keep the star up-to-date in the menupanel), so makes sense to re-evaluate such decision.
Just make _itemdIds stay up-to-date and remove complication due to the old behavior.

@@ +1045,5 @@
> +    let isStarred = PlacesUtils.getMostRecentBookmarkForURI(uri) != -1;
> +    let desiredLabel = isStarred ? this._menuEditBookmark
> +                                 : this._menuBookmarkThisPage;
> +    menuItem.setAttribute("label", desiredLabel);
> +    PlacesCommandHook.updateBookmarkAllTabsCommand();

Since you will have to update the label also when opening the bookmarks view in the panel ui, I suggest you to keep this into the original position, add another call into that popupshowing handler that will be the same method you will call from panelui

@@ +1249,5 @@
>      let widget = widgetGroup.forWindow(window);
>      if (widgetGroup.areaType == CustomizableUI.TYPE_MENU_PANEL) {
>        let view = document.getElementById("PanelUI-bookmarks");
> +      view.addEventListener("ViewShowing", this);
> +      view.addEventListener("ViewHiding", this);

why this change, doesn't look useful per se

@@ +1293,5 @@
>        viewToolbar.setAttribute("checked", "true");
> +
> +    let bookmarkElement = document.getElementById("panelMenuBookmarkThisPage");
> +    let uri = gBrowser.selectedBrowser.currentURI;
> +    let isStarred = PlacesUtils.getMostRecentBookmarkForURI(uri) != -1;

ugh!

@@ +1310,5 @@
>  
>    onPanelMenuViewHiding: function BUI_onViewHiding(aEvent) {
>      this._panelMenuView.uninit();
>      delete this._panelMenuView;
> +    aEvent.target.removeEventListener("ViewHiding", this);

why do we remove only this and not ViewShowing?

Comment 6

[:Gijs (he/him)]

Attachment: fix menu in Australis to say 'Edit this bookmark' for pages which have already been bookmarked, * *

I went with updating the broadcaster onpopupshowing because it seemed the easiest non-churny fix for the issue we discussed on IRC (ie updating it for about:blank/about:newtab). I added some more checks in the bookmarks listeners to avoid calling updateStar too often. The viewshowing listener removal was at the top of viewshowing in the previous patch, I've moved it down to the bottom. I also decided that for a broadcaster, reusing the existing string and adding the other in the dtd made more sense, so I've done that instead.

Comment 7

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

Comment on attachment 8368620 [details] [diff] [review]
fix menu in Australis to say 'Edit this bookmark' for pages which have already been bookmarked,  * *

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

Touching this code is prone to small regressions, but I think it will be that kind of things we can fix with trivial patches, we should just take care to examine incoming bugs for eventual regressions and prioritize those sooner than later.
There is some test using BookmarkingUI, so I suggest doing a tryrun before the real push, just in case.

::: browser/base/content/browser-menubar.inc
@@ +375,2 @@
>                  command="Browser:AddBookmarkAs"
> +                key="addBookmarkAsKb" observes="bookmarkThisPageBroadcaster"/>

please put it on a newline

::: browser/base/content/browser-places.js
@@ +1228,3 @@
>  
> +    let isStarred = this._itemIds.length > 0;
> +    if (isStarred) {

for a single use the temp var is not useful, it would be if it was reused later...

@@ +1237,5 @@
>      }
>    },
>  
> +  _updateBookmarkPageMenuItem: function BUI__updateBookmarkPageMenuItem(forceReset) {
> +    let broadcaster = document.getElementById("bookmarkThisPageBroadcaster");

may move to a lazy getter

@@ +1238,5 @@
>    },
>  
> +  _updateBookmarkPageMenuItem: function BUI__updateBookmarkPageMenuItem(forceReset) {
> +    let broadcaster = document.getElementById("bookmarkThisPageBroadcaster");
> +    let isStarred = !forceReset && this._itemIds && this._itemIds.length > 0;

I'd appreciate a comment about when forceReset it used and why, for future reference

@@ +1239,5 @@
>  
> +  _updateBookmarkPageMenuItem: function BUI__updateBookmarkPageMenuItem(forceReset) {
> +    let broadcaster = document.getElementById("bookmarkThisPageBroadcaster");
> +    let isStarred = !forceReset && this._itemIds && this._itemIds.length > 0;
> +    let desiredLabel = isStarred ? "editlabel" : "bookmarklabel";

nit: just label? :)

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +85,5 @@
>  
>      <panelview id="PanelUI-bookmarks" flex="1" class="PanelUI-subView">
>        <label value="&bookmarksMenu.label;" class="panel-subview-header"/>
>        <toolbarbutton id="panelMenuBookmarkThisPage"
> +                     class="subviewbutton" observes="bookmarkThisPageBroadcaster"

please on a new line

Comment 8

[:Gijs (he/him)]

remote:   https://tbpl.mozilla.org/?tree=Try&rev=4c0fffac1775

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #8)
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=4c0fffac1775

m-bc failures across the board. Investigating.

Comment 10

[:Gijs (he/him)]

Added a nullcheck, and another try run:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=bb8943906a72


We now call this._updateStar in the "not disabled" case for onPageProxyStateChanged. This happens early on new window open, and breaks stuff. The nullcheck should be enough to fix this.

Comment 11

[:Gijs (he/him)]

Try was green, so pushed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/431244ecfe99

Comment 12

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/431244ecfe99

Comment 13

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=33911990&tree=Fx-Team#error0 - this added two or four "this._itemIds is undefined" to every browser-chrome run, green or orange, the better to distract people looking for something other than their own failure to blame.

Comment 14

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

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Microsoft Surface Pro 2 device)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0

Verified as fixed on latest Aurora (build ID: 20140303004002).