Overinvalidation of position:sticky on scrolling

VERIFIED FIXED in Firefox 27

Status

()

Core
Layout: R & A Pos
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: jrmuizel, Unassigned)

Tracking

({perf})

Trunk
mozilla27
Points:
---

Firefox Tracking Flags

(firefox27+ verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Comment 1

4 years ago
This makes position sticky have much worse performance than it should.
tracking-firefox27: --- → ?
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.)
Component: Keyboard: Navigation → Layout: R & A Pos
Depends on: 886646
Keywords: perf
OS: Mac OS X → All
Hardware: x86 → All
Summary: Always invalidating position:sticky → Overinvalidation of position:sticky / position:relative on scrolling
Version: unspecified → Trunk
No longer depends on: 886646
Created attachment 821627 [details] [diff] [review]
Make position sticky frames an active scroll root
Attachment #821627 - Flags: review?(roc)
I don't see any bugs with position:relative (on this test case at least).
(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 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.
(I filed bug 930463 on the "inspect element" overinvalidation that I noted in comment 5 (and misdiagnosed in comment 2), BTW.)
Summary: Overinvalidation of position:sticky / position:relative on scrolling → Overinvalidation of position:sticky on scrolling
Created attachment 821649 [details] [diff] [review]
Make position sticky frames an active scroll root v2
Attachment #821627 - Attachment is obsolete: true
Attachment #821627 - Flags: review?(roc)
Attachment #821649 - Flags: review?(roc)
Blocks: 916315
https://hg.mozilla.org/mozilla-central/rev/a6cad3e7ba5b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Updated

4 years ago
status-firefox27: --- → fixed
tracking-firefox27: ? → +

Updated

4 years ago
Keywords: verifyme
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).
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.