Private Browsing/Accessibility indicator makes the menu toolbar too large

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: johannh, Assigned: yzen)

Tracking

({regression})

57 Branch
Firefox 58
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57+ verified, firefox58 verified)

Details

Attachments

(7 attachments, 1 obsolete attachment)

[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?
Flags: qe-verify+
Flags: needinfo?(yzenevich)
OS: Unspecified → Windows
Posted image Windows 10
Attachment #8913116 - Attachment description: Screen Shot 2017-09-28 at 10.57.42.png → Windows 7 - Private Browsing
Attachment #8913117 - Attachment description: Screen Shot 2017-09-28 at 10.58.11.png → Windows 7 - No indicator
Summary: Private Browsing/Accessibility indicator makes the tabs toolbar too large when the menu bar is enabled → Private Browsing/Accessibility indicator makes the menu toolbar too large
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Flags: needinfo?(yzenevich)
Tracked for 57 since it's a new regression.
Posted image win7.jpg
Be careful about tab bar overflow, the '+' button usually sits right where that icon is moving to.
Posted patch 1403867 patch v1 (obsolete) — Splinter Review
I added a comment in the CSS for visibility rules to make it clearer what is visible in what conditions.
Attachment #8914878 - Flags: review?(jhofmann)
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 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.
Attachment #8914878 - Flags: review?(jhofmann)
Makes sense will try this approach tomorrow (PTO today)
Posted patch 1403867 v2Splinter Review
Attachment #8914878 - Attachment is obsolete: true
Attachment #8915643 - Flags: review?(jhofmann)
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?
I'd say they should align with window buttons. ni'ing Amy for this.
Flags: needinfo?(amlee)
(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.
Flags: needinfo?(amlee)
(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.
Priority: -- → P1
Comment on attachment 8915643 [details] [diff] [review]
1403867 v2

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

Seems good to me, thank you.
Attachment #8915643 - Flags: review?(jhofmann) → review+
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6c355f7633a
fixing positioning of indicators when menubar persists. r=johannh
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
Attachment #8915643 - Flags: approval-mozilla-beta?
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?
Attachment #8915643 - Flags: approval-mozilla-beta?
(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.
(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.
follow up with selector clean up
Attachment #8916019 - Flags: review?(dao+bmo)
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.
Attachment #8916019 - Flags: review?(dao+bmo) → review+
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
Attachment #8915643 - Flags: approval-mozilla-beta?
Comment on attachment 8916019 [details] [diff] [review]
1403867 follow up

This is a follow up of the main bug. See comment 25
Attachment #8916019 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/c6c355f7633a
https://hg.mozilla.org/mozilla-central/rev/e0b45659c519
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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 on attachment 8915643 [details] [diff] [review]
1403867 v2

Photon related (I guess), Beta57+
Attachment #8915643 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8916019 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1411011
No longer depends on: 1411011
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.