1238531 - [MAC] “Sign in to Sync” text is written with black using Dev Edition theme

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P2)

Description

[Vasilica Tamas, QA [:vtamas]]

Reproducible on: Firefox 45.0a2 under Mac OS X 10.10.5

STR
1.Launch Firefox with a clean profile.
2.Click menu [≡] → select "Customize".
3.Move the "Synced tabs" icon to toolbar or tab bar.
4.Exit “Customize” mode.
5.Click on “Synced tabs” button.

ER
“Sign in to Sync” is written with white. See screenshot: http://i.imgur.com/PJvK4vz.jpg

AR
“Sign in to Sync” (or “Sync Preferences” when tabs option is disabled) is written with black. 
See screenshots:  http://i.imgur.com/ly2pfjE.png 
                  http://i.imgur.com/hemnywL.png


Additional notes:
- Reproducible on: Firefox 45.0a2 (2016-01-11) under Mac OS X 10.10.5.
- It is also reproducible on Firefox 45.0a2 from 2015-12-16
- Firefox 46.0a1 is not affected.
- This is not reproducible on Windows and Ubuntu.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

I can't repro on a non-retina macbook air using 45.0a2 (2016-01-12), so a couple of clarifications:

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #0)
> STR
> 1.Launch Firefox with a clean profile.
> 2.Click menu [≡] → select "Customize".
> 3.Move the "Synced tabs" icon to toolbar or tab bar.
> 4.Exit “Customize” mode.

To clarify, the "synced tabs" button should appear in the panel for a new profile, so these steps aren't necessary, right?

> 5.Click on “Synced tabs” button.

I see white text if I skip directly to (5) on a brand-new profile. I did try customizing the button away and back again and still got white.

> - Reproducible on: Firefox 45.0a2 (2016-01-11) under Mac OS X 10.10.5.
> - Firefox 46.0a1 is not affected.

I suspect/guess it is the devedition dark theme given the screenshots show that theme and it doesn't reproduce on nightly - can you please clarify the repo on 45, then try and repro on 46 after setting the default theme on that profile to "dev edition" (or whatever else you can think of to make nightly act live dev edition).

Thanks!

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #1)
> I suspect/guess it is the devedition dark theme given the screenshots show

To be clear, my testing using 45 on a brand-new profile also started up in the devedition dark theme, but still I failed to reproduce it. So I suspect it is *something* to do with that theme, but there's more to it than just that.

Comment 3

[Vasilica Tamas, QA [:vtamas]]

(In reply to Mark Hammond [:markh] from comment #1)

> To clarify, the "synced tabs" button should appear in the panel for a new
> profile, so these steps aren't necessary, right?

By “new profile” I actually mean a clean profile, with no add-ons or featured/complete themes installed that could cause a related bug and this is the reason why the first step is to open a new profile (which doesn’t have any add-ons or themes installed).
 
> I see white text if I skip directly to (5) on a brand-new profile. I did try
> customizing the button away and back again and still got white.

Starting a new profile, the "Synced tabs" button appears by default in the Panel Menu, but this issue is not reproducible for "Synced tabs" displayed in Panel Menu. 
You will be able to reproduce this bug only dragging the “Synced tabs” button from Panel Menu to toolbar or tabs bar.

> I suspect/guess it is the devedition dark theme given the screenshots show
> that theme and it doesn't reproduce on nightly - can you please clarify the
> repo on 45, then try and repro on 46 after setting the default theme on that
> profile to "dev edition" (or whatever else you can think of to make nightly
> act live dev edition).

- This issue also reproduces on Firefox 46.0a1 (2016-01-12) with Dev Edition Light theme enabled.
See screenshot: http://i.imgur.com/wM5Lmw6.png
- It is *not* reproducible on Firefox 45.0a2 (2016-01-12) with Default theme enabled.
See screenshot: http://i.imgur.com/h102tu7.png

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #3)
> By “new profile” I actually mean a clean profile, with no add-ons or
> featured/complete themes installed that could cause a related bug and this
> is the reason why the first step is to open a new profile (which doesn’t
> have any add-ons or themes installed).
...
> You will be able to reproduce this bug only dragging the “Synced tabs”
> button from Panel Menu to toolbar or tabs bar.

I tried that first and still couldn't repro, using a similarly configured test profile I regularly use - I then tried a brand-new profile and still failed. It would be awesome if you could come up with STR for a brand-new profile? It might require creating a profile using (say) 45, then doing nothing but upgrading then customizing (or maybe something quite different)

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #4)
> It might require creating a profile using (say) 45, 

oops - I meant 44 - ie, prior to the synced-tabs menu landing.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Chris observed that this can only be reproduced when the button is in the toolbar, not when it is in the panel - I only tried the panel. I'll verify that.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Dao, I'm hoping you can shed some light here. The issue is that using the DevEdition theme, a toolbarbutton in a panel UI, with no explicit color rule, is showing up with white text when the button is anywhere other than the toolbar, and black text when it is in the toolbar.

The reason is the rule at https://dxr.mozilla.org/mozilla-central/rev/6764bc656c1d146962d53710d734c2ac87c2306f/browser/themes/osx/browser.css#73:

> #navigator-toolbox toolbarbutton:-moz-lwtheme {
>  color: inherit;
>  text-shadow: inherit;
> }

