1498772 - Sticky elements in table are positioned incorrectly in RTL document

Status: RESOLVED WORKSFORME

(Core :: Layout: Positioned, defect)

Description

[thesdev]

Attachment: sticky-fail-restored.jpg

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0

Steps to reproduce:

I created a <table> with position:sticky <th> s in an RTL document. The fiddle is here: http://jsfiddle.net/rsmxa3og/ and the result is here: http://jsfiddle.net/rsmxa3og/embedded/result/#Result


Actual results:

The <th> s positions are calculated wrong, they are off center to left or right. It drastically shows when window gets maximized or restored.


Expected results:

the <th> s should stay in their correct normally expected positions.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

I agree this looks wrong. Does it happen for relative positioning as well? Or only sticky?

Do you know if it happens for other kinds of layouts or it's exclusive to <table>?

Comment 2

[thesdev]

It only happens for sticky.
I've used position: sticky in various places throughout my app and <table> is the only place I've seen this happening.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Thanks for that (and for the report! :P).

I'll try to take a look if I have the time since it's probably straight-forward to fix... The bug likely lies in StickyScrollContainer::ComputeStickyLimits:

  https://searchfox.org/mozilla-central/rev/26b40a44691e0710838130b614c2f2662bc91eec/layout/generic/StickyScrollContainer.cpp#159

Comment 4

[thesdev]

That's great news! Thank you.

Comment 5

[Jonathan Watt [:jwatt]]

This goes wrong at at:

https://searchfox.org/mozilla-central/rev/3e7afaa5b57b3f8ed100cd1f13bb0a93b9b2cb99/layout/generic/ReflowInput.h#950

We call into that function under the stack:

  mozilla::LogicalPoint::GetPhysicalPoint
  mozilla::ReflowInput::ApplyRelativePositioning
  nsTableRowFrame::ReflowChildren
  nsTableRowFrame::Reflow
  nsContainerFrame::ReflowChild
  nsTableRowGroupFrame::ReflowChildren
  nsTableRowGroupFrame::Reflow
  nsContainerFrame::ReflowChild
  nsTableFrame::ReflowChildren
  nsTableFrame::ReflowTable
  nsTableFrame::Reflow
  nsContainerFrame::ReflowChild
  nsTableWrapperFrame::OuterDoReflowChild
  nsTableWrapperFrame::Reflow
  nsBlockReflowContext::ReflowBlock
  nsBlockFrame::ReflowBlockFrame
  nsBlockFrame::ReflowLine
  nsBlockFrame::ReflowDirtyLines
  nsBlockFrame::Reflow
  nsContainerFrame::ReflowChild
  nsHTMLScrollFrame::ReflowScrolledFrame
  nsHTMLScrollFrame::ReflowContents
  nsHTMLScrollFrame::Reflow

That is, we're evaluating:

  nsSize frameSize = aFrame->GetSize();

before nsTableRowFrame::ReflowChildren has called FinishReflowChild to update aFrame's mRect, and so frameSize is out of date. The result returned by GetPhysicalPoint and assigned to 'pos' is then wrong, and that broken value is subsequently passed into the other ReflowInput::ApplyRelativePositioning overload the very next line and cached as a frame property.

Once we return all the way back up the call stack above to nsHTMLScrollFrame::Reflow we then call `mHelper.UpdateSticky();`, but that again uses the broken cached nsIFrame::NormalPositionProperty frame property.

Comment 6

[Jonathan Watt [:jwatt]]

Actually we pretty consistently call ApplyRelativePositioning before finishing overflow for positioned frames. I assume position:sticky is broken on RTL pages for a lot more than just tables.

I guess the question is whether we can move the ApplyRelativePositioning calls to occur after updating frames' positions, or if we need to pass the new (as yet unset) position into ApplyRelativePositioning.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Looks like for block layout we SetSize() before-hand:

  https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/layout/generic/nsBlockReflowContext.cpp#442

Which could explain why this works on normal blocks.

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

I don't see the the wrong result in the fiddle as shown in the image in comment 0 in current Nightly 127. I've tried mozregression to find the fix, but the older builds keep crashing. I'd say this bug has been fixed. Feel free to reopen or file a new bug if this is still happening.