Closed Bug 979378 Opened 6 years ago Closed 6 years ago

[Australis] Update Panel Checkbox

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: shorlander, Assigned: bwinton)

References

Details

(Whiteboard: [Australis:P3+])

Attachments

(4 files, 1 obsolete file)

Update the menu panel checkmark to a consistent item that fits with the panel style.
Summary: [Australis] Update Panel Checkbox for Windows → [Australis] Update Panel Checkbox
Assignee: nobody → bwinton
(WIP up at http://people.mozilla.org/~bwinton/temp/bug979378.patch, building on Windows now.
I'll check the results tomorrow morning.)
Duplicate of this bug: 978566
Duplicate of this bug: 938603
Sounds like bug 978566 and bug 938603 should be fixed by this, please check and reopen (or just fix here :) if not.
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…  :)
Flags: needinfo?(shorlander)
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.
Flags: needinfo?(shorlander)
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.
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.
Attached patch bug979378.patch (obsolete) — Splinter Review
Wow, that took a while.
Attachment #8387169 - Flags: ui-review?(shorlander)
Attachment #8387169 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8387169 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 980534
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.
Attachment #8387169 - Flags: ui-review?(shorlander) → ui-review-
Attached image Checkmark Alignment
Forgot to mention that this breaks the checkmark on View Bookmarks Toolbar in the Bookmarks Toolbar submenu.
Duplicate of this bug: 980895
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.
Attachment #8387169 - Attachment is obsolete: true
Attachment #8387599 - Flags: ui-review?(shorlander)
Attachment #8387599 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8387599 - Flags: ui-review?(shorlander)
Attachment #8387599 - Flags: ui-review+
Attachment #8387599 - Flags: review?(gijskruitbosch+bugs)
Attachment #8387599 - Flags: review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/834c3308130a
Status: NEW → ASSIGNED
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-on-fx-team]
https://hg.mozilla.org/mozilla-central/rev/834c3308130a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
The [fixed-on-fx-team] flag hasn't been removed.
Thanks ntim!
Whiteboard: [Australis:P3+][fixed-on-fx-team] → [Australis:P3+]
Depends on: 981419
No longer blocks: 980534
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
Attachment #8387599 - Flags: approval-mozilla-aurora?
Attachment #8387599 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1000744
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.