1547759 - mis-positioned row background in a position: relative table cell with RTL direction

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P3)

Description

[chris]

Attachment: Reproduction case HTML

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.108 Safari/537.36

Steps to reproduce:

See the attached HTML which is a much reduced reproduction case from our software.

Actual results:

The background for the td.dataList-cell.dataList-cell--iconic element renders outside of the cell, unexpectedly.

You can get it to recalculate by changing the size of the browser window.

If you change the "dir" from "rtl" to "ltr", it renders normally.

It also renders normally in other browsers.

If you remove position: relative from the cell it seems to resolve it.

Sometimes it seems intermittent, but it is mostly broken.

Expected results:

The cell with the icon should have a red background.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e6e712904806da25a9c8f48ea4533abe7c6ea8f4&tochange=d6bf703c5deaf1e328babd03d5e68ff2a4ffe10e

Via local build:
Last good: 395b6c53e42b064c4463d5156df5baa23d98dc5d
First bad: 1e3130e96f03b7e3bff220e05c5e06b54f0c285f

Triggered by: 1e3130e96f03b7e3bff220e05c5e06b54f0c285f L. David Baron — Bug 1308876 - Mark child frames as dirty before starting reflow of the parent, so that if we reflow a child twice, it's only dirty the first time. r=dholbert

Comment 2

[Patricia Lawless]

Bulk change of P3 carryover bugs to wontfix for 68.

Comment 3

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

This is not fixed by bug 1474771, so it needs further investigation.

Steps to reproduce are that the heart icon should have a red square background behind it -- whereas it's incorrectly showing up to the right of the heart, with a blank area behind the heart.

Comment 4

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

Attachment: simpler testcase

Comment 5

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

The frame tree looks correct at first glance. My suspicion is that something is wrong with the cell's GetNormalPosition() and that produces incorrect behavior in nsTableRowFrame::PaintCellBackgroundsForFrame.

Comment 6

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

So I debugged what's going on here. The problem essentially comes down to this pair of function calls. We call ApplyRelativePositioning and we pass it the container size (since the container might be in reflow, and its size might not have been set yet)... but we don't do the same for the frame's size. In this case, the frame's size doesn't get set until the FinishReflowChild call, which correctly computes a physical position based on the logical position, the frame's size, and the container's size.

This seems like it might be a problem common to multiple ApplyRelativePositioning callers. I suspect there are other bugs with relative/sticky positioning and RTL. Though perhaps the way table rows use the normal position is unusual.

One option might be restructuring the code; another might be storing the normal position as a LogicalPoint.

Comment 7

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

I went through all the callers of ApplyRelativePositioning, and I think most of them are actually fine. I think there are seven that are buggy in that they call ApplyRelativePositioning before setting a size just computed in reflow:

• nsCanvasFrame::Reflow (though I don't think this ever matters)
• nsFlexContainerFrame::ReflowFlexItem (but I don't think this one matters either since I think nsFlexContainerFrame::MoveFlexItemToFinalPosition fixes things)
• nsGridContainerFrame::ReflowInFlowChild
• nsTableFrame::PlaceRepeatedFooter
• nsTableFrame::ReflowChildren
• nsTableRowFrame::ReflowChildren (the first call in the function) (the one relevant to this bug)
• nsTableRowGroupFrame::ReflowChildren

Comment 8

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

Oh, and one case of this bug was explicitly fixed before, in 79d17c017fb6

Comment 9

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

Here's a try run.

Comment 10

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

And a new try run with an assertion condition corrected.

Comment 11

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

And yet another try run, with more assertions fixed, and more assertions added (and more patches added).

(And note that the last two try runs don't show all patches since the ones at the bottom of the stack are unmodified.)

Comment 12

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

And another try run.

Comment 13

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

And here's yet another try run with a better version of the logical conversion in nsTableFrame::PlaceChild.

Comment 14

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

Attachment: Bug 1547759 - Print frame's normal position (when present) in frame dump debugging output.

Comment 15

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

Attachment: Bug 1547759 - Add reftest.

Comment 16

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

Attachment: Bug 1547759 - Pass ReflowInput to FinishReflowChild in almost all cases.

At first glance, it might look like this would change behavior, since
FinishReflowChild passes aReflowInput to DidReflow, which in turn
notifies aReflowInput's mPercentBSizeObserver. However, if you examine
how the mPercentBSizeObserver is propagated, it can only be set for the
anonymous block wrapping the children of table cells, the children of
table cells, or additionally a child of a table wrapper frame that has
it set (i.e., a table or its caption, when logically a child of a table
cell). Since all of the frames for which this is being changed are
either internal table elements that are inside of the table, or are
things that can never be a descendant of a table at all, there should be
no change in behavior.

Comment 17

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

Attachment: Bug 1547759 - Add a flag to allow FinishReflowChild to handle relative positioning, and convert the caller for which this makes sense.

Note that this changes the writing mode used for the SetSize call from
the parent's to the child's. (This controls what position is kept
constant.) I don't think this changes behavior at all, since there is
(for this caller) a SetRect call just below.

Comment 18

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

Attachment: Bug 1547759 - Switch nsTableFrame to use the logical version of FinishReflowChild.

Comment 19

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

Attachment: Bug 1547759 - Ensure that we call ApplyRelativePositioning after the frame's new size has been set so it works correctly for RTL.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d03476b033f
Print frame's normal position (when present) in frame dump debugging output. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/d41e25253691
Add reftest. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/70b58d442347
Pass ReflowInput to FinishReflowChild in almost all cases. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/801afb0532e2
Add a flag to allow FinishReflowChild to handle relative positioning, and convert the caller for which this makes sense. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/46ad5eb1a666
Switch nsTableFrame to use the logical version of FinishReflowChild. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/ae11564c4d7f
Ensure that we call ApplyRelativePositioning after the frame's new size has been set so it works correctly for RTL. r=jfkthame,dholbert

Comment 21

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/6d03476b033f
https://hg.mozilla.org/mozilla-central/rev/d41e25253691
https://hg.mozilla.org/mozilla-central/rev/70b58d442347
https://hg.mozilla.org/mozilla-central/rev/801afb0532e2
https://hg.mozilla.org/mozilla-central/rev/46ad5eb1a666
https://hg.mozilla.org/mozilla-central/rev/ae11564c4d7f

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.