1305195 - In private browsing mode, zoom level indicator is unreadable when dark developer edition theme is in use

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P3)

Description

[Abhinav Kumar]

Attachment: Screenshot (45).png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160922113459

Steps to reproduce:

Open a webpage in private browsing mode and changed zoom level.


Actual results:

The zoom level appears but cannot be read as both background color of theme and text color is same.


Expected results:

Text color should be white in private browsing mode.

Comment 1

[Loic]

Where did you place the zoom buttons?

Comment 2

[Abhinav Kumar]

(In reply to Loic from comment #1)
> Where did you place the zoom buttons?

In Firefox 51.0a2 (Dev edition), custom zoom levels are put in location bar. Beside the drop down button and reading list button which is not readable in Private mode.

Comment 3

[arni2033]

Should be in "Theme" component.

This was implemented in broken state in bug 565718. Pushlog:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=8725f14625e0c87776f9acf66e6712ceca301000&tochange=ef0801fdc7abe018a8b026c00e8bdebcc690001c

Comment 4

[Loic]

[Tracking Requested - why for this release]:

Comment 5

[Andrei Vaida [:avaida]]

Windows 10 x64 
  - 51.0a2 (2016-09-27) is affected

Mac OS X 10.11.6 
  - 51.0a2 (2016-09-27) is unaffected, but the styling of the indicator is different 
    compared to other platforms
  - there's no border around the actual zoom percentage

Ubuntu 16.04 x86 
  - 51.0a2 (2016-09-27) is partially affected
  - the indicator has a white border, but the actual text showing the percentage is
    grey and not that visible

Comment 6

[magicp]

Also menubar and identity icon are affected in private browsing mode with dark developer edition theme.

Comment 7

[Justin Dolske [:Dolske]]

Jared: do you have cycles to pick this up?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1305195 - Inherit the color of the urlbar-zoom-reset button from the URL bar so the button text color matches that of the location bar text color.

Review commit: https://reviewboard.mozilla.org/r/83704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83704/

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

mozscreenshot push, https://treeherder.mozilla.org/#/jobs?repo=try&revision=c794df68aec8ef912630c86d71ee63747b0378b5

Comment 10

[:Gijs (he/him)]

Comment on attachment 8798179 [details]
Bug 1305195 - Inherit the color of the urlbar-zoom-reset button from the URL bar so the button text color matches that of the location bar text color.

https://reviewboard.mozilla.org/r/83704/#review82292

I think Matt or someone else more familiar with mozscreenshots should review those changes.

::: browser/themes/osx/browser.css:1681
(Diff revision 1)
>    margin: 0 3px;
>    font-size: .8em;
>    padding: 0 8px;
>    border-radius: 1em;
>    background-color: hsla(0,0%,0%,.05);
> -  border: 1px solid hsla(0,0%,0%,.1);
> +  border: 1px solid ThreeDLightShadow;

Why the OS X change? And why doesn't this need color: inherit? Can we move all these styles to shared/ ?

Comment 11

[Marcia Knous [:marcia]]

Tracking 52+ as it is important to be able read the zoom indicator.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8798179 [details]
Bug 1305195 - Inherit the color of the urlbar-zoom-reset button from the URL bar so the button text color matches that of the location bar text color.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83704/diff/1-2/

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

I removed the osx and linux changes as well as the mozscreenshots changes. The osx and linux regression here isn't as bad as the Windows one, and I don't have an OSX machine presently to test with.

If you can confirm that the linux and/or osx changes in the original patch work then I'm fine with adding them back in. I had meant to post a not on here after submitting the patch saying that I hadn't tested on osx or linux but I forgot.

The mozscreenshot changes have been removed because they would also need to enable devedition-dark theme as well as open a private browsing window.

Comment 14

[:Gijs (he/him)]

Egh, sorry for the delay - I hope to get to this on Monday if not before. The Windows change looks fine to me, but I was planning to also test the Linux change and check what to do on OS X, but didn't get around to it yesterday.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8798179 [details]
Bug 1305195 - Inherit the color of the urlbar-zoom-reset button from the URL bar so the button text color matches that of the location bar text color.

https://reviewboard.mozilla.org/r/83704/#review82292

> Why the OS X change? And why doesn't this need color: inherit? Can we move all these styles to shared/ ?

So for posterity: because otherwise there's no visible border on the dark developer theme.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8798179 [details]
Bug 1305195 - Inherit the color of the urlbar-zoom-reset button from the URL bar so the button text color matches that of the location bar text color.

https://reviewboard.mozilla.org/r/83704/#review83108

r=me on the Linux and OS X changes as well.

Could you still file a followup to move (most of) these styles somewhere shared as they seem to be almost identical on all platforms?

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8798179 [details]
Bug 1305195 - Inherit the color of the urlbar-zoom-reset button from the URL bar so the button text color matches that of the location bar text color.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83704/diff/2-3/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56bfb8ef2073
Inherit the color of the urlbar-zoom-reset button from the URL bar so the button text color matches that of the location bar text color. r=Gijs

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56bfb8ef2073

Comment 20

[Gerry Chang [:gchang]]

Track 51+ as regression.

Hi :jaws,
Since this bug is a regression and also affects 51, do you consider to uplift this for 51?

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8798179 [details]
Bug 1305195 - Inherit the color of the urlbar-zoom-reset button from the URL bar so the button text color matches that of the location bar text color.

Approval Request Comment
[Feature/regressing bug #]: bug 565718 shipping in v51
[User impact if declined]: regression in release of new feature
[Describe test coverage new/current, TreeHerder]: manual testing, css only changes
[Risks and why]: low risk, only CSS changes
[String/UUID change made/needed]: none

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8798179 [details]
Bug 1305195 - Inherit the color of the urlbar-zoom-reset button from the URL bar so the button text color matches that of the location bar text color.

Fix a UI related regression. Take it in 51 aurora.

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 24

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 52.0a1 (2016-09-23) (64-bit) on Windows 7 , 64 Bit !

This bug's fix is verified with latest Aurora & latest Nightly


Build ID   :  20161021004016
User Agent :  Mozilla/5.0(Windows NT 6.1; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0

Build ID   :    20161020030211
User Agent :    Mozilla/5.0 (WindowsNT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0

[bugday-20161019]