1448822 - Checkmarks for menuitem.subviewbutton elements no longer appear (i.e. Bookmarks Star -> View Bookmarks Sidebar)

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

Reported over IRC, verified locally and bisected:

If you use the "Bookmarks menu" (the star), then enabling "Show Bookmarks sidebar" and "Bookmarks toolbar->Show Bookmarks toolbar" used to show a checkmark if those were enabled. They no longer do.

This was broken by bug 1416493.

Comment 1

[:Gijs (he/him)]

Brian, I thought we fixed this by putting things in the xul components css file? Can you take a look?

Comment 2

[Brian Grinstead [:bgrins]]

I've started to look into this and here's what I found so far:

At least on OSX, there are two rules that are fighting for priority on `- moz-appearance`for this element (menuitem#BMB_viewBookmarksSidebar):

1. panelUI.inc (document style): `.subviewbutton { -moz-appearance: none; }` at https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/browser/themes/shared/customizableui/panelUI.inc.css#775
2. menu.css (UA style): `:not(menulist) > menupopup > menuitem[checked="true"] { -moz-appearance: checkmenuitem; }` at https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/toolkit/themes/osx/global/menu.css#164 

Even though (2) has more specificity than (1), (1) gets applied due to stylesheet priority. What I haven't figured out is how this used to work when menu.css was loaded as a XBL style, since it would have the same problem.

I also think that Bug  1416493 (in Firefox 59) may not actually be the problem, since after that we were loading menu.css as a document style so we would get the checkmenuitem appearance. Bug 1420229 (in Firefox 60) where we moved menu.css into UA to more closely match XBL sheets is probably actually the problem at least on OSX.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: checkbox-moz-appearance-59.png

Shows styles applying in 59

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: checkbox-moz-appearance-61.png

Shows styles applying in 61

Comment 5

[Brian Grinstead [:bgrins]]

There's a similar situation on windows, where "checkmenuitem" set on `menuitem[type="checkbox"], menuitem[checked="true"]` gets overridden by that same subviewbutton class rule: https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/toolkit/themes/windows/global/menu.css#238

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: checkbox-moz-appearance-58.png

Turns out the -moz-appearance is none even in 58, where it works. In 58 we set: 

.subviewbutton[checked="true"] {
    background-position: top 7px left 4px;
}
.subviewbutton[checked="true"] {
    background: url(chrome://browser/skin/check.svg) center left 7px / 11px 11px no-repeat transparent;
        background-position-x: left 7px;
        background-position-y: center;
    fill: currentColor;
}

And that's what draws the actual check. Today though we use list-style-image and for some reason that isn't working (https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/browser/themes/shared/customizableui/panelUI.inc.css#916-917).

Comment 7

[Brian Grinstead [:bgrins]]

Gijs, do you know why the list-style-image referenced in Comment 6 isn't being shown in the bookmarks menu popup but is on, say, the sidebar switcher popup to indicate the selected sidebar? I've tried narrowing it down and nothing is jumping out. I think there's some special rules with list-style-image and XUL, right? That typically would only apply to elements with display: list-item.

Comment 8

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Gijs, do you know why the list-style-image referenced in Comment 6 isn't
> being shown in the bookmarks menu popup but is on, say, the sidebar switcher
> popup to indicate the selected sidebar?

As far as I can tell, the sidebar switcher is using background images?

https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/sidebar.inc.css#103-108

>  I've tried narrowing it down and
> nothing is jumping out. I think there's some special rules with
> list-style-image and XUL, right? That typically would only apply to elements
> with display: list-item.

list-style-image on a container like a menuitem or toolbarbutton gets used by a XUL <image> that's a descendant, via some logic that I'm not really sure of (presumably somewhere in the XUL C++ implementation?

Here, that descendant is inside the .menu-iconic-left, which in the case of the bookmarks menupopup dropdown, has both display:none and visibility:hidden. Removing both those properties in the devtools shows up the checkmark. Does that help? I don't know off-hand why the changes from bug 1416493 would have changed anything there.

Comment 9

[Brian Grinstead [:bgrins]]

AFAICT this wasn't caused by Bug 1416493, based on the fact that the checkbox shows up in 59 (as per attachment 8962457 [details]).

My best theory right now is that there are two moving parts here: we used to use background-image for subviewbuttons but now we use list-style-image as of 59. And we also had a temporary time when we loaded menu.css as a document style (in 59 / Bug 1416493) that may have papered over this change by forcing moz-appearance. Then when we restored the UA style behavior (in 60 / Bug 1420229), the moz-appearance went back and the list-style-image doesn't work (since it's doubly hidden both with display and visibility as per Comment 8).

I do see we force on background-image for sidebar switcher toolbarbuttons (last touched in Bug 1421853). Still not sure if the right thing to do here is to restore the visibility/display on the image to make the existing list-style-image work or to set background-image.

Comment 10

[Brian Grinstead [:bgrins]]

We are explicitly hiding image on checked menuitems with https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/browser/themes/shared/customizableui/panelUI.inc.css#1300-1302. I don't know why we do that.

Comment 11

[Brian Grinstead [:bgrins]]

I'm going to be unavailable until this coming Wednesday, so won't be working on this in the meantime. Gijs or Paolo: please feel free to take this if you have some cycles, otherwise I'll take a look at it again in a week or so.

I don't really understand the intention of styling as in Comment 10 - are menuitems with subviewbutton class supposed to each do their own thing to render checkboxes?

Comment 12

[:Paolo Amadini]

The .menu-iconic-left are used, and for the main menu like "Work Offline"

Comment 13

[:Paolo Amadini]

The .menu-iconic-left spaces are used for the main menu items like "Work Offline" to show a checkbox in place of the regular icon, and these are styled using "list-style-image". On Windows and Linux, the same system should work for the Bookmarks menu. This is not done on Mac because menu items with no icon don't have an empty space reserved for it. The sidebar is also a special case on all platforms, because we want to display both the checkbox and the icon at the same time.

So here there were various different issues, actually every platform had a different bug! The most interesting one seems to be that !important appearance rules in the user agent sheet couldn't be overridden, and this affected Linux.

Comment 14

[:Paolo Amadini]

Attachment: Bug 1448822 - Checkmarks for menuitem.subviewbutton elements no longer appear.

On Windows, where the checkbox appears in the space reserved for the icon, a new rule is added to ensure that the icon is displayed.

On Mac, where there is no space reserved for the icon, the system checkbox appearance is used, even though this causes the item to change the background color when checked.

On Linux, the checkbox properties in the "menu.css" user agent sheet are no longer marked as important so they can be overridden by regular stylesheets, and the checkbox can appear in the space reserved for the icon.

An unneeded rule for .toolbarbutton-1[checked=true] is also removed.

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

Comment 15

[:Gijs (he/him)]

Comment on attachment 8966191 [details]
Bug 1448822 - Checkmarks for menuitem.subviewbutton elements no longer appear.

https://reviewboard.mozilla.org/r/234936/#review240680

I don't think the macOS fix is workable. There's also some issues with the other bits...

::: commit-message-30d72:5
(Diff revision 1)
> +Bug 1448822 - Checkmarks for menuitem.subviewbutton elements no longer appear. r=Gijs
> +
> +On Windows, where the checkbox appears in the space reserved for the icon, a new rule is added to ensure that the icon is displayed.
> +
> +On Mac, where there is no space reserved for the icon, the system checkbox appearance is used, even though this causes the item to change the background color when checked.

This looks really jarring, especially on mouseover, when the appearance is blue, vs. all the other items (grey). It'll be worse with theming. I don't think this is an OK regression.

::: browser/themes/windows/customizableui/panelUI.css:48
(Diff revision 1)
>    /* This is 16px for an icon + 3px for its margins + 1px for its padding +
>     * 2px for its border, see above */
>    min-height: 22px;
>  }
>  
> +menuitem[checked="true"].subviewbutton > .menu-iconic-left > .menu-iconic-icon {

This could do without the tagname selector. Just `.subviewbutton[checked=true]` would do, because no other type of item would have a `.menu-iconic-icon` descendant anyway, and this will still override all the other rules that I'm seeing in the inspector.

::: toolkit/themes/linux/global/menu.css:172
(Diff revision 1)
>  }
>  
>  /* ::::: checkbox menuitem ::::: */
>  
>  menuitem[checked="true"] {
> -  -moz-appearance: checkmenuitem !important;
> +  -moz-appearance: checkmenuitem;

While I'm supportive of this type of change generally, this seems scary to uplift to 60 at this point.

Comment 16

[:Paolo Amadini]

(In reply to :Gijs (he/him) from comment #15)
> This looks really jarring, especially on mouseover, when the appearance is
> blue, vs. all the other items (grey). It'll be worse with theming. I don't
> think this is an OK regression.

Except this is what we do on Release right now...

> ::: toolkit/themes/linux/global/menu.css:172
> (Diff revision 1)
> >  }
> >  
> >  /* ::::: checkbox menuitem ::::: */
> >  
> >  menuitem[checked="true"] {
> > -  -moz-appearance: checkmenuitem !important;
> > +  -moz-appearance: checkmenuitem;
> 
> While I'm supportive of this type of change generally, this seems scary to
> uplift to 60 at this point.

I don't know if this is by design, but what I've observed is that since bug 1420229 turned this into a UA stylesheet the "!important" rules cannot be overridden by regular style sheets, so I don't see any other way of restoring the behavior we had before without backing out bug 1420229. I can update the patch so we leverage the "checkmenuitem" appearance instead of showing our own checkbox, but this would also be different from what we were doing before.

Comment 17

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #16)
> I can update the patch so we leverage the "checkmenuitem" appearance instead of
> showing our own checkbox

...and actually I've just verified that changing the appearance creates a misalignment in the text of the item. We could try to compensate for it but I'm not sure if we can choose a value that works for all themes.

Comment 18

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #16)
> (In reply to :Gijs (he/him) from comment #15)
> > This looks really jarring, especially on mouseover, when the appearance is
> > blue, vs. all the other items (grey). It'll be worse with theming. I don't
> > think this is an OK regression.
> 
> Except this is what we do on Release right now...

Maybe, but it wasn't on earlier versions, and we have no way of fixing it while keeping the moz-appearance, so I'd rather not dig us into that hole. Why not fix it the right way while we're here? :-)

> > ::: toolkit/themes/linux/global/menu.css:172
> > (Diff revision 1)
> > >  }
> > >  
> > >  /* ::::: checkbox menuitem ::::: */
> > >  
> > >  menuitem[checked="true"] {
> > > -  -moz-appearance: checkmenuitem !important;
> > > +  -moz-appearance: checkmenuitem;
> > 
> > While I'm supportive of this type of change generally, this seems scary to
> > uplift to 60 at this point.
> 
> I don't know if this is by design, but what I've observed is that since bug
> 1420229 turned this into a UA stylesheet the "!important" rules cannot be
> overridden by regular style sheets, so I don't see any other way of
> restoring the behavior we had before without backing out bug 1420229.

Brian, do you have ideas about this?

> I can
> update the patch so we leverage the "checkmenuitem" appearance instead of
> showing our own checkbox, but this would also be different from what we were
> doing before.

I'm not sure what you mean by this. To be clear, I think it's fine (probably not just fine but even "necessary") to use moz-appearance:none for the items in the panel - but potentially having it be overridden "elsewhere" (which is hard to audit for) is what's scary.

Comment 19

[:Paolo Amadini]

(In reply to :Gijs (he/him) from comment #18)
> (In reply to :Paolo Amadini from comment #16)
> > (In reply to :Gijs (he/him) from comment #15)
> > > This looks really jarring, especially on mouseover, when the appearance is
> > > blue, vs. all the other items (grey). It'll be worse with theming. I don't
> > > think this is an OK regression.
> > 
> > Except this is what we do on Release right now...
> 
> Maybe, but it wasn't on earlier versions, and we have no way of fixing it
> while keeping the moz-appearance, so I'd rather not dig us into that hole.
> Why not fix it the right way while we're here? :-)

Ah, right, I lost that bit. I also found a related moving part in bug 1421853.

> > I can
> > update the patch so we leverage the "checkmenuitem" appearance instead of
> > showing our own checkbox, but this would also be different from what we were
> > doing before.
> 
> I'm not sure what you mean by this.

This is basically the behavior that is currently on Beta.

> To be clear, I think it's fine (probably
> not just fine but even "necessary") to use moz-appearance:none for the items
> in the panel - but potentially having it be overridden "elsewhere" (which is
> hard to audit for) is what's scary.

Yeah, and to be fair the misalignment is not too bad, so we might also consider keeping the current "regression" that shows the platform checkmark on Linux. On Release on Linux we currently show no checkbox at all.

(All in all it seems that most users won't care, the items seem to work just fine without the checkbox if you don't know it should be there... it's pretty obvious what is the state of the sidebar and the toolbar in fact.)

Comment 20

[:Paolo Amadini]

Comment on attachment 8966191 [details]
Bug 1448822 - Checkmarks for menuitem.subviewbutton elements no longer appear.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234936/diff/1-2/

Comment 21

[:Paolo Amadini]

I also have a patch where I'm trying to get rid of the checkbox and use the same system we use in the Bookmarking Tools menu, using updateToggleControlLabel. Unfortunately, the Linux style sheets only need the menuitem to have checked="true" to force the appearance, independently of type="checkbox", so this isn't working unless we also get rid of the "checked" attribute, which is currently set by a XUL broadcaster element...

Comment 22

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #21)
> I also have a patch where I'm trying to get rid of the checkbox and use the
> same system we use in the Bookmarking Tools menu, using
> updateToggleControlLabel. Unfortunately, the Linux style sheets only need
> the menuitem to have checked="true" to force the appearance, independently
> of type="checkbox", so this isn't working unless we also get rid of the
> "checked" attribute, which is currently set by a XUL broadcaster element...

Yeah, this would be another way of doing this, ie fixing bug 1391948 and then just removing the checkbox appearance entirely. The broadcasting stuff we could fix by removing the observes (or replacing command= with oncommand= or some other listener; not sure how the broadcaster is hooked up), and updating the state of the item onpopupshowing. Do you think that's a better approach than the patch you attached? I'm happy to go either way, perhaps slightly leaning towards bug 1391948.

Comment 23

[:Paolo Amadini]

Hm, all in all I'm also leaning towards bug 1391948, getting rid of the type="checkbox" menuitems and updating the items manually.

Comment 24

[:Paolo Amadini]

Attachment: Bug 1448822 - Remove the checkbox styling in the menu of the Bookmarks button.

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

Comment 25

[:Paolo Amadini]

The checked part of "panelview .toolbarbutton-1:-moz-any(@buttonStateActive@,[checked=true])" was simply unused. Let me know if I missed anything else that should be removed.

If we fix bug 1391948 we probably don't need to uplift this part. Based on what Brian says, we may consider removing the "!important" rules on Linux so we improve our ability to change the styling of elements in the future.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8966954 [details]
Bug 1448822 - Remove the checkbox styling in the menu of the Bookmarks button.

https://reviewboard.mozilla.org/r/235638/#review241360

Thanks!

Comment 27

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #25)
> The checked part of "panelview
> .toolbarbutton-1:-moz-any(@buttonStateActive@,[checked=true])" was simply
> unused. Let me know if I missed anything else that should be removed.

I assume you checked that it's also not used by the sidebar dropdown in the browser sidebar?

Comment 28

[:Paolo Amadini]

Right, that's one of the places I'm testing for regressions. There's no special background there for the checked elements.

Comment 29

[Brian Grinstead [:bgrins]]

Thanks Paolo and Gijs for figuring this out!

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dce413699c31
Remove the checkbox styling in the menu of the Bookmarks button. r=Gijs

Comment 31

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/dce413699c31

Comment 32

[:Paolo Amadini]

Just a footnote that the various regressions mentioned here were made irrelevant by bug 1391948, and the patch in this bug only contains cleanup that doesn't require uplift to Beta.