1391948 - Don't use checkboxes for the "View Bookmarks Toolbar" and "View Bookmarks Sidebar" items in the Bookmarks button menu

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/message/reply/question/etc.)]

Attachment: screenshot.png

"View Bookmarks Toolbar" item icon is missing in "Bookmarks Toolbar" item submenu in "Bookmarks" button menu

The proper icon for "View Bookmarks Toolbar" item is shown in:
- "Hamburger" button menu => "Library" item menu => "Bookmarks" item menu => "Bookmarking tools" item menu
- "Library" button menu => "Bookmarks" item menu => "Bookmarking tools" item menu

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/message/reply/question/etc.)]

Attachment: screenshot2.png

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/message/reply/question/etc.)]

Attachment: screenshot3.png

Comment 3

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/message/reply/question/etc.)]

Also this "View Bookmarks Toolbar", should be active, like the same items located in:
- "Hamburger" button menu => "Library" item menu => "Bookmarks" item menu => "Bookmarking tools" item menu
- "Library" button menu => "Bookmarks" item menu => "Bookmarking tools" item menu,
so there should be "View Bookmarks Toolbar" and "Hide Bookmarks Toolbar", instead tick option.

Comment 4

[:Paolo Amadini]

Attachment: Bug 1391948 - Don't use checkboxes for the "View Bookmarks Toolbar" and "View Bookmarks Sidebar" items in the Bookmarks button menu.

Review commit: https://reviewboard.mozilla.org/r/235636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235636/

Comment 5

[:Paolo Amadini]

I haven't run tests yet, but asking for review in the meantime. Tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=505f13777a0de15f66fc55de52aacf2001607e35

Comment 6

[:Paolo Amadini]

I ended up rewriting a bunch of code since it turns out what we do here is actually quite straightforward and similar to the Bookmarking Tools manu, it just used to be done in both places in a convoluted way with unneeded observers, "checked" attributes, and calls to updateToggleControlLabel.

Since this code is limited to the feature, I think it's quite upliftable, probably more than removing "!important" in the UA style sheet on Linux. Gijs, do you agree we can uplift this?

Comment 7

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #6)
> I ended up rewriting a bunch of code since it turns out what we do here is
> actually quite straightforward and similar to the Bookmarking Tools manu, it
> just used to be done in both places in a convoluted way with unneeded
> observers, "checked" attributes, and calls to updateToggleControlLabel.
> 
> Since this code is limited to the feature, I think it's quite upliftable,
> probably more than removing "!important" in the UA style sheet on Linux.
> Gijs, do you agree we can uplift this?

Yes. For Marco's context, this would not entirely coincidentally fix the issue with the appearance of these items which is tracked in bug 1448822. That's a regression in 59/60-ish (it's a bit messy). So if we can uplift to 60 that would be nice.

We're sort of lucky here that there's no access keys...

Comment 8

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #7)
> Yes. For Marco's context, this would not entirely coincidentally fix the
> issue with the appearance of these items which is tracked in bug 1448822.
> That's a regression in 59/60-ish (it's a bit messy). So if we can uplift to
> 60 that would be nice.

It's unclear to me what we want in the end, because for example the Library / Bookmarking Tools doesn't have a checkbox, it changes the text from "show" to "hide" and has a common toolbar icon in both cases.
I guess we are removing the checkbox and making everything coherent with the Bookmarking Tools menu?

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8966952 [details]
Bug 1391948 - Don't use checkboxes for the "View Bookmarks Toolbar" and "View Bookmarks Sidebar" items in the Bookmarks button menu.

https://reviewboard.mozilla.org/r/235636/#review241854

Overwall it looks much simpler. The only downside I see is that this reads the collapsed status of PersonalToolbar every time the bookmarks menu is opened, and I wonder if in some cases that may cause a synchronous layout flush. But since the bookmarks menu is not in the toolbar by default I guess we don't care that much. In the future we could probably store somewhere a cache of the toolbars status, if setToolbarVisibility is the only point where we change it.

::: browser/base/content/browser-places.js:1644
(Diff revision 1)
> -          break;
> -        case "panelMenu_viewBookmarksSidebar":
> -          button.setAttribute("checked", SidebarUI.currentID == "viewBookmarksSidebar");
> -          break;
> -        case "panelMenu_viewBookmarksToolbar":
> -          let toolbar = document.getElementById("PersonalToolbar");
> +                     placement && placement.area == CustomizableUI.AREA_NAVBAR);
> +    this.selectLabel("panelMenu_viewBookmarksSidebar",
> +                     SidebarUI.currentID == "viewBookmarksSidebar");
> +    this.selectLabel("panelMenu_viewBookmarksToolbar",
> +                     !document.getElementById("PersonalToolbar").collapsed);
> +    PanelUI.showSubView("PanelUI-bookmarkingTools", triggerNode);

In the end, looks like selectLabel is just a different implementation of updateToggleControlLabel for elements that don't have a "checked" attribute, but probably shorter than if (cond) {setAttribute} else {rmAttribute};updateToggleControlLabel(elm).
Did I get it right?

