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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Patrick Brosset <:pbro>]

Attachment: Screenshot 2014-03-25 15.08.28.png

See the attached screenshot.
On Mac OSX, the floating scrollbar is displayed below the folders in the sources panel of the debugger.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: Screenshot 2014-03-31 15.02.33.png

Also happens in the inspector's computed-view (at least).

Comment 2

[Paul Rouget [:paul]]

I run into this issue too in the App Manager. I don't think it's devtools specific.

Comment 3

[Paul Rouget [:paul]]

http://i.imgur.com/ku02DBY.png

Comment 4

[Paul Rouget [:paul]]

Attachment: awesomebar

Comment 5

[Timothy Nikkel (:tnikkel)]

Did bug 926292 fix this?

Comment 6

[Paul Rouget [:paul]]

(In reply to Timothy Nikkel (:tn) from comment #5)
> Did bug 926292 fix this?

Apparently not.

Comment 7

[Girish Sharma [:Optimizer]]

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.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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?

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: patch

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.

Comment 11

[Timothy Nikkel (:tnikkel)]

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 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: patch + reftest

(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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8402437 [details] [diff] [review]
patch + reftest

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

No, you're right.

Comment 15

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9085d8ca22e

Comment 16

[Phil Ringnalda (:philor)]

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

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: mark tests passes, change text to expect scrollbar on top

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?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 19

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d234394e0147
https://hg.mozilla.org/integration/mozilla-inbound/rev/389ff4b5612c

Comment 20

[Timothy Nikkel (:tnikkel)]

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

Comment 21

[Girish Sharma [:Optimizer]]

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 ?

Comment 22

[André Reinald]

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.

Comment 23

[André Reinald]

Sorry wrong bug number, the correct one is 925686.

Comment 25

[Timothy Nikkel (:tnikkel)]

Attachment: mark tests passes, change text to expect scrollbar on top v2

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).

Comment 26

[Markus Stange [:mstange]]

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.

Comment 27

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd92d0ebed9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a745d6526479

Comment 28

[Timothy Nikkel (:tnikkel)]

(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.

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 31

[Markus Stange [:mstange]]

(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

Comment 32

[Timothy Nikkel (:tnikkel)]

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.

Comment 33

[Markus Stange [:mstange]]

Good point, I don't know.

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dd92d0ebed9b
https://hg.mozilla.org/mozilla-central/rev/a745d6526479

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

I should have marked this as leave open when I mentioned the talos regression.

Comment 36

[Joel Maher ( :jmaher ) (UTC -8)]

filed bug 994861 to track the performance regression.

Comment 38

[Cornel Ionce [:noni] [Hubs QA]]

Verified fixed on Firefox 31 beta 6, build ID: 20140630185627