1403867 - Private Browsing/Accessibility indicator makes the menu toolbar too large

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Johann Hofmann [:johannh]]

Attachment: Windows 7 - Private Browsing

[Tracking Requested - why for this release]:
Visible regression that potentially worsens the Photon experience for a non-trivial amount of users.

I first saw this on https://www.reddit.com/r/firefox/comments/72xvh6/why_does_firefox_now_look_like_internet_explorer/

This user has the menu bar enabled and an accessibility indicator turned on. The accessibility indicator takes up too much vertical space, which makes the tabs toolbar look way too large. This obviously also happens with the private browsing indicator.

Windows 10 also seems affected, but the added space is much smaller and I think we could live with that.

I attached some screenshots of my own.

It's hard to say, really, but I think this could affect a non-trivial amount of users. Windows 7 is 46% of our user base and anecdotally I believe that a lot of them have the menu bar enabled. Would be nice to know.

Yura, can you take a look at this please?

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Windows 7 - No indicator

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Windows 10

Comment 3

[Amy Lee [:amylee] UX]

Attachment: My recommendation is to move the icon to the tab area. This is where the icon moves to when the user selects to show the title bar in the customization panel. See attached for reference.

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 57 since it's a new regression.

Comment 5

[Jim Mathies [:jimm]]

Attachment: win7.jpg

Be careful about tab bar overflow, the '+' button usually sits right where that icon is moving to.

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1403867 patch v1

I added a comment in the CSS for visibility rules to make it clearer what is visible in what conditions.

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9ef97ae4925424b4a585213e882cd0c7572aafd for folks to try

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8914878 [details] [diff] [review]
1403867 patch v1

>+  if (toolbar.id == "toolbar-menubar" && AppConstants.platform == "win") {
>+    document.documentElement.setAttribute("menubarautohide", !isVisible);

I don't understand what this is good for. It seems to me that at this point, the indicator should just always be part of the tab strip.

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8914878 [details] [diff] [review]
1403867 patch v1

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

I would agree with dao. At this point, can't we just always show the indicators in the tab strip? It looks like that would simplify our code a lot.

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

Makes sense will try this approach tomorrow (PTO today)

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1403867 v2

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8915643 [details] [diff] [review]
1403867 v2

>+:root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar :-moz-any(.private-browsing-indicator, .accessibility-indicator),
>+:root[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive] ~ #TabsToolbar :-moz-any(.private-browsing-indicator, .accessibility-indicator) {
>+  margin-top: calc(-1 * var(--space-above-tabbar));
>+}

What's the point of this? Shouldn't the indicators be aligned with the tabs regardless of --space-above-tabbar? Why would you move them up into what supposed to be extra window drag space?

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

I'd say they should align with window buttons. ni'ing Amy for this.

Comment 14

[Amy Lee [:amylee] UX]

