Closed Bug 926292 Opened 11 years ago Closed 10 years ago

Mac-scrollbar is behind z-index: 1

Categories

(Core :: Layout, defect)

24 Branch
x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox28 --- wontfix
firefox29 - affected
firefox30 - affected
firefox31 --- verified

People

(Reporter: dimasound, Assigned: mstange)

References

Details

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Steps to reproduce:

1. Goto http://diokuz.github.io/baron/
2. Scroll content in left block


Actual results:

Mac OsX style scrollbar is behind blue headers.


Expected results:

Scrollbar should always be above any html-elements.
It looks the same to me in both Safari and Firefox.  Are you sure this is a Firefox issue?  What should it look like?
Flags: needinfo?(dimasound)
I can see the blue header background overlaid over the scroll bar control when I scroll down.
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: CSS Object Model
Ever confirmed: true
Product: Firefox → Core
Already fixed for Firefox 25.
Status: NEW → RESOLVED
Closed: 11 years ago
Component: DOM: CSS Object Model → Layout
Resolution: --- → DUPLICATE
Er, never mind; that bug is allegedly fixed for 24 as well.
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(spohl.mozilla.bugs)
Resolution: DUPLICATE → ---
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to David Baron [:dbaron] (needinfo? me) from comment #4)
> Er, never mind; that bug is allegedly fixed for 24 as well.

Right, we fixed our side of things. Bug 891136 comment 5 explains in more detail why content can still break. There is at least one tech evangelism bug currently open (bug 920550). This is an unfortunate consequence of overlay scrollbars. Note that on that page however, the footer is actually *supposed* to cover the browser's scrollbars. The page then adds its own scrollbars. The only problem I can see on this page is that the footer's layout seems to break when resizing the browser window for example and/or after scrolling the left-hand text field, revealing the browser's scrollbars underneath. I'm not sure if the problem of the footer having the wrong size after resizing or scrolling the left text field is our issue or the content's. :dbaron, are you able to tell?
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(dbaron)
I don't currently have the time to investigate that.
Flags: needinfo?(dbaron)
Thanks David, no worries. Hmm, I'm hesitant to just close this without some more experienced eyes on the issue first. Rob, would you be able to tell if the footer's layout breaking is our issue (see comment 5)? Any advice on how to proceed would be appreciated. Thanks!
Flags: needinfo?(roc)
I don't have a Mac to test overlay scrollbars with. On Linux, I don't see any layout breakage when I scroll up and down in the left-hand pane interleaved with resizing the window.
Flags: needinfo?(roc)
Thanks Rob (sorry, I forgot you didn't have a Mac to test with). The layout issue reproduces at least as far back as nightly 2012-08-04, so this isn't tied to the new overlay scrollbars. I'm going to close this bug (as written) as duplicate of bug 891136 for now.
Status: NEW → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → DUPLICATE
This bug is not duplicate bug 891136!!! I dont know why Google Docs scrollbar was not working, but this bug still here, even on nightly build.

Take this!

http://jsfiddle.net/NFMZg/

just scroll .scroller and hover the .item - it will override scrollbar (Mac Os X 10.9, Firefox nightly 30.0a1 (2014-02-06)).
Status: RESOLVED → REOPENED
Flags: needinfo?(dimasound)
Resolution: DUPLICATE → ---
It's true that we don't display the scrollbars at all in this case. Safari displays the scrollbars, but when the mouse hovers over the scrollbar, it doesn't widen the scrollbar like it usually does (presumably because the mouse hover is consumed by the div). I'd say Safari's behavior is more appealing to the eye, but not fully functional either.

From an earlier conversation with Rob I recall that "... you can feature-detect disappearing scrollbars by creating an overflow:auto block, giving it a vertically overflowing block child, and testing the width of the child.". If you detect overlay scrollbars, you can reduce the width of your .item divs, ideally by 16px since that's the width of the scrollbars when hovered. What you end up with is something like this:
http://jsfiddle.net/LAW7W/1/ (note that this does not include the feature detection mentioned above)

I'll ni? Rob just to make sure that I'm not missing something.
Flags: needinfo?(roc)
That sounds right.
Flags: needinfo?(roc)
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → DUPLICATE
What the point of this "modern" scrollbar then?? I cant disable it either.

> Safari displays the scrollbars

Also Chrome, Opera, Yandex, Android and other webkit/blink related browsers - over 90% of Mac/iOs browsers.

> when the mouse hovers over the scrollbar, it doesn't widen the scrollbar like it usually does (presumably because the mouse hover is consumed by the div)

No! That's because its never going widen on divs.

> http://jsfiddle.net/LAW7W/1/

This is totally wired.

> 16px

This is the official firefox magic-response? Mkay...

Im pretty sure this bug is not duplicating bug 891136. This bug is about z-index overlay of mac-scrollbar. If you say this is not a bug - just set status to "WONTFIX".

But this is a bug.
Resolution: DUPLICATE → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
I think we should do something here but it's not obvious what to do.

Here are some examples:

<div style="overflow:auto"><div style="position:relative; z-index:100">...</div></div>
The scrollbar should be above the inner DIV so it's visible. That's what this bug and other bugs like it are about.

<div style="overflow:auto"><div style="position:absolute; width:200%; z-index:100">...</div></div>
The scrollbar should be below the inner DIV, because it would be below the inner DIV content in all browsers normally. It would be interesting to see how Chrome/Safari handle this case with overlay scrollbars. No Mac since I'm at home at the moment.

<div style="overflow:auto"></div>
<div style="position:absolute; width:200%; z-index:100">...</div>
The scrollbar should be below the second DIV, because it would be below the second DIV content in all browsers normally. Wonder how Chrome/Safari handle it.

<div style="overflow:auto; position:relative;"><div style="position:absolute; width:200%; z-index:100">...</div></div>
We probably want the scrollbar to be on top of the inner DIV in this case, since the inner DIV is clipped by the outer DIV and so the scrollbar would be visible if we weren't using overlay scrollbars.

So roughly, we want the scrollbar to be on top of elements that are being scrolled/clipped by the scrolling container, but otherwise under elements that are above the scrolling container in z-order.

Of course then you can construct examples where you can't do both. E.g.
<div style="overflow:auto;">
  <div style="position:absolute; width:200%; z-index:100">...</div>
  <div style="position:relative; z-index:200">...</div>
</div>
The overlay scrollbar should be over the rel-pos DIV because a non-overlay-scrollbar would not have been covered by it. But the overlay scrollbar should be under the abs-pos DIV because a non-overlay scrollbar would be covered by the abs-pos DIV. Of course we can't do both :-(.

One option might be: if the scrolled element has any visible positioned descendants that are scrolled by the element, put the overlay scrollbars above the topmost such positioned descendants in z-order. That would ensure the scrollbars are visible, but sometimes err on the side of the scrollbars being too high in z-order and being on top of content they shouldn't be. That would handle all the examples except the last one, where the scrollbar would be on top of the abs-pos DIV when it shouldn't. The behavior would be a little bit bizarre since the scrollbar z-position would depend on the contents of the scrolled element.
Scrollbar will be above inner divs in all cases (Chrome, Safari, Opera, Yandex)

http://jsfiddle.net/WUgbd/

And that make sense.

Only people from Opera presto (12-) agreed with you.
Funny, this bug can be a hack for baronjs problem)
I mean https://bugzilla.mozilla.org/show_bug.cgi?id=926294

