Closed Bug 940133 Opened 6 years ago Closed 6 years ago

Australis: Invert icons in the tabstrip and menubar on Windows Classic

Categories

(Firefox :: Theme, defect)

All
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: alice0775, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(2 files, 6 obsolete files)

Attached image screenshot
Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/f2adb62d07eb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118094134

Steps To Reproduce
0. Make sure setting Windows7 Classic visual style from control panel
1. Start Australis build
2. Open many tabs 
   or
   Customize and put icon in Tabbar

Actual Results:
Toolbuttons are almost invisible
Summary: Australis: Toolbuttons in the Tabbar are almost invisible on Windows7 Classic → Australis: Need inverted icons for tab overflow and new tab buttons on Windows7 Classic
Whiteboard: [Australis:P4][Australis:M?]
Hardware: x86_64 → All
Summary: Australis: Need inverted icons for tab overflow and new tab buttons on Windows7 Classic → Australis: Invert icons in the tabstrip on Windows Classic
Whiteboard: [Australis:P4][Australis:M?] → [Australis:P4]
Component: Toolbars and Customization → Theme
Bumping to P3 to match the bug that got duped to this one.
Whiteboard: [Australis:P4] → [Australis:P3]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attached patch WIP Patch v2 (obsolete) — Splinter Review
Attachment #8385353 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8385365 - Attachment is obsolete: true
Comment on attachment 8385393 [details] [diff] [review]
Patch v2

I didn't add a comment next to the windows-classic overrides yet. What do you think about this:

"Show icons in the TabsToolbar and #menubar as inverted when in Classic mode for improved contrast."
Attachment #8385393 - Attachment description: WIP Patch v3 → Patch v2
Attachment #8385393 - Flags: review?(MattN+bmo)
Comment on attachment 8385393 [details] [diff] [review]
Patch v2

This doesn't seem to take into account that drawing in the title bar may be disabled.
Attachment #8385393 - Flags: review-
Attached patch WIP Patch v4 (obsolete) — Splinter Review
Attachment #8385393 - Attachment is obsolete: true
Attachment #8385393 - Flags: review?(MattN+bmo)
Attached patch Patch v5 (obsolete) — Splinter Review
Tested on my WinXP vm.
Attachment #8385455 - Attachment is obsolete: true
Attachment #8385618 - Flags: review?(MattN+bmo)
Comment on attachment 8385618 [details] [diff] [review]
Patch v5

Review of attachment 8385618 [details] [diff] [review]:
-----------------------------------------------------------------

You forgot to not do this with LWT for some rules.

::: browser/themes/windows/browser.css
@@ +14,5 @@
>  %define navbarTextboxCustomBorder border-color: rgba(0,0,0,.32);
>  %define forwardTransitionLength 150ms
>  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
>  %define conditionalForwardWithUrlbarWidth 27
> +%define nestedButtons #zoom-out-button, #zoom-in-button, #cut-button, #copy-button, #paste-button

Nit: I would rather this move to browser/themes/shared/browser.inc so we don't need to move it again if OS X and Linux need it.

@@ +1651,5 @@
>  }
>  
> +@media (-moz-windows-classic) {
> +  #main-window[tabsintitlebar]:not([inFullscreen]) .tabbrowser-arrowscrollbox > .scrollbutton-up,
> +  #main-window[tabsintitlebar]:not([inFullscreen]) .tabbrowser-arrowscrollbox > .scrollbutton-down {

This could be cleaner by combining it with the ruleset above and using :-moz-system-metric(windows-classic)

@@ +1703,5 @@
>  
> +@media (-moz-windows-classic) {
> +  #main-window[tabsintitlebar]:not([inFullscreen]) .tabs-newtab-button,
> +  #main-window[tabsintitlebar]:not([inFullscreen]) #TabsToolbar > #new-tab-button,
> +  #main-window[tabsintitlebar]:not([inFullscreen]) #TabsToolbar > toolbarpaletteitem > #new-tab-button {

ditto

@@ +1721,5 @@
>    list-style-image: url("chrome://browser/skin/toolbarbutton-dropdown-arrow-inverted.png");
>  }
>  
> +@media (-moz-windows-classic) {
> +  #main-window[tabsintitlebar]:not([inFullscreen]) #alltabs-button {

ditto
Attachment #8385618 - Flags: review?(MattN+bmo) → review-
Attached patch Patch v6Splinter Review
Attachment #8385618 - Attachment is obsolete: true
Attachment #8385641 - Flags: review?(MattN+bmo)
Comment on attachment 8385641 [details] [diff] [review]
Patch v6

Review of attachment 8385641 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Nit: Including the menubar in your commit message
Attachment #8385641 - Flags: review?(MattN+bmo) → review+
Summary: Australis: Invert icons in the tabstrip on Windows Classic → Australis: Invert icons in the tabstrip and menubar on Windows Classic
Duplicate of this bug: 887992
Depends on: 979747
https://hg.mozilla.org/mozilla-central/rev/532818bf6b7a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Comment on attachment 8385641 [details] [diff] [review]
Patch v6

[Approval Request Comment]
Bug caused by (feature/regressing bug #): issue since beginning of australis
User impact if declined: poor contrast with icons customized to menubar or tabs toolbar
Testing completed (on m-c, etc.): locally and on m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

There is a follow-up bug to also invert the text for the Zoom controls when placed in the menubar or tabs bar, but I don't think that bug should block this getting merged to Aurora.
Attachment #8385641 - Flags: approval-mozilla-aurora?
Attachment #8385641 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Verified as fixed on the latest Firefox 29 beta and Firefox 30 Aurora on Windows XP 32bit.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.