Closed Bug 987680 Opened 6 years ago Closed 6 years ago

The scrollbar on MacOS is displayed below non-transparent items (awesomebar affected)

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox31 + verified

People

(Reporter: pbro, Assigned: tnikkel)

References

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

See the attached screenshot.
On Mac OSX, the floating scrollbar is displayed below the folders in the sources panel of the debugger.
Also happens in the inspector's computed-view (at least).
Component: Developer Tools: Debugger → Developer Tools
Summary: osx scrollbar below folders in debugger's sources panel → The scrollbar on MacOS is displayed below non-transparent sidebar items
I run into this issue too in the App Manager. I don't think it's devtools specific.
Component: Developer Tools → Graphics
Product: Firefox → Core
Attached image awesomebar
Did bug 926292 fix this?
(In reply to Timothy Nikkel (:tn) from comment #5)
> Did bug 926292 fix this?

Apparently not.
Summary: The scrollbar on MacOS is displayed below non-transparent sidebar items → The scrollbar on MacOS is displayed below non-transparent items (awesomebar affected)
Happens on Windows too in dark theme case, where the scrollbar is floating. Thus I am guessing that all scrollbars are now layer-wise behind the content of the box, just that floating scrollbars are inside of the content area. Still, its not devtools specific.

This started to happen recently, so its a regression from some platform landing.
OS: Mac OS X → All
On Mac at least, the regression window suggests to me that bug 926292 is cause:

Last good revision: 812c528dce3a
First bad revision: d5acc6457aaa
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=812c528dce3a&tochange=d5acc6457aaa

Markus, what do you think?
Blocks: 926292
Component: Graphics → Layout
Flags: needinfo?(mstange)
Attached patch patch (obsolete) — Splinter Review
We were placing the overlay scrollbars in the background layer if there weren't other positioned descendents, so behind everything but backgrounds. They need to be above everything in this case, so outlines.
Assignee: nobody → tnikkel
Attachment #8402347 - Flags: review?(roc)
Flags: needinfo?(mstange)
The only wrinkle with outlines is that we sort them by content order. If one of the nodes is anonymous the comparator will report them as equal. I think that should be fine since the scrollbar will already be on top, where we want it.
Comment on attachment 8402347 [details] [diff] [review]
patch

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

Please add a reftest. Floating scrollbars are always shown during reftests now so it should be easy.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2151,5 @@
> +    if (!positionedDescendants->IsEmpty()) {
> +      newItem->SetOverrideZIndex(MaxZIndexInList(positionedDescendants, aBuilder));
> +      positionedDescendants->AppendNewToTop(newItem);
> +    } else {
> +      aLists.Outlines()->AppendNewToTop(newItem);

Shouldn't this be Content()? We don't want them above positioned content in the same stacking context.
Attachment #8402347 - Flags: review?(roc) → review-
Attached patch patch + reftestSplinter Review
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Please add a reftest. Floating scrollbars are always shown during reftests
> now so it should be easy.

Done.

> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +2151,5 @@
> > +    if (!positionedDescendants->IsEmpty()) {
> > +      newItem->SetOverrideZIndex(MaxZIndexInList(positionedDescendants, aBuilder));
> > +      positionedDescendants->AppendNewToTop(newItem);
> > +    } else {
> > +      aLists.Outlines()->AppendNewToTop(newItem);
> 
> Shouldn't this be Content()? We don't want them above positioned content in
> the same stacking context.

It looks like in Gecko we put the Outlines and then the positioned descendents with z-index auto and >=0 (at least according to nsIFrame::BuildDisplayListForStackingContext). This seems to be allowed by the spec, but it recommends the outlines above the positioned descendents like you say. I can change it to Content if you prefer, I've left it as Outlines in this patch.
Attachment #8402347 - Attachment is obsolete: true
Attachment #8402437 - Flags: review?(roc)
Comment on attachment 8402437 [details] [diff] [review]
patch + reftest

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

No, you're right.
Attachment #8402437 - Flags: review?(roc) → review+
You made some reftests pass!

You didn't remove the failing annotations.

You made some reftests fail.

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/fe57c073f019
The one test that we are failing is layout/reftests/z-index/overlayscrollbar-sorting-2.html and this is because it expects non-positioned sibling content to cover scrollbars. I don't know if there is any way to win here, if the scrollframe doesn't form a stacking context we're going have to take some situation where the scrollbars aren't where we want them. Unless we do something even more complicated here (put the scrollbars on the lowest display item list that has scrolled content?).

Our behaviour with the patch from the bug matches Chrome and Safari on the reftest.

I propose putting the scrollbars on top in this case. What do you think?
Attachment #8402487 - Flags: review?(roc)
Comment on attachment 8402487 [details] [diff] [review]
mark tests passes, change text to expect scrollbar on top

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

Sure
Attachment #8402487 - Flags: review?(roc) → review+
And obviously that's going to cause failures on platforms that use classic scrollbars, in the background layer, so backed out until I can clean this up properly.
https://hg.mozilla.org/integration/mozilla-inbound/rev/16fb88e8f882
One note about non-osx os that use classic scrollbars - Windows/Linux has classic scrollbars, but in DevTools, Dark Theme, we make that scrollbar to appear as overlaying scrollbar by making it transparent and stuff. (You can see this all over the place in Dark Themed toolbox, specially in inspector).

Now if this scrollbar is still drawn in a background layer, will comment 7 still be valid , or it will be fixed ?
This bug seems quite similar to bug 815791 which was marked RESOLVED WONTFIX. I didn't insist on that one because it was related to using an add-on (tree style tab). Here the same behavior displays without that add-on, so may affect more users.
Sorry wrong bug number, the correct one is 925686.
Blocks: 971719
Depends on: 993619
No longer depends on: 993619
No longer blocks: 993638
Duplicate of this bug: 993638
If we are not using overlay scrollbars just disable the covering element, the test will pass by default, we aren't testing regular scrollbars in this test.

Writing this I noticed that we are only testing overlay scrollbars on tinderbox on android and b2g: based on the failing reftest images the mac tinderbox must have the system pref "always show scrollbars" set. I'm guessing that this is not the most common environment of actual users (or it won't be going forward), so we should probably work towards getting that changed (10.6 will still test classic scrollbars).
Attachment #8402487 - Attachment is obsolete: true
Attachment #8403905 - Flags: review?(roc)
Thank you for fixing this, Timothy.

(In reply to Timothy Nikkel (:tn) from comment #25)
> Created attachment 8403905 [details] [diff] [review]
> Writing this I noticed that we are only testing overlay scrollbars on
> tinderbox on android and b2g: based on the failing reftest images the mac
> tinderbox must have the system pref "always show scrollbars" set.

Indeed. 

> I'm
> guessing that this is not the most common environment of actual users (or it
> won't be going forward), so we should probably work towards getting that
> changed (10.6 will still test classic scrollbars).

Running tests on 10.9 will give us that coverage. See https://tbpl.mozilla.org/?showall=1&tree=Cedar&jobname=10.9 - failures caused by tests not expecting overlay scrollbars are one reason why we don't run those tests everywhere yet.
(In reply to Markus Stange [:mstange] from comment #26)
> Running tests on 10.9 will give us that coverage. See
> https://tbpl.mozilla.org/?showall=1&tree=Cedar&jobname=10.9 - failures
> caused by tests not expecting overlay scrollbars are one reason why we don't
> run those tests everywhere yet.

Good to know, thanks.

(In reply to Girish Sharma [:Optimizer] from comment #21)
> One note about non-osx os that use classic scrollbars - Windows/Linux has
> classic scrollbars, but in DevTools, Dark Theme, we make that scrollbar to
> appear as overlaying scrollbar by making it transparent and stuff. (You can
> see this all over the place in Dark Themed toolbox, specially in inspector).
> 
> Now if this scrollbar is still drawn in a background layer, will comment 7
> still be valid , or it will be fixed ?

I'm not sure how that's all implemented. If it's not fixed by this bug then please file a new bug and cc me. Thanks.
Blocks: 993517
Duplicate of this bug: 990327
We havea 4.89% tscroll regression on win8 as a result of the landing in comment 27.  

Here is a graph for it:
http://graphs.mozilla.org/graph.html#tests=[[287,131,31]]&sel=1396964336000,1397137136000&displayrange=7&datatype=running

for background on the test:
https://wiki.mozilla.org/Buildbot/Talos/Tests#tscroll.2C_tscrollx
Whiteboard: [talos_regression]
(In reply to Joel Maher (:jmaher) from comment #30)
> We havea 4.89% tscroll regression on win8 as a result of the landing in
> comment 27.  

We had the inverse improvement on March 20th from http://hg.mozilla.org/integration/mozilla-inbound/rev/d5acc6457aaa which introduced the bug that this patch fixes:
http://graphs.mozilla.org/graph.html#tests=[[287,131,31]]&sel=1395273391151.3914,1395298910292.08,6.9595184853549386,8.032138857196667&displayrange=30&datatype=running
Why would this have any effect either way on Windows 8? No overlay scrollbars there I don't think, so this should have been no change there.
Good point, I don't know.
https://hg.mozilla.org/mozilla-central/rev/dd92d0ebed9b
https://hg.mozilla.org/mozilla-central/rev/a745d6526479
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
I should have marked this as leave open when I mentioned the talos regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
filed bug 994861 to track the performance regression.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [talos_regression]
Keywords: verifyme
Blocks: 1008197
Duplicate of this bug: 993517
Verified fixed on Firefox 31 beta 6, build ID: 20140630185627
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.