Closed
Bug 979747
Opened 10 years ago
Closed 10 years ago
Australis: should use inverted text for zoom widget and bookmark items in the tabstrip and menubar on Windows Classic
Categories
(Firefox :: Toolbars and Customization, defect)
Tracking
()
VERIFIED
FIXED
Firefox 31
Tracking | Status | |
---|---|---|
firefox28 | --- | unaffected |
firefox29 | --- | verified |
firefox30 | --- | verified |
firefox31 | --- | verified |
People
(Reporter: alice0775, Assigned: MattN)
References
(Blocks 1 open bug)
Details
(Keywords: ux-consistency, Whiteboard: [Australis:P3-])
Attachments
(3 files, 1 obsolete file)
9.34 KB,
image/png
|
Details | |
29.77 KB,
image/png
|
Details | |
4.30 KB,
patch
|
mikedeboer
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Steps to Reproduce: 1. Move Zoom widget to Tabstrip
Reporter | ||
Comment 1•10 years ago
|
||
Steps to Reproduce: 1. Move Bookmarks Toolbar Items widget to Menubar
Comment 3•10 years ago
|
||
I'm pretty sure there's another bug about this already, but that one wasn't specific to classic, and I don't have it to hand. We should just make sure we fix this for the other cases where we use inverted text, too.
Keywords: ux-consistency
Summary: Australis: Inverted %level of Zoom widget in the tabstrip and menubar on Windows Classic → Australis: should use inverted text for zoom widget and bookmark items in the tabstrip and menubar on Windows Classic
Whiteboard: [Australis:P3-]
Reporter | ||
Updated•10 years ago
|
status-firefox28:
--- → unaffected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
Version: 30 Branch → 29 Branch
Assignee | ||
Comment 4•10 years ago
|
||
So part of this is that @nestedButtons@ doesn't include #zoom-reset-button but adding it would actually break stuff since then it would get get the Toolbar-inverted list-style-image that it doesn't need: http://hg.mozilla.org/mozilla-central/rev/532818bf6b7a#l1.1 Perhaps we can do some combination of the following: A) rename the define to make it more clear it's just nestedButtons with an image B) add a comment above the define to note that #zoom-reset-button is special C) have two defines (one as a subset of the other): one for all 6 and the other just the image ones. D) Include all 6 in the define but always add ":not(#zoom-reset-button)" to selectors about the image. E) Include all 6 in the define but override the @nestedButtons@ rule with a #zoom-reset-button to remove the list-style-image and related properties. For the tabs toolbar we can probably just do color: inherit; since the color is already the caption text colour on the toolbar itself[1]. We should probably do the same for the menubar. I'm not sure if that works on XP currently since [1] is in "ifdef WINDOWS_AERO" but maybe there is a different rule that does similar. [1] https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css?mark=93-99#86
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Hardware: x86_64 → All
Comment 5•10 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #4) > I'm not sure if that works on XP > currently since [1] is in "ifdef WINDOWS_AERO" but maybe there is a > different rule that does similar. > > [1] > https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/ > browser.css?mark=93-99#86 Only the media query is in the ifdef.
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #5) > Only the media query is in the ifdef. Oops, I read this too quickly, you're right.
Assignee | ||
Comment 7•10 years ago
|
||
I included some cleanup like I discussed in comment 4 but it turned out that limiting this fix to standard widgets didn't made sense.
Attachment #8393975 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8393975 [details] [diff] [review] v.1 Inherit text color on toolbars above the titlebar on Windows Classic Review of attachment 8393975 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/browser.inc @@ +1,5 @@ > %filter substitution > > +% Note that zoom-reset-button is a bit different since it doesn't use an image and thus has the image with display: none. > +%define nestedButtons #zoom-out-button, #zoom-reset-button, #zoom-in-button, #cut-button, #copy-button, #paste-button > +%define primaryToolbarButtons #back-button, #forward-button, #home-button, #print-button, #downloads-button, #bookmarks-menu-button, #new-tab-button, #new-window-button, #fullscreen-button, #sync-button, #feed-button, #tabview-button, #webrtc-status-button, #social-share-button, #open-file-button, #find-button, #developer-button, #preferences-button, #privatebrowsing-button, #save-page-button, #switch-to-metro-button, #add-ons-button, #history-panelmenu, #nav-bar-overflow-button, #PanelUI-menu-button, #characterencoding-button, #email-link-button, #sidebar-button, @nestedButtons@ I'm adding #zoom-reset-button to nestedButtons since we already set display: none on its toolbarbutton-icon and that we should handle it consistently otherwise (e.g. hover styles) and don't forget about it. ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ -400,5 @@ > > -#zoom-in-button > .toolbarbutton-text, > -#zoom-out-button > .toolbarbutton-text, > -#zoom-reset-button > .toolbarbutton-icon { > - display: none; I moved this since it wasn't specific to the panelUI
Comment 9•10 years ago
|
||
Matt, this patch changes moves things around rather substantially, which I think is all good, but which platforms did you check this patch on?
Flags: needinfo?(MattN+bmo)
Comment 10•10 years ago
|
||
Comment on attachment 8393975 [details] [diff] [review] v.1 Inherit text color on toolbars above the titlebar on Windows Classic Review of attachment 8393975 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review because you detailed some previous directions and then went the other way, and now I don't know why these buttons need to be part of @primaryToolbarButtons@ ::: browser/themes/shared/browser.inc @@ +1,5 @@ > %filter substitution > > +% Note that zoom-reset-button is a bit different since it doesn't use an image and thus has the image with display: none. > +%define nestedButtons #zoom-out-button, #zoom-reset-button, #zoom-in-button, #cut-button, #copy-button, #paste-button > +%define primaryToolbarButtons #back-button, #forward-button, #home-button, #print-button, #downloads-button, #bookmarks-menu-button, #new-tab-button, #new-window-button, #fullscreen-button, #sync-button, #feed-button, #tabview-button, #webrtc-status-button, #social-share-button, #open-file-button, #find-button, #developer-button, #preferences-button, #privatebrowsing-button, #save-page-button, #switch-to-metro-button, #add-ons-button, #history-panelmenu, #nav-bar-overflow-button, #PanelUI-menu-button, #characterencoding-button, #email-link-button, #sidebar-button, @nestedButtons@ Hmm, so, the nestedButtons bit and moving things about seems fine... what I'm worried about is making the nested buttons part of the @primaryToolbarButtons. That has a lot of subtle effects. Why was it necessary?
Attachment #8393975 -
Flags: review?(gijskruitbosch+bugs) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8393975 [details] [diff] [review] v.1 Inherit text color on toolbars above the titlebar on Windows Classic Review of attachment 8393975 [details] [diff] [review]: ----------------------------------------------------------------- I changed direction since I found "#zoom-reset-button > .toolbarbutton-icon" has display: none and I realized that this problem would likely affect any toolbarbutton with text in the titlebar. I tested only on Windows 7 Classic and Windows 7 Glass since the primary fix is in a classic media query. I can run my screenshot diff tool on other platforms to see if there were unintentional change if you think it's worth it. ::: browser/themes/shared/browser.inc @@ +1,5 @@ > %filter substitution > > +% Note that zoom-reset-button is a bit different since it doesn't use an image and thus has the image with display: none. > +%define nestedButtons #zoom-out-button, #zoom-reset-button, #zoom-in-button, #cut-button, #copy-button, #paste-button > +%define primaryToolbarButtons #back-button, #forward-button, #home-button, #print-button, #downloads-button, #bookmarks-menu-button, #new-tab-button, #new-window-button, #fullscreen-button, #sync-button, #feed-button, #tabview-button, #webrtc-status-button, #social-share-button, #open-file-button, #find-button, #developer-button, #preferences-button, #privatebrowsing-button, #save-page-button, #switch-to-metro-button, #add-ons-button, #history-panelmenu, #nav-bar-overflow-button, #PanelUI-menu-button, #characterencoding-button, #email-link-button, #sidebar-button, @nestedButtons@ All of the nested buttons except zoom-reset-button were already part of primaryToolbarButtons, I just reduced duplication in how they were included. I explained in comment 8 why I'm adding #zoom-reset-button.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(MattN+bmo)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(mdeboer)
Comment 12•10 years ago
|
||
Thanks for explaining that Matt! I'll review the patch as well.
Flags: needinfo?(mdeboer)
Updated•10 years ago
|
Attachment #8393975 -
Flags: review?(mdeboer)
Comment 13•10 years ago
|
||
Comment on attachment 8393975 [details] [diff] [review] v.1 Inherit text color on toolbars above the titlebar on Windows Classic Review of attachment 8393975 [details] [diff] [review]: ----------------------------------------------------------------- I just checked this patch on OSX and regrettably this shows the Toolbar.png sprite in a miniature version above the percentage label (when in a toolbar) or next to the label (when in a panel). Please fix this for the next round. /me off checking Linux.
Attachment #8393975 -
Flags: review?(mdeboer)
Comment 14•10 years ago
|
||
I just remembered why I can see the sprite on OSX... toolbarbuttons.inc.css isn't included on OSX HiDPI.
Comment 15•10 years ago
|
||
Other than that, this patch looks good.
Assignee | ||
Comment 16•10 years ago
|
||
I'm not really sure why I need another review just to revert two hunks… did I miss something else? It's not obvious at all that toolbarbutton.inc.css is only included for @1x on OS X and we always included @1x rules for HiDPI in the past.
Attachment #8393975 -
Attachment is obsolete: true
Attachment #8397706 -
Flags: review?(mdeboer)
Comment 17•10 years ago
|
||
Comment on attachment 8397706 [details] [diff] [review] v.2 Revert CSS move Review of attachment 8397706 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for making you request another review round... I did find a nit though :P ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +1,5 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +/* Note that this file isn't used for HiDPI on OS X. */ I think you wanted to move this comment to toolbarbuttons.inc.css...
Attachment #8397706 -
Flags: review?(mdeboer) → review+
Comment 18•10 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #16) > It's not obvious at all that toolbarbutton.inc.css is only included for @1x > on OS X and we always included @1x rules for HiDPI in the past. I know! This has bit me/ us in the past, so we should really revert that decision and just include it regardless of DPI setting... unless it was done this way for perf reasons, of course.
Assignee | ||
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/87c555678a52 (In reply to Mike de Boer [:mikedeboer] from comment #18) > (In reply to Matthew N. [:MattN] from comment #16) > > It's not obvious at all that toolbarbutton.inc.css is only included for @1x > > on OS X and we always included @1x rules for HiDPI in the past. > > I know! This has bit me/ us in the past, so we should really revert that > decision and just include it regardless of DPI setting... unless it was done > this way for perf reasons, of course. I looked at the bug that added it and it seemed like mconley considered it cleaner. I can see some advantage to not having the overrides showing in the style inspector and DOMi. I think the core issue is that the display: none styles don't fit in with the reset and we need to rename/reorg some of those files. I'm making a list for after 29.
status-firefox31:
--- → affected
Flags: in-testsuite?
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8397706 [details] [diff] [review] v.2 Revert CSS move [Approval Request Comment] Bug caused by (feature/regressing bug #): Tabs In Titlebar on Classic User impact if declined: Text in the tab toolbar or menubar may be hard to read (black on dark blue). See attachment 8385908 [details] and attachment 8385914 [details]. Testing completed (on m-c, etc.): Testing completed locally by myself and mikedeboer on most platforms Risk to taking this patch (and alternatives if risky): Low risk adding one CSS rule to inherit color instead of using the default of black for buttons. String or IDL/UUID changes made by this patch: None
Attachment #8397706 -
Flags: approval-mozilla-beta?
Attachment #8397706 -
Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/87c555678a52
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8397706 -
Flags: approval-mozilla-beta?
Attachment #8397706 -
Flags: approval-mozilla-beta+
Attachment #8397706 -
Flags: approval-mozilla-aurora?
Attachment #8397706 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 22•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/755136215267
Comment 23•10 years ago
|
||
Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/a97accabb098
Comment 24•10 years ago
|
||
I was able to confirm the fix for this issue on Windows 7 64-bit [1], using: - the latest Beta (Build ID: 20140331125246), - the latest Aurora (Build ID: 20140401004001), - the latest Nightly (Build ID: 20140401030203), however, the bookmarks toolbar item labels (bookmarks, live bookmarks) still seem to be displayed with a lower opacity or slightly darker font. 1. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•