David Baron, you should support your namesake js-library))
(In reply to Xerol from comment #16)
> Scrollbar will be above inner divs in all cases (Chrome, Safari, Opera,
> Yandex)
> 
> http://jsfiddle.net/WUgbd/

On Mac, maybe. On Linux Chrome, the scrollbar is under "inner".
(In reply to Xerol from comment #16)
> Scrollbar will be above inner divs in all cases (Chrome, Safari, Opera,
> Yandex)
> 
> http://jsfiddle.net/WUgbd/
> 
> And that make sense.

Chrome appears to be doing something adaptive like I suggested in comment #15. If you move "inner" outside the scrollable DIV (and give it top:0), suddenly the scrollbar stops covering it.
Attached patch v1 (obsolete) — Splinter Review
This implements the suggestion in the last paragraph of comment 15. I wrote the patch but roc told me how to do it.
Attachment #8392066 - Flags: review?(roc)
Comment on attachment 8392066 [details] [diff] [review]
v1

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

Looks mostly good. Fix the comment and let's get review from Mats.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2093,5 @@
> +MaxZIndexInList(nsDisplayList* aList, nsDisplayListBuilder* aBuilder)
> +{
> +  aList->SortByZOrder(aBuilder, nullptr);
> +  nsDisplayItem* top = aList->GetTop();
> +  return top ? top->ZIndex() : 0;

Don't sort the list. Just scan it and return the maximum z-value.
Attachment #8392066 - Flags: review?(roc)
Comment on attachment 8392066 [details] [diff] [review]
v1

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2109,5 @@
> +    new (aBuilder) nsDisplayOwnLayer(aBuilder, aSourceFrame, aSource,
> +                                     aFlags, aScrollTargetId) :
> +    new (aBuilder) nsDisplayWrapList(aBuilder, aSourceFrame, aSource);
> +
> +  newItem->SetOverrideZIndex(MaxZIndexInList(aDest, aBuilder));

Add a comment here explaining why we're doing this.
Attachment #8392066 - Attachment is obsolete: true
Attachment #8392668 - Flags: review?(roc)
Attachment #8392669 - Flags: review?(matspal)
Comment on attachment 8392668 [details] [diff] [review]
part 1: add a pref to stop overlay scrollbars from fading out

FYI, this doesn't compile:
layout/generic/ScrollbarActivity.cpp:35:1: error: control reaches end of non-void function [-Werror,-Wreturn-type]

You probably want to add this:
+  return sForceAlwaysVisible;
 }
Comment on attachment 8392669 [details] [diff] [review]
part 2: put scrollbars on top, with tests

>layout/base/nsDisplayList.h
>+   * Compute the used z-index of our frame; returns zero for elements to which
>+   * z-index does not apply, and for z-index:auto

End the sentence with a full stop.

I think it would be prudent to document here that the value can be
overridden, by adding for example:
@note This can be overridden, @see nsDisplayWrapList::SetOverrideZIndex.

>layout/generic/nsGfxScrollFrame.cpp
>+AppendToTop(nsDisplayListBuilder* aBuilder, const nsDisplayListSet& aLists,
> [...]
>+    newItem->SetOverrideZIndex(MaxZIndexInList(positionedDescendants, aBuilder));

FTR, I note that we're calling MaxZIndexInList() for each scroll part.
scrollParts.Length() is usually 1 or 2, occasionally 3, and the number of
positioned descendant is typically low as well, so I think it's OK.
Attachment #8392669 - Flags: review?(matspal) → review+
BTW, we have quite a few intermittent-failure tests that are failing due to fading scrollbars
on OSX.  See for example, bug 952788, bug 947688, bug 947089 etc.  Perhaps you could add
a comment on those bugs that this pref might help?
Hmm, I wonder if it would be better to set this pref to true inside the reftest
framework, and that tests that specifically want to test the fading of overlay
scrollbars should set it to false.  That seems like a more robust solution in
the long run.
(In reply to Mats Palmgren (:mats) from comment #27)
> BTW, we have quite a few intermittent-failure tests that are failing due to
> fading scrollbars
> on OSX.  See for example, bug 952788, bug 947688, bug 947089 etc.  Perhaps
> you could add
> a comment on those bugs that this pref might help?

Thanks for the pointers! I'll comment there.

(In reply to Mats Palmgren (:mats) from comment #28)
> Hmm, I wonder if it would be better to set this pref to true inside the
> reftest
> framework, and that tests that specifically want to test the fading of
> overlay
> scrollbars should set it to false.  That seems like a more robust solution in
> the long run.

It does. I'll file a new bug about that.


https://hg.mozilla.org/integration/mozilla-inbound/rev/8052705f985a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5acc6457aaa
We should try uplifting these patches to Aurora and ideally Beta because they fix what is effectively a regression on real sites.
https://hg.mozilla.org/mozilla-central/rev/8052705f985a
https://hg.mozilla.org/mozilla-central/rev/d5acc6457aaa
Assignee: nobody → mstange
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
followup on disabling overlay scrollbar fadeout in reftest is bug 986404
Ah, this plan was my other idea for fixing bug 976370, and likely makes that patch obsolete.
Depends on: 993517
Sylvestre, uplift to aurora and possibly beta was suggested in comment 30 so I flagged this bug for you to have a look.
Flags: needinfo?(sledru)
Liz, sure. Thanks for bringing that to my attention!

Markus, The patch seems a bit big. Do you think we should do the uplift for 29? Thanks
Flags: needinfo?(sledru) → needinfo?(mstange)
No need to track this, please nominate for uplift with a risk assessment and we can consider it for Aurora (30) which is soon to be Beta, but we've been shipping this regression for a while and we're done taking non security patches to 29 before release.
Depends on: 1016939
This will ride the train. Clearing the n-i
Flags: needinfo?(mstange)
Keywords: verifyme
Verified fixed on Firefox 31 Beta 4 (Build ID: 20140623175014) using Mac OS X 10.9.2. 

After discussing with Markus on IRC (thanks again!) I was able to confirm that the snippet from Comment 10 now has the expected behavior while System Preferences... > General > Show scroll bars is configured to "When scrolling" on Mac OS X.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.