1421853 - subviewbutton in sidebarMenu-popup does not have checkmark

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[magicp]

Attachment: sidebarMenu-popup-subviewbutton.png

Steps to reproduce:
1. Launch the latest Nightly
2. Open Bookmarks sidebar (Ctrl + B)
3. Open sidebar menu

Actual Results:
Checkmark is not shown even if .subviewbutton[checked="true"].

Expected Results:
Checkmark should be shown.

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f673da3cfce0dae5731a4e3d06954a0a622166ea&tochange=0ee02ed83f71b4c4d2ed2d142eea26c5ff15081c

Comment 1

[:Paolo Amadini]

Attachment: Bug 1421853 - subviewbutton in sidebarMenu-popup does not have checkmark.

Since the sidebar menu is the only element using the small checkbox style, this is re-implemented placing the styles closer to the existing ones that are required for the checkbox to work. Some descendant selectors can be removed in the process.

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

Comment 2

[:Gijs (he/him)]

Comment on attachment 8934581 [details]
Bug 1421853 - subviewbutton in sidebarMenu-popup does not have checkmark.

https://reviewboard.mozilla.org/r/205472/#review211446

With the minor change below, r=me.

::: browser/themes/shared/sidebar.inc.css:105
(Diff revision 1)
> +#sidebarMenu-popup > .subviewbutton[checked="true"] {
> +  list-style-image: none;
> +  background: url(chrome://browser/skin/check.svg) center left 7px / 11px 11px no-repeat transparent;

It seems like most of this rule except the background position is duplicated on both sides of the if/else, and could be factored out into a separate rule above this %ifndef/else block. That would leave only the background position stuff in the platform-specific rules (and the margin/padding stuff for osx), which will make this easier to read, also because it'll make the non-rtl and rtl bits obvious mirrors of each other (ie center left / center right, respectively).

Comment 3

[:Paolo Amadini]

Comment on attachment 8934581 [details]
Bug 1421853 - subviewbutton in sidebarMenu-popup does not have checkmark.

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

Comment 4

[:Paolo Amadini]

Yes, I think this is easier to read, even if it duplicates the rule. I moved the background-size to its own line since it can't be specified in the shorthand without the position. I also swapped the order of the rules so the positions are all close to each other.

Let me know if you have any comments, I'll land this later today.

Comment 5

[:Gijs (he/him)]

Yep, looks good to me.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e262daafc582
subviewbutton in sidebarMenu-popup does not have checkmark. r=Gijs

Comment 7

[Dorel Luca [:dluca]]

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

Comment 8

[Mohammad Maruf Rahman]

I have reprodcued this bug with Nightly 59.0a1 (2017-11-29)on Windows 10, 64 Bit!

This bug's fix is verified with Latest Latest Nightly !

Build ID  : 20180106220101
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0