Comment 10

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #8)
> (In reply to :Gijs (he/him) from comment #7)
> > Yes. For Marco's context, this would not entirely coincidentally fix the
> > issue with the appearance of these items which is tracked in bug 1448822.
> > That's a regression in 59/60-ish (it's a bit messy). So if we can uplift to
> > 60 that would be nice.
> 
> It's unclear to me what we want in the end, because for example the Library
> / Bookmarking Tools doesn't have a checkbox, it changes the text from "show"
> to "hide" and has a common toolbar icon in both cases.
> I guess we are removing the checkbox and making everything coherent with the
> Bookmarking Tools menu?

Yes, given that the BM Tools thing is the "new" way of doing it, I think the consistency is the right direction here. It also helps with bug 1448822 because the styling of [checked] menuitems (or toolbar buttons pretending to be menu items) is a bit of a mess.

Comment 11

[Marco Bonardo [:mak]]

nvm the "flush" problem, I just realized we are just reading a property here, that will likely read an attribute, still some overhead, but should be harmless.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91cfe8e76dd1
Don't use checkboxes for the "View Bookmarks Toolbar" and "View Bookmarks Sidebar" items in the Bookmarks button menu. r=mak

Comment 13

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/91cfe8e76dd1

Comment 14

[:Gijs (he/him)]

Virtual_Man: Please don't touch uplift flags for other releases when marking bugs as verified...

Comment 15

[:Paolo Amadini]

Updating the bug title to match more precisely what we ended up doing.

Comment 16

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/message/reply/question/etc.)]

There isn't uplift request for patch and this bug was some kind of feature request, so basing on that I marked it as wontfix for older builds.

Comment 17

[:Paolo Amadini]

Comment on attachment 8966952 [details]
Bug 1391948 - Don't use checkboxes for the "View Bookmarks Toolbar" and "View Bookmarks Sidebar" items in the Bookmarks button menu.

Approval Request Comment
[Feature/Bug causing the regression]:

We shipped various regressions in Firefox 59 all related to checkboxes in the so-called "Bookmarks Menu", which is the one opened with a button that is not present by default in the toolbar.

The regressions are different on every platform, and more are scheduled for release in Firefox 60. They are caused by several different bugs and more details can be found in the original report in bug 1448822.

We ended up determining that we don't really need these checkboxes, so we just updated the interface so we don't use them anymore. This is easier to uplift than just fixing the styles, because the latter would require us to make changes in files that may potentially affect the rest of the user interface.

[User impact if declined]:

The most notable is the jarring item background on Mac that we shipped already, see bug 1448822 comment 18. On other platforms the checkbox does not appear or the item is misaligned.

[Is this code covered by automated tests?]:

The button styling is not covered.

[Has the fix been verified in Nightly?]:

Yes, and a small follow-up was filed for Windows in bug 1454148.

[Needs manual test from QE? If yes, steps to reproduce]:

Yes, place the "Bookmarks Menu" button in the toolbar, toggle the "View Bookmarks Sidebar" item and the "View Bookmarks Toolbar" submenu item, and check that they don't have a checkbox and their appearance does not change, but only their label is different.

Do the same for the items in the "Bookmarking Tools" submenu of the default "Bookmarks" view of the "Library" panel, whose behavior shouldn't have changed.

[List of other uplifts needed for the feature/fix]:

Bug 1454148, which at the moment just landed in mozilla-inbound.

[Is the change risky?]:
[Why is the change risky/not risky?]:

Risk is limited to the feature, we're only modifying the menu code and not the styling rules. We are also reusing existing strings that were already made for the purpose in other buttons in the user interface that didn't use a checkbox already. Any last-minute styling follow-up would be trivial like the one in bug 1454148.

[String changes made/needed]:

None.

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8966952 [details]
Bug 1391948 - Don't use checkboxes for the "View Bookmarks Toolbar" and "View Bookmarks Sidebar" items in the Bookmarks button menu.

remove checkboxes from bookmarks menu to sidestep regressions, approved for 60.0b13

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/43df2081c89e

Comment 20

[Oana Botisan, Desktop QA]

I managed to reproduce the bug using an older version of Nightly 2017-08-19 on Windows 10 x64.

I retested everything on latest Nightly 61.0a1 and beta 60.0b13 using Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12 and they don't have a checkbox. But on the other hand I've noticed a few things:

1. [Win, Ubuntu, mac] When Bookmarks Menu is placed on the toolbar -> View/Hide Bookmarks Sidebar, View/Hide Bookmarks toolbar don't have an icon. Even though they have one in Library -> Bookmarks -> Bookmarking Tools. 
2. [mac] The text for Show All Bookmarks and View/Hide Bookmarks Sidebar is not aligned with the rest of the text from the Bookmarks Menu doorhanger.
3. [Win] The shortcut for Show All Bookmarks (Crtl+Shift+B) is not placed on the righter side of the Bookmarks Menu doorhanger, just like on Ubuntu and macOS.

