980247 - position:sticky elements with multiple continuations duplicate offsets on later continuations when parent is reflowed without reflowing the continuations

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Abel Lin(alin, abel)]

Attachment: sample

I thought the sticky problem only happened on B2G OOP. But the bug exists on desktop as well. As the attached, the symptom happens when launching(including maiming/scrolling/minimizing) the window. The layout rendering was disorder. But it is normal until reloading the page. Bug 973851 Comment 9, 10 are related  discussion to the bug. I elaborate the symptom more detailed.

summarize what I found, a normal layout showed up when reloading the html, the flow as below:

nsBlockFrame::ReflowDirtyLines(...) {
    ...
    for ( ; line != line_end; ++line, aState.AdvanceToNextLine()) {
        ...
        if (selfDirty)                 // selfDirty -> true
            line->MarkDirty();
        ...
        if (line->IsDirty() && ...) {  // line->IsDirty() -> true
            ReflowLine(...);           // it will call children's Reflow and PlaceLine
        } else {
        }
    }
}

So the current children's position is calculated correctly. And then the
StickyScrollContainer::PositionContinuations iterates the children to add
the translate against the css(#sticky.top).

But launching the html, the abnormal has the following flow in ReflowDirtyLines:
Eventually the 'selfDirty' was false. Some children's line dirty flag were set from somewhere rather than 'selfDirty'; other children's line dirty flag were not set. So the children without dirty flag wouldn't go into the ReflowLine to reset the right position.

Hence, the StickyScrollContainer::PositionContinuations will calculate the wrong sticky position to children without dirty flag, wrong calculation as below:
StickyScrollContainer::PositionContinuations(nsIFrame* aFrame)
{
  ...
  for (nsIFrame* cont = aFrame; cont;
       cont = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(cont)) {
    cont->SetPosition(cont->GetPosition() + translation);
  }
}

Get the frame' position and add the translate, and then set back again.
If the frame's position wasn't reset, the sticky position will be incremental.

If a iframe was forced to set dirty, the symptom was gone on desktop and b2g reftest. it seems like:

//http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?from=nsBlockFrame.cpp&case=true#1901
line->MarkDirty(); // added
if (line->IsDirty() && (line->HasFloats() || !willReflowAgain)) {
...
}

It feels like a dirty flag wasn't set to sticky case.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Marking the frame dirty and forcing an extra reflow seems like the wrong solution.  Instead, we should make the StickyScrollContainer use GetNormalPosition() instead of GetPosition().  (It's possible the normal position isn't currently stored correctly on continuations; if it isn't, we'd have to fix that as well.)

Comment 2

[Corey Ford [:coyotebush]]

Yes, it looks like PositionContinuations should just be written in terms of GetNormalPosition(). I think nsHTMLReflowState::ApplyRelativePositioning is called on continuations, so the stored normal position should be okay?

Comment 3

[Peter Chang[:pchang]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #1)
> Marking the frame dirty and forcing an extra reflow seems like the wrong
> solution.  Instead, we should make the StickyScrollContainer use
> GetNormalPosition() instead of GetPosition().  (It's possible the normal
> position isn't currently stored correctly on continuations; if it isn't,
> we'd have to fix that as well.)

Change the following GetPosition to GetNormalPosition could fix this issue.
http://dxr.mozilla.org/mozilla-central/source/layout/generic/StickyScrollContainer.cpp#337

But I found there are two more GetPosition() inside StickyScrollContainer.
http://dxr.mozilla.org/mozilla-central/source/layout/generic/StickyScrollContainer.cpp#260
http://dxr.mozilla.org/mozilla-central/source/layout/generic/StickyScrollContainer.cpp#332

Comment 4

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #3)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #1)
> > Marking the frame dirty and forcing an extra reflow seems like the wrong
> > solution.  Instead, we should make the StickyScrollContainer use
> > GetNormalPosition() instead of GetPosition().  (It's possible the normal
> > position isn't currently stored correctly on continuations; if it isn't,
> > we'd have to fix that as well.)
> 
> Change the following GetPosition to GetNormalPosition could fix this issue.
> http://dxr.mozilla.org/mozilla-central/source/layout/generic/
> StickyScrollContainer.cpp#337
> 
> But I found there are two more GetPosition() inside StickyScrollContainer.
> http://dxr.mozilla.org/mozilla-central/source/layout/generic/
> StickyScrollContainer.cpp#260
> http://dxr.mozilla.org/mozilla-central/source/layout/generic/
> StickyScrollContainer.cpp#332
I think we could keep this GetPosition because it tries to calculate the translation between GetPosition and GetNormalPosition

Comment 5

[Corey Ford [:coyotebush]]

The two instances of GetPosition in PositionContinuations would need to change at the same time.

The one at the end of ComputeStickyLimits shouldn't change, because 'rect' is computed by GetAllInFlowRectsUnion in terms of the frames' actual positions, not normal positions.

Comment 6

[Abel Lin(alin, abel)]

Attachment: sticky.patch

By my understanding, GetNormalPosition is to keep original relative position to parent. But GetPosition may be affected by SetPosition, which means the position information sometimes can't reflect right position.

Is my understanding wrong?

Comment 7

[Corey Ford [:coyotebush]]

Comment on attachment 8387559 [details] [diff] [review]
sticky.patch

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

::: layout/generic/StickyScrollContainer.cpp
@@ +256,5 @@
>    }
>  
>    // These limits are for the bounding box of aFrame's continuations. Convert
>    // to limits for aFrame itself.
> +  nsPoint frameOffset = aFrame->GetNormalPosition() - rect.TopLeft();

No, I'm pretty certain this line shouldn't change. Indeed, I hope some of our existing reftests break if you change this.

GetPosition() returns the actual current position of the frame. GetNormalPosition() returns the position minus any effects of relative/sticky positioning.

Comment 8

[Corey Ford [:coyotebush]]

(The problem here isn't that the result of GetPosition() is wrong per se, just that adding the same *incremental* sticky positioning change to all continuations isn't always correct.)

Comment 9

[Abel Lin(alin, abel)]

Got it.
Thanks.

Comment 10

[Abel Lin(alin, abel)]

Got it, thanks.

Comment 11

[Abel Lin(alin, abel)]

Attachment: sticky.patch

https://tbpl.mozilla.org/?tree=Try&rev=6f93afdb54a9

Comment 12

[Corey Ford [:coyotebush]]

Comment on attachment 8388426 [details] [diff] [review]
sticky.patch

This looks like the right change to me. It should have a better commit message that describes the fix (instead of the problem). We'll also want at least one test (probably a reftest in layout/reftests/position-sticky), which could be based on your original testcase, and can optionally be in a separate patch.

Anyway, I assume you meant to request dbaron's review again on this.

Comment 13

[Abel Lin(alin, abel)]

I found the patch solved partial problem. Another problem is StickyScrollContainer::ComputeStickyLimits.
The calculation of marginRect is wrong. Some cases would hit the condition like original reftest(position-sticky/block-in-inline-3.html).

By tracing the flow:
ComputeStickyLimits =>
nsLayoutUtils::GetAllInFlowRectsUnion(...,nsLayoutUtils::RECTS_USE_MARGIN_BOX) =>
...
nsLayoutUtils::GetAllInFlowBoxes =>
 while (aFrame) {
    AddBoxesForFrame(aFrame, aCallback);  // last frame position differs from normal position
 }
AddBoxesForFrame =>
aCallback->AddBox(aFrame) =>
outer->GetOffsetTo =>
nsIFrame::GetOffsetTo =>
  for (f = this; f != aOther && f; f = f->GetParent(), i++) {
      offset += f->GetPosition(); //  problem here!
  }

Wrong marginRect union is affectd by the wrong offset of last frame.
In normal case(reloading html), the return value is the same between GetPosition and GetNormalPosition.
In abnornal case, GetPosition is equal to GetNormalPosition.

I'm not sure the how GetPosition differs from GetNormalPosition.
Using GetNormalPosition can't solve the problem as well.

Comment 14

[Abel Lin(alin, abel)]

(In reply to Abel Lin(alin, abel) from comment #13)
> I found the patch solved partial problem. Another problem is
> StickyScrollContainer::ComputeStickyLimits.
> The calculation of marginRect is wrong. Some cases would hit the condition
> like original reftest(position-sticky/block-in-inline-3.html).
> 
> By tracing the flow:
> ComputeStickyLimits =>
> nsLayoutUtils::GetAllInFlowRectsUnion(...,nsLayoutUtils::
> RECTS_USE_MARGIN_BOX) =>
> ...
> nsLayoutUtils::GetAllInFlowBoxes =>
>  while (aFrame) {
>     AddBoxesForFrame(aFrame, aCallback);  // last frame position differs
> from normal position
>  }
> AddBoxesForFrame =>
> aCallback->AddBox(aFrame) =>
> outer->GetOffsetTo =>
> nsIFrame::GetOffsetTo =>
>   for (f = this; f != aOther && f; f = f->GetParent(), i++) {
>       offset += f->GetPosition(); //  problem here!
>   }
> 
> Wrong marginRect union is affectd by the wrong offset of last frame.
> In normal case(reloading html), the return value is the same between
> GetPosition and GetNormalPosition.
> In abnornal case, GetPosition is equal to GetNormalPosition.
> 
> I'm not sure the how GetPosition differs from GetNormalPosition.
> Using GetNormalPosition can't solve the problem as well.
correct:
is equal to => is NOT equal to

Comment 15

[Corey Ford [:coyotebush]]

It's not clear to me that there's anything wrong with computing the continuation union areas in terms of GetPosition(). As long as the continuations are in the right place relative to each other to begin with, it shouldn't matter, right?

Do you have a test case that would be fixed by changing the union calculations (or is even the original test case still broken)?

Comment 16

[Abel Lin(alin, abel)]

(In reply to Corey Ford [:corey] from comment #15)
> It's not clear to me that there's anything wrong with computing the
> continuation union areas in terms of GetPosition(). As long as the
> continuations are in the right place relative to each other to begin with,
> it shouldn't matter, right?
Right. But the wrong calculation is translation, not position relative to each other.
However, translation calculation is from ComputeStickyLimits.
> 
> Do you have a test case that would be fixed by changing the union
> calculations (or is even the original test case still broken)?
The original test case is still broken.
I am intended to look into how the continuation union is calculated.

I think few folks is aware the test case is broken because the test case is launched from begging, the result is ok. But after launching, if the html is zoomed/scrolled, the result is wrong. The patch solved the incremental problem, but the union calculation is still wrong.

Comment 17

[Abel Lin(alin, abel)]

Attachment: abnormal.mp4

Comment 18

[Abel Lin(alin, abel)]

Attachment: normal.mp4

Comment 19

[Abel Lin(alin, abel)]

Attachment: stick.patch2(unofficial)

abnormal.mp4 -> sticky.patch
normal.mp4 -> sticky.patch + stick.patch2(unofficial)

The 2nd patch shows the wrong union calculation is in nsIFrame::GetOffsetTo.
So i add a new function NormalPositionGetOffsetTo to get the normal position,
only in StickyScrollContainer::ComputeStickyLimit.

The patch is so ugly, but it can help me to explain how the union is wrong.

Comment 20

[Corey Ford [:coyotebush]]

Okay, I see that the first patch seems to fix your test case from comment 0, but doesn't fully fix the similar situation with the block-in-inline-3.html reftest. The second patch seems to help with that, but breaks some other reftests.

Changing the GetPosition() call at the end of ComputeStickyLimits to GetNormalPosition(), which I would have expected to be necessary along with changing the union computation, doesn't make that better. I don't completely understand what's going on in block-in-inline-3.html, though, so I'll poke at it some more.

Comment 21

[Corey Ford [:coyotebush]]

After a while in a debugger, I'm not sure I've learned anything new and useful, but I'll try to summarize what I see going on: on alternate reflows (as your video showed), the sticky element in block-in-inline-3.html moves up and down. This happens because the heights of rect and marginRect change (for me, alternating between 3960 and 6000), affecting the containing-block logic. In turn, that's because the result of GetPosition() on the last continuation ("after block") alternates. That makes sense (since it's moving up and down!), so the question is why GetPosition() on the other two continuations doesn't vary, even after having set it in a previous call to PositionContinuations().

Comment 22

[Corey Ford [:coyotebush]]

Come to think of it, though, this isn't the same as the original problem of duplicated offsets, so should probably be out of scope for this bug. Could we focus on landing the first patch (including one or more tests) and continue this discussion about block-in-inline-3.html in another bug?

Comment 23

[Abel Lin(alin, abel)]

(In reply to Corey Ford [:corey] from comment #22)
> Come to think of it, though, this isn't the same as the original problem of
> duplicated offsets, so should probably be out of scope for this bug. Could
> we focus on landing the first patch (including one or more tests) and
> continue this discussion about block-in-inline-3.html in another bug?
will do.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8388426 [details] [diff] [review]
sticky.patch

How do we ever get in a situation where the different continuations of a sticky element have different (GetPosition() - GetNormalPosition()) from each other?  And if we do, don't we end up with a bug if PositionContinuations doesn't get called?

Using GetNormalPosition() here seems reasonable, but now that I think about it more it feels like it's covering up a problem rather than fixing it.  Is the problem that nsHTMLReflowState::ApplyRelativePositioning is doing its work in a case where it shouldn't because we've decided to split the frame but haven't actually split it yet?  If that's actually the problem, this might be a reasonable workaround, although it would probably be best to also update the comments in nsHTMLReflowState::ApplyRelativePositioning.

>Bug 980247 - position:sticky elements with multiple continuations duplicate offsets on later continuations when parent is reflowed without reflowing the continuations

This could use a better commit message, as corey said.  Perhaps:

Bug 980247 - Use offsets from GetNormalPosition() when updating continuations of position:sticky elements instead of assuming that they're all currently offset by the same amount.


>* * *
>try: -b do -p all -u all -t none

Remove the try syntax



r=dbaron with that, but you should also add a reftest for what this is fixing, and this shouldn't land without the test

Comment 25

[Corey Ford [:coyotebush]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #24)
> Using GetNormalPosition() here seems reasonable, but now that I think about
> it more it feels like it's covering up a problem rather than fixing it.

Hmm, you might be right. No matter which way we write PositionContinuations, we shouldn't be trying to push the sticky element 20px farther down each time... unless we forgot that we pushed the first continuation down already, which is consistent with my debugging results.

So now I see how these two situations quite possibly have the same root cause that we should fix.

Comment 26

[Abel Lin(alin, abel)]

Attachment: sticky.reftest

The test is to verify the continuations.

Comment 27

[Abel Lin(alin, abel)]

Attachment: sticky.patch

Comment 28

[Abel Lin(alin, abel)]

Attachment: sticky.reftest

https://tbpl.mozilla.org/?tree=Try&rev=96ebbc4472a7

Comment 29

[Corey Ford [:coyotebush]]

Comment on attachment 8390416 [details] [diff] [review]
sticky.patch

Carrying forward dbaron's r+ on the C++ patch.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Abel Lin(alin, abel) from comment #28)
> Created attachment 8390584 [details] [diff] [review]
> sticky.reftest
> 
> https://tbpl.mozilla.org/?tree=Try&rev=96ebbc4472a7

Looks like the tests you pushed to try were attachment 8390412 [details] rather than attachment 8390584 [details] [diff] [review].  (Which is good, given the diffs between them.)

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8390584 [details] [diff] [review]
sticky.reftest

>Bug 980247 - Use offsets from GetNormalPosition() when updating continuations of position:sticky elements instead of assuming that they're all currently offset by the same amount

Please use a commit message related to what this patch is doing, so something like:

Bug 980247 - Add a reftest for ...

>diff --git a/layout/reftests/position-sticky/block-in-inline-continuations-ref.html b/layout/reftests/position-sticky/block-in-inline-continuations-ref.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/position-sticky/block-in-inline-continuations-ref.html

Since you appear to have copied this from block-in-inline-3, could you record it as an hg copy?

>+      #scroll {== block-in-inline-continuations.html block-in-inline-continuations-ref.html

You need to fix this (it was introduced between the two patches).


>+    <meta name="assert" content="Inline elements split and they contain blocks should always have all parts moved the same offset from their normal position when scrolling(Bug 980247)">

Put a space before the "(".

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8390584 [details] [diff] [review]
sticky.reftest

>+        font: 20px/1;

I think it's probably better to restore use of the Ahem font if you can.  (Both in this 'font' declaration, and the link to ahem.css.)  Doing that reduces the risk of creating a test that only tests what is intended on some platforms, since it avoids most of the variation in font metrics between platforms.

But you'll need to retest after that change that the test still fails before the patch and passes with the patch.




In the test file, please put the class="reftest-wait" on the root element in markup rather than setting it in script.

>+        document.documentElement.removeAttribute("class");
>+        doScroll(20);

It seems better to write these in the other order.


>+      }, false);
>+    </script>
>+</body>
>+</html>
>+
>+

Please restore the indentation of </body> and don't add two blank lines at the end.


r=dbaron with that and comment 31, assuming that you've tested that the test fails without the patch and passes with it.  (In general, you should test tests both before and after the patch, and you should say that you've done so when uploading them so your reviewer knows you've done so.)

Comment 33

[Abel Lin(alin, abel)]

Attachment: sticky.reftest

I maybe messed my reftest before. So I didn't know how it passed on my local side.
It means that the scrolling in patch reftest can't detect the problem of incrmental offset.
The operation of resizing window can repro it. So I use a approach to do it.
Using iframe can be resized. The new sticky.reftest used the appraoch. So please review it again.

Test result:
  
// only sticky.reftest, verified the sticky position problem
https://tbpl.mozilla.org/?tree=Try&rev=85682dcd8b55

// sticky.reftest + sticky.patch, all pass
https://tbpl.mozilla.org/?tree=Try&rev=fc7d4d6f2fb7

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8391144 [details]
sticky.reftest

>diff --git a/layout/reftests/position-sticky/block-in-inline-continuations-inner.html b/layout/reftests/position-sticky/block-in-inline-continuations-inner.html

>+    <title>CSS Test: Sticky Positioning - continuations have no effect when zooming</title>

Maybe change this from "CSS Test" to "iframe for CSS Test"

>+    <link rel="match" href="block-in-inline-continuations-ref.html">
>+    <meta name="assert" content="Inline elements split and contain blocks should always have all parts moved the same offset from their normal position">

Don't have these on the inner iframe.

>+
>+
>+

Best to avoid these blank lines at the start and end of some of the files.

>diff --git a/layout/reftests/position-sticky/block-in-inline-continuations-ref-inner.html b/layout/reftests/position-sticky/block-in-inline-continuations-ref-inner.html

>+    <title>CSS Test: Sticky Positioning - continuations have no effect when zooming</title>

"iframe for CSS Test Reference: ..."

>diff --git a/layout/reftests/position-sticky/block-in-inline-continuations-ref.html b/layout/reftests/position-sticky/block-in-inline-continuations-ref.html

>+    <title>CSS Test: Sticky Positioning - continuations have no effect when zooming</title>

"CSS Test Reference: ..."


r=dbaron with those minor changes, and thanks for fixing the reftest




(In reply to Abel Lin(alin, abel) from comment #33)
> Test result:
>   
> // only sticky.reftest, verified the sticky position problem
> https://tbpl.mozilla.org/?tree=Try&rev=85682dcd8b55
> 
> // sticky.reftest + sticky.patch, all pass
> https://tbpl.mozilla.org/?tree=Try&rev=fc7d4d6f2fb7

You didn't need to run such huge try runs for these.  It would have been fine to run just reftests on a platform or two, and to do only the test + patch run on try.  When I talked about testing the test both with and without the patch in comment 32, I meant that testing locally would be fine; there was no need for a try run to satisfy that request.

Comment 35

[Abel Lin(alin, abel)]

Attachment: sticky.reftest

https://tbpl.mozilla.org/?tree=Try&rev=d25a04789351

Comment 36

[Ryan VanderMeulen [:RyanVM]]

It's not clear what all needs landing here? Please make sure obsolete/unnecessary patches are marked as such and any tests that need landing are included as well.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/becd60bbd383
https://hg.mozilla.org/integration/mozilla-inbound/rev/52755193f692

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Fix a brilliant rebase error on my part:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6598c5d66e7

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/becd60bbd383
https://hg.mozilla.org/mozilla-central/rev/52755193f692
https://hg.mozilla.org/mozilla-central/rev/e6598c5d66e7