Last Comment Bug 979747 - Australis: should use inverted text for zoom widget and bookmark items in the tabstrip and menubar on Windows Classic
: Australis: should use inverted text for zoom widget and bookmark items in the...
Status: VERIFIED FIXED
[Australis:P3-]
: ux-consistency
Product: Firefox
Classification: Client Software
Component: Toolbars and Customization (show other bugs)
: 29 Branch
: All Windows 7
-- normal (vote)
: Firefox 31
Assigned To: Matthew N. [:MattN] (PM if requests are blocking you)
:
: :Gijs
Mentors:
Depends on:
Blocks: australis-merge 940133
  Show dependency treegraph
 
Reported: 2014-03-04 23:01 PST by Alice0775 White
Modified: 2014-04-01 06:52 PDT (History)
5 users (show)
MattN+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
verified
verified
verified


Attachments
screenshot (9.34 KB, image/png)
2014-03-04 23:01 PST, Alice0775 White
no flags Details
screenshot (Bookmarks) (29.77 KB, image/png)
2014-03-04 23:06 PST, Alice0775 White
no flags Details
v.1 Inherit text color on toolbars above the titlebar on Windows Classic (3.75 KB, patch)
2014-03-19 22:59 PDT, Matthew N. [:MattN] (PM if requests are blocking you)
gijskruitbosch+bugs: feedback+
Details | Diff | Splinter Review
v.2 Revert CSS move (4.30 KB, patch)
2014-03-27 02:11 PDT, Matthew N. [:MattN] (PM if requests are blocking you)
mdeboer: review+
sledru: approval‑mozilla‑aurora+
sledru: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Alice0775 White 2014-03-04 23:01:47 PST
Created attachment 8385908 [details]
screenshot

Steps to Reproduce:
1. Move Zoom widget to Tabstrip
Comment 1 User image Alice0775 White 2014-03-04 23:06:59 PST
Created attachment 8385914 [details]
screenshot (Bookmarks)

Steps to Reproduce:
1. Move Bookmarks Toolbar Items widget to Menubar
Comment 2 User image Guillaume C. [:ge3k0s] 2014-03-05 01:28:22 PST
It's a bit similar on Aero.
Comment 3 User image :Gijs 2014-03-05 03:49:29 PST
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 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-03-17 18:42:14 PDT
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 User image Dão Gottwald [:dao] 2014-03-18 02:52:25 PDT
(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 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-03-18 23:06:21 PDT
(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 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-03-19 22:59:47 PDT
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.
Comment 8 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-03-19 23:02:39 PDT
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 User image Mike de Boer [:mikedeboer] 2014-03-20 04:20:29 PDT
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 User image :Gijs 2014-03-20 04:47:09 PDT
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 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-03-20 11:48:29 PDT
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 User image Mike de Boer [:mikedeboer] 2014-03-25 03:15:09 PDT
Thanks for explaining that Matt! I'll review the patch as well.
Comment 13 User image Mike de Boer [:mikedeboer] 2014-03-26 03:22:33 PDT
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 User image Mike de Boer [:mikedeboer] 2014-03-26 03:27:44 PDT
I just remembered why I can see the sprite on OSX... toolbarbuttons.inc.css isn't included on OSX HiDPI.
Comment 15 User image Mike de Boer [:mikedeboer] 2014-03-26 08:43:11 PDT
Other than that, this patch looks good.
Comment 16 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-03-27 02:11:11 PDT
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.
Comment 17 User image Mike de Boer [:mikedeboer] 2014-03-27 03:00:19 PDT
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 User image Mike de Boer [:mikedeboer] 2014-03-27 03:01:36 PDT
(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 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-03-27 23:30:54 PDT
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 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-03-27 23:37:09 PDT
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 User image Wes Kocher (:KWierso) 2014-03-28 16:52:15 PDT
https://hg.mozilla.org/mozilla-central/rev/87c555678a52
Comment 22 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-03-29 13:59:46 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/755136215267
Comment 23 User image Mike de Boer [:mikedeboer] 2014-03-31 10:03:16 PDT
Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/a97accabb098
Comment 24 User image Andrei Vaida, QA [:avaida] – please ni? me 2014-04-01 06:52:59 PDT
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

Note You need to log in before you can comment on or make changes to this bug.