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

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Alice0775 White]

Attachment: screenshot

Steps to Reproduce:
1. Move Zoom widget to Tabstrip

Comment 1

[Alice0775 White]

Attachment: screenshot (Bookmarks)

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

Comment 2

[u428464]

It's a bit similar on Aero.

Comment 3

[:Gijs (he/him)]

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.

Comment 4

[Matthew N. [:MattN]]

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

Comment 5

[Dão Gottwald [:dao]]

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

Comment 6

[Matthew N. [:MattN]]

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

Comment 7

[Matthew N. [:MattN]]

Attachment: 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.

Comment 8

[Matthew N. [:MattN]]

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

[Mike de Boer [:mikedeboer]]

Matt, this patch changes moves things around rather substantially, which I think is all good, but which platforms did you check this patch on?

Comment 10

[:Gijs (he/him)]

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?

Comment 11

[Matthew N. [:MattN]]

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.

Comment 12

[Mike de Boer [:mikedeboer]]

Thanks for explaining that Matt! I'll review the patch as well.

Comment 13

[Mike de Boer [:mikedeboer]]

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.

Comment 14

[Mike de Boer [:mikedeboer]]

I just remembered why I can see the sprite on OSX... toolbarbuttons.inc.css isn't included on OSX HiDPI.

Comment 15

[Mike de Boer [:mikedeboer]]

Other than that, this patch looks good.

Comment 16

[Matthew N. [:MattN]]

Attachment: 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.

Comment 17

[Mike de Boer [:mikedeboer]]

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

Comment 18

[Mike de Boer [:mikedeboer]]

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

Comment 19

[Matthew N. [:MattN]]

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.

Comment 20

[Matthew N. [:MattN]]

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

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/87c555678a52

Comment 22

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/755136215267

Comment 23

[Mike de Boer [:mikedeboer]]

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/a97accabb098

Comment 24

[Andrei Vaida [:avaida]]

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