Closed
Bug 979378
Opened 11 years ago
Closed 11 years ago
[Australis] Update Panel Checkbox
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 30
People
(Reporter: shorlander, Assigned: bwinton)
References
Details
(Whiteboard: [Australis:P3+])
Attachments
(4 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Gijs
:
review+
Gijs
:
ui-review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Update the menu panel checkmark to a consistent item that fits with the panel style.
Reporter | ||
Updated•11 years ago
|
OS: Windows 8.1 → All
Reporter | ||
Comment 1•11 years ago
|
||
Reporter | ||
Updated•11 years ago
|
Summary: [Australis] Update Panel Checkbox for Windows → [Australis] Update Panel Checkbox
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → bwinton
Assignee | ||
Comment 2•11 years ago
|
||
(WIP up at http://people.mozilla.org/~bwinton/temp/bug979378.patch, building on Windows now.
I'll check the results tomorrow morning.)
Comment 5•11 years ago
|
||
Sounds like bug 978566 and bug 938603 should be fixed by this, please check and reopen (or just fix here :) if not.
Assignee | ||
Comment 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
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.
Assignee | ||
Comment 9•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
Wow, that took a while.
Attachment #8387169 -
Flags: ui-review?(shorlander)
Attachment #8387169 -
Flags: review?(gijskruitbosch+bugs)
Comment 11•11 years ago
|
||
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+
Reporter | ||
Comment 12•11 years ago
|
||
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-
Reporter | ||
Comment 13•11 years ago
|
||
Forgot to mention that this breaks the checkmark on View Bookmarks Toolbar in the Bookmarks Toolbar submenu.
Assignee | ||
Comment 15•11 years ago
|
||
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 16•11 years ago
|
||
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+
Comment 17•11 years ago
|
||
Status: NEW → ASSIGNED
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-on-fx-team]
Comment 18•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Comment 19•11 years ago
|
||
The [fixed-on-fx-team] flag hasn't been removed.
Comment 20•11 years ago
|
||
Thanks ntim!
Whiteboard: [Australis:P3+][fixed-on-fx-team] → [Australis:P3+]
Comment 21•11 years ago
|
||
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?
Updated•11 years ago
|
status-firefox29:
--- → affected
status-firefox30:
--- → fixed
Updated•11 years ago
|
Attachment #8387599 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 22•11 years ago
|
||
Updated•11 years ago
|
QA Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•