When reconstructing a frame after a wrapped table-column/table-caption, we reinsert the new frame *before* instead of after, due to IsValidSibling() failure

REOPENED
Unassigned

Status

()

defect
REOPENED
6 years ago
6 months ago

People

(Reporter: dholbert, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
As described in bug 876074 comment 2, 3, and 8: when a style change makes us recreate the frame for an element, and that frame was previously right after an anonymous-table for a table-column or table-caption, then we end up inserting the newly created frame *before* the table instead of *after*.

This means our frame tree ends up different depending on whether we generate it from a static testcase vs. a dynamically modified testcase.  That's unfortunate.  

The underlying problem seems to be that IsValidSibling() returns false when comparing the new frame to the table-column frame, as described in bug 876074 comment 8, which makes us skip past the table-column and try to insert at an earlier point.
(Reporter)

Comment 1

6 years ago
Here's a testcase with a block container.
STR: Load this testcase; dump the frame tree.

EXPECTED RESULTS: the canvas should end up after the table-caption, sort of like in attachment 754249 [details] (the frame dump before the dynamic-tweak)

ACTUAL RESULTS: The canvas ends up before the table-caption (after boom() makes us recreate its frame), as shown in attachment 754250 [details] (a frame dump after the dynamic tweak)
(Reporter)

Comment 2

6 years ago
Here's a testcase with a flex container, which asserts when its children get out of order, making this easier to observe.

EXPECTED RESULTS: No assertion.

ACTUAL RESULTS: Assertion failure saying "Frame list should've been sorted in reflow" or "Child frames aren't sorted correctly".  (Bug 876074 changed the assertion language from the former to the latter.)
So somehow what we want to do is walk up the frame tree to the nearest ancestor that has the right parent type or something...
(Reporter)

Comment 4

2 years ago
This is likely a non-issue, now that we don't sort the flex items anymore, per bug 812687. (The assertion failures here were sanity-checks that our sorting happened correctly, but now there's no sorting.)

I'll see if I can remove the "asserts" annotation for the crashtest that was asserting here... ( 876074-1.html )  It's currently marked as asserts(0-5) because the assertion didn't fire reliably, so it's entirely likely it stopped asserting when bug 812687 landed.
(Reporter)

Comment 5

2 years ago
Try run with patch to remove the annotation:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4dcd4ac16e924f1ced7df3017a9c3372f224c6e
Flags: needinfo?(dholbert)

Comment 6

2 years ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/634197858d62
Remove stale asserts(0-5) annotation for crashtest that doesn't assert anymore. (no review)
(Reporter)

Comment 7

2 years ago
Try run looked good, so I pushed a patch to remove the annotation, and I'm calling this bug FIXED by bug 812687.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Depends on: 812687
Flags: needinfo?(dholbert)
Resolution: --- → FIXED
(Reporter)

Comment 8

2 years ago
Er, I guess this is actually still technically an issue (per expectation described in comment 1), but it just doesn't cause assertions anymore, as of bug 812687.

In particular -- a frame dump for testcase 1 still shows the table+caption appearing *after* the canvas:
  HTMLCanvas(canvas)
  ...
  TableWrapper(body)
  ...
  CaptionList

--> Reopening.  I guess this still needs something along the lines of comment 3 (though it's not clear to me that there's much badness resulting from this in the meantime).
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
The leave-open keyword is there and there is no activity for 6 months.
:svoisen, maybe it's time to close this bug?
Flags: needinfo?(svoisen)
(Reporter)

Comment 11

6 months ago
I only added "leave-open" so that the assertion annotation tweak (which was not a fix) wouldn't cause this bug to be autoclosed on merge.

This is still an issue (albeit a low-priority one).  So no, it shouldn't be closed.
Flags: needinfo?(svoisen)
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.