Australis: should use inverted text for zoom widget and bookmark items in the tabstrip and menubar on Windows Classic

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Toolbars and Customization
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Alice0775 White, Assigned: MattN)

Tracking

(Blocks: 1 bug, {ux-consistency})

29 Branch
Firefox 31
All
Windows 7
ux-consistency
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox28 unaffected, firefox29 verified, firefox30 verified, firefox31 verified)

Details

(Whiteboard: [Australis:P3-])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8385908 [details]
screenshot

Steps to Reproduce:
1. Move Zoom widget to Tabstrip
(Reporter)

Comment 1

3 years ago
Created attachment 8385914 [details]
screenshot (Bookmarks)

Steps to Reproduce:
1. Move Bookmarks Toolbar Items widget to Menubar
It's a bit similar on Aero.

Comment 3

3 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

3 years ago
status-firefox28: --- → unaffected
status-firefox29: --- → affected
status-firefox30: --- → affected
Version: 30 Branch → 29 Branch
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
(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.
(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.
Created attachment 8393975 [details] [diff] [review]
v.1 Inherit text color on toolbars above the titlebar on Windows Classic

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)
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
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

3 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+
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.
Flags: needinfo?(MattN+bmo)
Flags: needinfo?(mdeboer)
Thanks for explaining that Matt! I'll review the patch as well.
Flags: needinfo?(mdeboer)
Attachment #8393975 - Flags: review?(mdeboer)
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)
I just remembered why I can see the sprite on OSX... toolbarbuttons.inc.css isn't included on OSX HiDPI.
Other than that, this patch looks good.
Created attachment 8397706 [details] [diff] [review]
v.2 Revert CSS move

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 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+
(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.
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?
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
status-firefox31: affected → fixed
Attachment #8397706 - Flags: approval-mozilla-beta?
Attachment #8397706 - Flags: approval-mozilla-beta+
Attachment #8397706 - Flags: approval-mozilla-aurora?
Attachment #8397706 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/755136215267
status-firefox30: affected → fixed
Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/a97accabb098
status-firefox29: affected → fixed
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
status-firefox29: fixed → verified
status-firefox30: fixed → verified
status-firefox31: fixed → verified
You need to log in before you can comment on or make changes to this bug.