Closed Bug 691609 Opened 13 years ago Closed 13 years ago

Floating scrollbars draw under border/background


(Core :: Layout, defect)

Not set





(Reporter: vingtetun, Assigned: mstange)




(1 file, 2 obsolete files)

The floating scrollbars used in Fennec is draw between the BorderBackground() DisplayList and the rest of the scrollbox childs frames.
This mean the scrollbars will be draw under any other type of content.
The patch moves the scrollbars from the BorderBackground() list to the PositionnedDescendant() list to ensure they stay on top.
Attachment #564404 - Flags: review?(roc)
After have read bug 657893, I think we want to take the code from this patch for moving the scrollbars to another display list instead of the relevant part of .
There is probably a comment I can borrow since it seems like the PositionnedDescendant() list is required to not mess up with layers and we probably want to keep this information to not change the list by error later.

I could be wrong since I don't know that much the layout code but I don't think there is any reason for keeping 2 calls to ::AppendScrollPartsTo() since as far as I understand if the scrollbars are placed in the PositionedDescendant() list, they will be ordered by z-index, and will leave on top of content by default if nothing has a defined z-index.

Patches in bug 657893 will probably merge easily on top of this one. 
Adding Florian to the discussion in case I have misunderstood his patches.
Attached patch alternative patch (obsolete) — Splinter Review
I hit the same problem in bug 636564. This is the patch I was going to attach there. It gets rid of the scrollbar overlay system metric / pref and instead makes the decision based on whether the scrollbar is positioned.
Does the mobile overlay scrollbar CSS include position:relative on the scrollbar? If not, it should.
Attachment #564462 - Flags: review?(roc)
(In reply to Markus Stange from comment #3)
> Does the mobile overlay scrollbar CSS include position:relative on the
> scrollbar? If not, it should.

It does. I do like the  idea of removing the pref.
Comment on attachment 564404 [details] [diff] [review]
Move the scrollbars from BorderBackground() to PositionedDescendants()

Removing r? since the latest proposed approach is likely better.
Attachment #564404 - Flags: review?(roc)
Comment on attachment 564462 [details] [diff] [review]
alternative patch

Review of attachment 564462 [details] [diff] [review]:

This patch looks good, but instead of having two functions, just pass a parameter to indicate whether to use the positioned scroll parts or not.
Attached patch v2Splinter Review
merged the functions
Assignee: nobody → mstange
Attachment #564404 - Attachment is obsolete: true
Attachment #564462 - Attachment is obsolete: true
Attachment #564462 - Flags: review?(roc)
Attachment #574045 - Flags: review?(roc)
Blocks: 636564
Can we land this bug or does it rely on something else?
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.