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

VERIFIED FIXED in Firefox 60

Status

()

Firefox
Bookmarks & History
P1
normal
VERIFIED FIXED
9 months ago
a month ago

People

(Reporter: Virtual, Assigned: Paolo)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {nightly-community, ux-consistency})

57 Branch
Firefox 61
x86_64
Windows 7
nightly-community, ux-consistency
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 verified, firefox61 verified)

Details

(Whiteboard: [reserve-photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

Created attachment 8899110 [details]
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
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
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.
Summary: "View Bookmarks Toolbar" item icon is missing in "Bookmarks Toolbar" item submenu in "Bookmarks" button menu → Make "View Bookmarks Toolbar" item active in "Bookmarks Toolbar" item submenu in "Bookmarks" button menu and add its proper new Photon item icon
Whiteboard: [photon] [triage] → [photon-visual] [photon-structure] [triage]

Updated

9 months ago
Flags: qe-verify+

Updated

9 months ago
Whiteboard: [photon-visual] [photon-structure] [triage] → [photon-structure] [triage]

Updated

9 months ago
Priority: -- → P4
Whiteboard: [photon-structure] [triage] → [reserve-photon-structure]
Blocks: 1352110
No longer blocks: 1349210

Updated

4 months ago
Priority: P4 → P5
(Assignee)

Updated

2 months ago
Blocks: 1448822
Comment hidden (mozreview-request)
(Assignee)

Comment 5

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

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

Comment 6

2 months ago
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?
Flags: needinfo?(gijskruitbosch+bugs)
(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...
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
(Assignee)

Updated

2 months ago
Priority: P5 → P1

Comment 8

2 months ago
(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

2 months ago
mozreview-review
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?
Attachment #8966952 - Flags: review?(mak77) → review+
(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.
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

2 months ago
Pushed by paolo.mozmail@amadzone.org:
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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/91cfe8e76dd1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Status: RESOLVED → VERIFIED
status-firefox57: affected → wontfix
status-firefox58: affected → wontfix
status-firefox59: affected → wontfix
status-firefox60: affected → wontfix
status-firefox61: fixed → verified
Virtual_Man: Please don't touch uplift flags for other releases when marking bugs as verified...
status-firefox60: wontfix → affected
(Assignee)

Comment 15

a month ago
Updating the bug title to match more precisely what we ended up doing.
Summary: Make "View Bookmarks Toolbar" item active in "Bookmarks Toolbar" item submenu in "Bookmarks" button menu and add its proper new Photon item icon → Don't use checkboxes for the "View Bookmarks Toolbar" and "View Bookmarks Sidebar" items in the Bookmarks button menu
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.
(Assignee)

Comment 17

a month ago
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.
Attachment #8966952 - Flags: approval-mozilla-beta?
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
Attachment #8966952 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 19

a month ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/43df2081c89e
status-firefox60: affected → fixed

Comment 20

a month ago
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?
Flags: needinfo?(Virtual)
Created attachment 8968427 [details]
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!
Flags: needinfo?(Virtual)
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.
status-firefox60: fixed → verified
(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

a month ago
Based on the previous comments, I will clear out the flag for qe-verify+.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.