Last Comment Bug 691609 - Floating scrollbars draw under border/background
: Floating scrollbars draw under border/background
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Markus Stange [:mstange]
:
Mentors:
Depends on:
Blocks: 636564 711986 770453
  Show dependency treegraph
 
Reported: 2011-10-03 17:27 PDT by Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
Modified: 2012-07-03 02:42 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Move the scrollbars from BorderBackground() to PositionedDescendants() (3.79 KB, patch)
2011-10-03 17:27 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
alternative patch (13.21 KB, patch)
2011-10-03 23:14 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
v2 (11.99 KB, patch)
2011-11-12 07:49 PST, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review

Description Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-10-03 17:27:16 PDT
Created attachment 564404 [details] [diff] [review]
Move the scrollbars from BorderBackground() to PositionedDescendants()

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.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-03 17:36:44 PDT
This came up in bug 657893. Can you extract the relevant part of https://bugzilla.mozilla.org/attachment.cgi?id=561485 ?
Comment 2 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-10-03 19:04:25 PDT
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 https://bugzilla.mozilla.org/attachment.cgi?id=561485 .
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.
Comment 3 Markus Stange [:mstange] 2011-10-03 23:14:45 PDT
Created attachment 564462 [details] [diff] [review]
alternative patch

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.
Comment 4 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-10-04 04:56:21 PDT
(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 5 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-10-04 08:39:07 PDT
Comment on attachment 564404 [details] [diff] [review]
Move the scrollbars from BorderBackground() to PositionedDescendants()

Removing r? since the latest proposed approach is likely better.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-04 15:01:10 PDT
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.
Comment 7 Markus Stange [:mstange] 2011-11-12 07:49:57 PST
Created attachment 574045 [details] [diff] [review]
v2

merged the functions
Comment 8 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-19 06:25:59 PST
Can we land this bug or does it rely on something else?
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-19 20:11:02 PST
It can land AFAIK
Comment 10 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-20 04:31:35 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/7f8c69ad0895
Comment 11 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-27 06:42:40 PST
https://hg.mozilla.org/mozilla-central/rev/7f8c69ad0895

Note You need to log in before you can comment on or make changes to this bug.