979378 - [Australis] Update Panel Checkbox

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Stephen Horlander [:shorlander]]

Attachment: Panel Menu Item Checkmark

Update the menu panel checkmark to a consistent item that fits with the panel style.

Comment 1

[Stephen Horlander [:shorlander]]

Attachment: Panel Menu Item Checkmark @2x

Comment 2

[Blake Winton (:bwinton) (:☕️)]

(WIP up at http://people.mozilla.org/~bwinton/temp/bug979378.patch, building on Windows now.
I'll check the results tomorrow morning.)

Comment 5

[Justin Dolske [:Dolske]]

Sounds like bug 978566 and bug 938603 should be fixed by this, please check and reopen (or just fix here :) if not.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

That's my understanding.

The main blocker on this (which I will push on tomorrow), is getting shorlander to decide what we want to do on each platform.  (For instance, if we follow system convention then on Windows *every* menu has space on the left for icons whether they have icons or not.  Is that what we want to do?  Maybe…  :)

Comment 7

[Blake Winton (:bwinton) (:☕️)]

So, here's the spec, straight from Stephen's mouth:

Things which can be checked should have a check everywhere.
None of this active-state-when-checked styling.

OS X: flush-left when not checked, lined up with icon-text when checked.
Windows/Linux: lined up with icon-text whether checked or not.

Comment 8

[u428464]

On Windows there is also an alignment problem with the disabled "Subscribe To This Page" item that doesn't have any icons. I don't know if it should also be aligned or a greyed out icon should be added.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

A modification to the spec, again from Stephen.

Windows/Linux: always a gutter containing either an icon or a check.
OS X: always a gutter for just the check, then icon (potentially) beside, lined up with the labels.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug979378.patch

Wow, that took a while.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8387169 [details] [diff] [review]
bug979378.patch

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

r+ assuming ui-r+,

here's some nits:

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +77,5 @@
> +}
> +
> +#PanelUI-recentlyClosedWindows > .subviewbutton.restoreallitem > .toolbarbutton-icon,
> +#PanelUI-recentlyClosedTabs > .subviewbutton.restoreallitem > .toolbarbutton-icon,
> +#PanelUI-historyItems > .subviewbutton.restoreallitem > .toolbarbutton-icon {

display: none, and without the #id selector + child selector

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +881,5 @@
>    background-size: 1px 18px;
>    box-shadow: 0 0 0 1px hsla(0,0%,100%,.2);
>  }
>  
> +.PanelUI-subView toolbarbutton[checked="true"],

Can this just use .subview-button[checked="true"] ?

::: browser/themes/windows/customizableui/panelUIOverlay.css
@@ +34,5 @@
>  .widget-overflow-list .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon {
>    padding: 0 6px;
>  }
>  
> +.subviewbutton > .toolbarbutton-text {

Nit: make these hunks insert in the same place in Windows+Linux.

Comment 12

[Stephen Horlander [:shorlander]]

Comment on attachment 8387169 [details] [diff] [review]
bug979378.patch

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

Looks good on Windows and Linux. On OS X the checkmark's vertical alignment is off and the "View Bookmarks Sidebar" label isn't aligned in the Bookmarks Panel.

Comment 13

[Stephen Horlander [:shorlander]]

Attachment: Checkmark Alignment

Forgot to mention that this breaks the checkmark on View Bookmarks Toolbar in the Bookmarks Toolbar submenu.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Attachment: The hopefully final version of the patch.

Nits fixed, and ui updated.

This doesn't fix the bookmarks toolbar submenu checkmark, but I'ld like to do that in a followup patch, if possible.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8387599 [details] [diff] [review]
The hopefully final version of the patch.

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

Ship it!

(but maybe ditch the duplicate selectors below)

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +81,5 @@
> +}
> +
> +.restoreallitem > .toolbarbutton-icon,
> +.restoreallitem > .toolbarbutton-icon,
> +.restoreallitem > .toolbarbutton-icon {

Umm. More coffee required. :D

Comment 17

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/834c3308130a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/834c3308130a

Comment 19

[Tim Nguyen :ntim]

The [fixed-on-fx-team] flag hasn't been removed.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Thanks ntim!

Comment 21

[:Gijs (he/him)]

Comment on attachment 8387599 [details] [diff] [review]
The hopefully final version of the patch.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis bookmarks panel
User impact if declined: checkmarks in australis bookmarks panel don't look right
Testing completed (on m-c, etc.): on m-c for a while
Risk to taking this patch (and alternatives if risky): like bug 972405, medium, but at this point moving forward is less risky than trying to keep all these fixes off Aurora
String or IDL/UUID changes made by this patch: none

Comment 22

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ef90350476ac