930419 - Overinvalidation of position:sticky on scrolling

Status: VERIFIED FIXED

(Core :: Layout: Positioned, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

STR:
Turn on paint flashing.
Scroll http://html5-demos.appspot.com/static/css/sticky.html
or http://people.mozilla.org/~jmuizelaar/tmp/sticky/

Comment 1

[Jeff Muizelaar [:jrmuizel]]

This makes position sticky have much worse performance than it should.

Comment 2

[Daniel Holbert [:dholbert]]

I get the same sort of constant-invalidation if I manually "sticky" to "relative", FWIW. (using "inspect element") So, this isn't sticky-specific. --> Adjusting bug summary.

(Note that we implement sticky as an extended form of relative positioning, so it makes sense that this badness affects both positioning schemes.)

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Make position sticky frames an active scroll root

Comment 4

[Matt Woodrow (:mattwoodrow)]

I don't see any bugs with position:relative (on this test case at least).

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> I don't see any bugs with position:relative (on this test case at least).

Sorry, you're right -- I don't either. Turns out the post-"position"-tweaking invalidation that I was seeing was from having the element selected via "inspect element" -- that 'inspect' selection apparently makes us invalidate the element eagerly as we scroll. That's probably a bug, but is unrelated to this bug.

Comment 6

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

Comment on attachment 821627 [details] [diff] [review]
Make position sticky frames an active scroll root

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

::: layout/base/nsLayoutUtils.cpp
@@ +1169,4 @@
>      nsIScrollableFrame* sf = do_QueryFrame(parent);
> +    if (sf) {
> +      if (sf->IsScrollingActive() && sf->GetScrolledFrame() == f)
> +      {

{ on previous line

@@ +1174,5 @@
> +        // then use that. Otherwise use the scroll frame.
> +        if (stickyFrame) {
> +          return stickyFrame;
> +        } else {
> +          return f;

Don't do else after return.

@@ +1177,5 @@
> +        } else {
> +          return f;
> +        }
> +    } else {
> +      stickyFrame = nullptr;

This isn't right. The sticky frame is active if it has any active scrolling ancestor.

Comment 7

[Daniel Holbert [:dholbert]]

(I filed bug 930463 on the "inspect element" overinvalidation that I noted in comment 5 (and misdiagnosed in comment 2), BTW.)

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Make position sticky frames an active scroll root v2

Comment 9

[Matt Woodrow (:mattwoodrow)]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a6cad3e7ba5b

Comment 11

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed in latest Aurora 27.0a2 (buildID: 20131105004004).