(In reply to Yura Zenevich [:yzen] from comment #13)
> I'd say they should align with window buttons. ni'ing Amy for this.

Hi, 

Agreed, they should be vertically center aligned with the buttons.

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Yura Zenevich [:yzen] from comment #13)
> I'd say they should align with window buttons. ni'ing Amy for this.

Just to expand a little bit: the indicators are grouped with window buttons (separated by the drag space from tabs) visually so that's why I did that.

Comment 16

[Johann Hofmann [:johannh]]

Comment on attachment 8915643 [details] [diff] [review]
1403867 v2

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

Seems good to me, thank you.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6c355f7633a
fixing positioning of indicators when menubar persists. r=johannh

Comment 18

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8915643 [details] [diff] [review]
1403867 v2

This fixes incorrect titlebar sizing when menu bar persists. It also simplifies dramatically the logic around indicator positioning between titlebar and tabbbar (see screenshots).
[Feature/Bug causing the regression]: bug 1383051
[User impact if declined]: Windows users with menubars enabled will see too much vertical drag space, especially on Win7.
[Is this code covered by automated tests?]: No markup and css changes only.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: Maybe, making sure that extra space visible in screenshots is not there any more.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not really, CSS/markup change that actually simplifies things.
[Why is the change risky/not risky?]: n/a
[String changes made/needed]: none

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8915643 [details] [diff] [review]
1403867 v2

Cancelling approval request, as there are a couple of problems with this patch...

>+:root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar :-moz-any(.private-browsing-indicator, .accessibility-indicator),
>+:root[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive] ~ #TabsToolbar :-moz-any(.private-browsing-indicator, .accessibility-indicator) {

Why is there "#toolbar-menubar ~ " in the first selector?

Also, if I remember correctly, for perf reasons :-moz-any shouldn't be used without another qualifying id/class/element selector at the end of a selector chain.

>+:root .private-browsing-indicator {

What is :root doing here?

Comment 20

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Dão Gottwald [::dao] from comment #19)
> Comment on attachment 8915643 [details] [diff] [review]
> 1403867 v2
> 
> Cancelling approval request, as there are a couple of problems with this
> patch...
> 
> >+:root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar :-moz-any(.private-browsing-indicator, .accessibility-indicator),
> >+:root[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive] ~ #TabsToolbar :-moz-any(.private-browsing-indicator, .accessibility-indicator) {
> 
> Why is there "#toolbar-menubar ~ " in the first selector?

To compensate for this styling: https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#80-91

> 
> Also, if I remember correctly, for perf reasons :-moz-any shouldn't be used
> without another qualifying id/class/element selector at the end of a
> selector chain.

I can split these in individual ones.

> 
> >+:root .private-browsing-indicator {
> 
> What is :root doing here?

Good catch.

Comment 21

[Dão Gottwald [:dao]]

(In reply to Yura Zenevich [:yzen] from comment #20)
> > >+:root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar :-moz-any(.private-browsing-indicator, .accessibility-indicator),
> > >+:root[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive] ~ #TabsToolbar :-moz-any(.private-browsing-indicator, .accessibility-indicator) {
> > 
> > Why is there "#toolbar-menubar ~ " in the first selector?
> 
> To compensate for this styling:
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/
> browser.css#80-91

Okay, leave this as is then and we can clean it up in a followup.

Comment 22

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1403867 follow up

follow up with selector clean up

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 8916019 [details] [diff] [review]
1403867 follow up

>-:root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar :-moz-any(.private-browsing-indicator, .accessibility-indicator),
>+:root[sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar > .private-browsing-indicator,

Yep, this should also use the child selector. Well spotted.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0b45659c519
clean up indicator selector rules. r=dao

Comment 25

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8915643 [details] [diff] [review]
1403867 v2

This fixes incorrect titlebar sizing when menu bar persists. It also simplifies dramatically the logic around indicator positioning between titlebar and tabbbar (see screenshots).
[Feature/Bug causing the regression]: bug 1383051
[User impact if declined]: Windows users with menubars enabled will see too much vertical drag space, especially on Win7.
[Is this code covered by automated tests?]: No markup and css changes only.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: Maybe, making sure that extra space visible in screenshots is not there any more.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not really, CSS/markup change that actually simplifies things.
[Why is the change risky/not risky?]: n/a
[String changes made/needed]: none

Comment 26

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8916019 [details] [diff] [review]
1403867 follow up

This is a follow up of the main bug. See comment 25

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c6c355f7633a
https://hg.mozilla.org/mozilla-central/rev/e0b45659c519

Comment 28

[Valentina Claudia Ona]

I verified this issue using Nightly 58.0a1 and Firefox Beta 57.0b5 with Build ID 20171008220130 on Windows 10 x64, Windows 7 x32, Ubuntu 17.04
I will mark this as verified fixed.

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8915643 [details] [diff] [review]
1403867 v2

Photon related (I guess), Beta57+

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/120189e6b8b1
https://hg.mozilla.org/releases/mozilla-beta/rev/a29bc48fe417

Comment 31

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 58.0a1 (2017.09.28) on Win 7 x86.
I can confirm this issue is fixed, I verified using Firefox 57.0b13 on Win 7 x86, Win 10 x64 and Win 8.1 x64.