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

()

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: alice0775, 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

5 years ago
Posted image screenshot
Steps to Reproduce:
1. Move Zoom widget to Tabstrip
(Reporter)

Comment 1

5 years ago
Steps to Reproduce:
1. Move Bookmarks Toolbar Items widget to Menubar

Comment 2

5 years ago
It's a bit similar on Aero.

Comment 3

5 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

5 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.
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

5 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.
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: 5 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+
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.