Is this the expected behaviour? These differences appear because of the differences between the interfaces of the platforms?

Comment 21

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/message/reply/question/etc.)]

Attachment: screenshot.png

(In reply to Oana Botisan from comment #20)
> 1. [Win, Ubuntu, mac] When Bookmarks Menu is placed on the toolbar ->
> View/Hide Bookmarks Sidebar, View/Hide Bookmarks toolbar don't have an icon.
> Even though they have one in Library -> Bookmarks -> Bookmarking Tools.
This inconsistency is now tracked in bug #1454225.

(In reply to Oana Botisan from comment #20)
> 2. [mac] The text for Show All Bookmarks and View/Hide Bookmarks Sidebar is
> not aligned with the rest of the text from the Bookmarks Menu doorhanger.
I can't reproduce it in Mozilla Firefox Nightly 61.0a1 (2018-04-16) (64-bit) on Windows 7 (64-bit).
But I'm on Windows system and this looks like bug specific to Mac system, as you already wrote, so please create new bug about it.

(In reply to Oana Botisan from comment #20)
> 3. [Win] The shortcut for Show All Bookmarks (Crtl+Shift+B) is not placed on
> the righter side of the Bookmarks Menu doorhanger, just like on Ubuntu and
> macOS.
I also can't reproduce it in Mozilla Firefox Nightly 61.0a1 (2018-04-16) (64-bit) on Windows 7 (64-bit).
See attached screenshot how it looks on my end.

(In reply to Oana Botisan from comment #20)
> Is this the expected behaviour? These differences appear because of the
> differences between the interfaces of the platforms?
In 99,9% everything have to be consistent, if it not, please report bugs about them.

Thanks!

Comment 22

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/message/reply/question/etc.)]

Also I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 61.0a1 (2018-04-13), so I'm marking this bug as VERIFIED. Same as in latest Mozilla Firefox Beta. Thanks.

Comment 23

[:Gijs (he/him)]

(In reply to Oana Botisan from comment #20)
> 2. [mac] The text for Show All Bookmarks and View/Hide Bookmarks Sidebar is
> not aligned with the rest of the text from the Bookmarks Menu doorhanger.

I would expect the text to be aligned with the text in the "Show all bookmarks" footer (which is the only other item without icons) and that seems to be the case on nightly, at least, so I don't think this is a bug.

Comment 24

[Oana Botisan, Desktop QA]

Based on the previous comments, I will clear out the flag for qe-verify+.

Comment 25

[:Gijs (he/him)]

Marco, can you sanity-check me here? I'm looking at this because I noticed the patch in https://phabricator.services.mozilla.com/D28177 (bug 1547496) which uses updateToggleControlLabel. As far as I can tell, that is used from e.g. https://searchfox.org/mozilla-central/rev/66086345467c69685434dd1c5177b30a7511b1a5/browser/base/content/browser.js#5989 for e.g. https://searchfox.org/mozilla-central/rev/66086345467c69685434dd1c5177b30a7511b1a5/browser/base/content/browser.xul#1249-1250 (and various other places changed by this patch). But updateToggleControlLabel wasn't updated and I don't see label-(un)checked in use anywhere, except the additions from bug 1533533. Was not updating updateToggleControlLabel just an oversight? Should it be removed from all the callsites that currently call it that never call it with an element that has a label-checked anyway?

Comment 26

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #25)

Marco, can you sanity-check me here? I'm looking at this because I noticed the patch in https://phabricator.services.mozilla.com/D28177 (bug 1547496) which uses updateToggleControlLabel. As far as I can tell, that is used from e.g. https://searchfox.org/mozilla-central/rev/66086345467c69685434dd1c5177b30a7511b1a5/browser/base/content/browser.js#5989 for e.g. https://searchfox.org/mozilla-central/rev/66086345467c69685434dd1c5177b30a7511b1a5/browser/base/content/browser.xul#1249-1250 (and various other places changed by this patch). But updateToggleControlLabel wasn't updated and I don't see label-(un)checked in use anywhere, except the additions from bug 1533533. Was not updating updateToggleControlLabel just an oversight? Should it be removed from all the callsites that currently call it that never call it with an element that has a label-checked anyway?

I'm not sure I follow the whole investigation, updateToggleControlLabel seems to be a util for elements having a "checked" attribute, that includes toolbarbuttons, menuitems and checkboxes. The patch here used its own implementation with label-show and label-hide and removed type="checkbox" from the menuitems, Paolo pointed out the existing system was more complex than just flipping the label, and we were more coherent with other bookmarks options by removing the checkbox. But he didn't remove the original code and updateToggleControlLabel stayed there.
If new consumers use a checkbox menuitems it seems to make sense to use updateToggleControlLabel, rather than reinventing the wheel.
If onViewToolbarCommand never acts on nodes having a label-checked, it should probably not invoke updateToggleControlLabel. We likely missed removing that.