Closed Bug 1305195 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Theme, defect, P3)

51 Branch
All
Windows
defect

Tracking

()

VERIFIED FIXED
Firefox 52
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 + verified
firefox52 + verified

People

(Reporter: abhinav.kumar.in, Assigned: jaws)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached image 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.
Component: Untriaged → Location Bar
OS: Unspecified → Windows
Hardware: Unspecified → All
Whiteboard: In private browsing mode, zoom level indicator is unreadable
Summary: Zoom indicator private → In private browsing mode, zoom level indicator is unreadable
Whiteboard: In private browsing mode, zoom level indicator is unreadable
Where did you place the zoom buttons?
(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.
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
Blocks: 565718
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: In private browsing mode, zoom level indicator is unreadable → In private browsing mode, zoom level indicator is unreadable when dark developer edition theme is in use
Component: Location Bar → Theme
[Tracking Requested - why for this release]:
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
Flags: qe-verify+
Priority: -- → P3
Also menubar and identity icon are affected in private browsing mode with dark developer edition theme.
Jared: do you have cycles to pick this up?
Flags: needinfo?(jaws)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
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/ ?
Attachment #8798179 - Flags: review?(gijskruitbosch+bugs)
Tracking 52+ as it is important to be able read the zoom indicator.
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.
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 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 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?
Attachment #8798179 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by jwein@mozilla.com:
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
Depends on: 1309260
https://hg.mozilla.org/mozilla-central/rev/56bfb8ef2073
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Track 51+ as regression.

Hi :jaws,
Since this bug is a regression and also affects 51, do you consider to uplift this for 51?
Flags: needinfo?(jaws)
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
Flags: needinfo?(jaws)
Attachment #8798179 - Flags: approval-mozilla-aurora?
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.
Attachment #8798179 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.