So only when the panel button is in the toolbar, and only on OSX, the color is inherited from the window, which has specified black.

I know it's a long time ago, but best I can tell, this was added in bug 511107.

I'm guessing the rule is supposed to apply for buttons directly in the toolbox rather than for buttons in a panel opened from the button (ie, that the rule should be |#navigator-toolbox > toolbarbutton:-moz-lwtheme|)? Or maybe just specifying white in the style for the button is the appropriate fix? Or something else I'm missing?

FTR, the button in question is https://dxr.mozilla.org/mozilla-central/rev/6764bc656c1d146962d53710d734c2ac87c2306f/browser/components/customizableui/content/panelUI.inc.xul#136

Comment 8

[Dão Gottwald [:dao]]

(In reply to Mark Hammond [:markh] from comment #7)
> I'm guessing the rule is supposed to apply for buttons directly in the
> toolbox rather than for buttons in a panel opened from the button (ie, that
> the rule should be |#navigator-toolbox > toolbarbutton:-moz-lwtheme|)?

No, toolbarbuttons are never direct children of the toolbox :/

> Or
> maybe just specifying white in the style for the button is the appropriate
> fix? Or something else I'm missing?

Where does this button currently get its white text color from (at least when it does have white text)?

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(In reply to Dão Gottwald [:dao] from comment #8)
> Where does this button currently get its white text color from (at least
> when it does have white text)?

Doh - good question :) In the style dedicated to that button :/ https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#687

Without that, I'm at https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/global.css#26, at which point I'm heading to bed sobbing ;)

Thanks!

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-1238531-ensure-buttons-in-SyncedTabs-panel-alway.patch

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8710856 [details] [diff] [review]
0003-Bug-1238531-ensure-buttons-in-SyncedTabs-panel-alway.patch

Moving review request to Gijs now that Dao is out of action for some period.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8710856 [details] [diff] [review]
0003-Bug-1238531-ensure-buttons-in-SyncedTabs-panel-alway.patch

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

r=me with the nit for the comment addressed

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +686,5 @@
>  .PanelUI-remotetabs-prefs-button {
>    -moz-appearance: none;
>    background-color: #0096dd;
> +  /* !important for the color as an OSX specific rule when the DevEdition theme
> +     is used for buttons in the toolbox overrides. See bug 1238531 for details */

nit: s/when the DevEdition theme/when a lightweight theme/.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/95fdfaf5e766

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95fdfaf5e766

Comment 15

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 46.0a1 (2016-01-11) using Mac OS X 10.11.
Verified fixed on Firefox 47.0a1 (2016-02-28) under Mac OS X 10.11.