Closed
Bug 1324661
Opened 8 years ago
Closed 8 years ago
stylo: several tests non-fatally assert with "Table wrapper frames cannot have borders or paddings"
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: heycam, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
(deleted),
text/x-review-board-request
|
bholley
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
bholley
:
review+
|
Details |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
###!!! ASSERTION: Table wrapper frames cannot have borders or paddings: 'aBorder.IsAllZero() && aPadding.IsAllZero()', file /home/worker/workspace/build/src/layout/tables/nsTableWrapperFrame.cpp, line 436 ###!!! ASSERTION: Wrong parent style context: 'Error', file /home/worker/workspace/build/src/layout/base/RestyleManagerBase.cpp, line 302 gfx/tests/crashtests/783041-1.html layout/base/crashtests/405186-1.xhtml layout/generic/crashtests/1281102.html
Reporter | ||
Comment 1•8 years ago
|
||
This is fixed in the latest mozilla-central -> stylo merge.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 2•8 years ago
|
||
This didn't get fixed after all (see bug 1334938); sorry for the noise.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 3•8 years ago
|
||
layout/style/test/test_animations_async_tests.html layout/style/test/test_animations_effect_timing_enddelay.html layout/style/test/test_animations_event_order.html layout/style/test/test_animations_omta_start.html layout/style/test/test_animations_playbackrate.html layout/style/test/test_at_rule_parse_serialize.html layout/style/test/test_unclosed_parentheses.html
Blocks: 1337615
![]() |
Assignee | |
Comment 4•8 years ago
|
||
This breaks this testcase: <table style="position: absolute; top: 200px; left: 0px; border: 10px solid black"> <tr><td onclick="document.querySelector('table').style.top = '400px'">Hey</td></tr> </table> in terms of the rendering (while fixing the assertion on it), due to bug 1292609 and the table-outer not getting its updated "top" value... Going to poke at that a bit.
![]() |
Assignee | |
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Status: REOPENED → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8838615 [details] Bug 1324661 part 1. When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables. https://reviewboard.mozilla.org/r/113460/#review114984 ::: layout/base/ServoRestyleManager.cpp:136 (Diff revision 1) > +static void > +UpdateStyleContextForTableWrapper(nsIFrame* aTableWrapper, > + nsStyleContext* aTableStyleContext, > + ServoStyleSet* aStyleSet) > +{ > + // XXXbz Do we need to add any change hints here? Presumably it may be enough to pass this frame instead of the style frame to the frame change list, since I believe the style changes will be only inherited, right? Though I don't know if the table frame has some magic on the functions that RestyleManager::ProcessRestyledFrames have to look and update the wrapper frame too? ::: layout/base/ServoRestyleManager.cpp:146 (Diff revision 1) > + aStyleSet->ResolveAnonymousBoxStyle(nsCSSAnonBoxes::tableWrapper, > + aTableStyleContext, > + 0 /* Are these the right flags? */); > + aTableWrapper->SetStyleContext(newContext); > +} > + nit: trailing whitespace. ::: layout/base/ServoRestyleManager.cpp:157 (Diff revision 1) > ServoStyleSet* aStyleSet, > nsStyleChangeList& aChangeListToProcess) > { > - nsIFrame* primaryFrame = aElement->GetPrimaryFrame(); > + nsIFrame* styleFrame; > + if (nsIFrame* primaryFrame = aElement->GetPrimaryFrame()) { > + styleFrame = nsLayoutUtils::GetStyleFrame(primaryFrame); Given we're doing this, we could also just use nsLayoutUtils::GetStyleFrame(aElement), right?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
![]() |
Assignee | |
Comment 12•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8838615 [details] Bug 1324661 part 1. When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables. https://reviewboard.mozilla.org/r/113460/#review114984 > Presumably it may be enough to pass this frame instead of the style frame to the frame change list, since I believe the style changes will be only inherited, right? > > Though I don't know if the table frame has some magic on the functions that RestyleManager::ProcessRestyledFrames have to look and update the wrapper frame too? OK, now that I'm more awake I read through the Gecko code here again. What Gecko does when it detects this case is it restyles the _child_ (that is the table frame). Then it makes sure that any hints handled for that child frame are applied to the table wrapper as well; that's the "assumeDifferenceHint" bit in ElementRestyler::RestyleSelf. At least I think that's how it works. So in terms of our stuff here, I guess we can just add _both_ frames to the change list with the "changeHint" that Servo_TakeChangeHint returned? > nit: trailing whitespace. Yes, fixed in the most recent version. > Given we're doing this, we could also just use nsLayoutUtils::GetStyleFrame(aElement), right? Oh, good catch. Done.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8838615 [details] Bug 1324661 part 1. When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables. https://reviewboard.mozilla.org/r/113460/#review114984 > OK, now that I'm more awake I read through the Gecko code here again. What Gecko does when it detects this case is it restyles the _child_ (that is the table frame). Then it makes sure that any hints handled for that child frame are applied to the table wrapper as well; that's the "assumeDifferenceHint" bit in ElementRestyler::RestyleSelf. At least I think that's how it works. > > So in terms of our stuff here, I guess we can just add _both_ frames to the change list with the "changeHint" that Servo_TakeChangeHint returned? Ok, yeah, I think that'd work, though presumably Gecko's restyle manager also takes care of the "handled for descendants" bits, right? (so we don't repaint or reflow twice, etc). I think we should push the parent with the hint that `Servo_TakeChangeHint` returns, and the child with `NS_HintsNotHandledForDescendants(thatHint)`, what do you think?
![]() |
Assignee | |
Comment 16•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8838615 [details] Bug 1324661 part 1. When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables. https://reviewboard.mozilla.org/r/113460/#review114984 > Ok, yeah, I think that'd work, though presumably Gecko's restyle manager also takes care of the "handled for descendants" bits, right? (so we don't repaint or reflow twice, etc). > > I think we should push the parent with the hint that `Servo_TakeChangeHint` returns, and the child with `NS_HintsNotHandledForDescendants(thatHint)`, what do you think? Yes, that makes sense.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8838615 [details] Bug 1324661 part 1. When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables. https://reviewboard.mozilla.org/r/113460/#review115046 ::: layout/base/ServoRestyleManager.cpp:160 (Diff revision 5) > // Although we shouldn't generate non-ReconstructFrame hints for elements with > // no frames, we can still get them here if they were explicitly posted by > // PostRestyleEvent, such as a RepaintFrame hint when a :link changes to be > // :visited. Skip processing these hints if there is no frame. > - if ((primaryFrame || changeHint & nsChangeHint_ReconstructFrame) && changeHint) { > - aChangeListToProcess.AppendChange(primaryFrame, aElement, changeHint); > + if ((styleFrame || (changeHint & nsChangeHint_ReconstructFrame)) && changeHint) { > + if (isTable) { Probably `MOZ_UNLIKELY`? But not a big deal. ::: layout/base/ServoRestyleManager.cpp:226 (Diff revision 5) > - for (nsIFrame* f = primaryFrame; f; > + for (nsIFrame* f = styleFrame; f; > f = GetNextContinuationWithSameStyle(f, oldStyleContext)) { > f->SetStyleContext(newContext); > } > > + MOZ_ASSERT(styleFrame, I'd move this assert down given I'm writing a patch for display: contents right now. Though I can also remove it myself when rebasing, I guess :)
Attachment #8838615 -
Flags: review+
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8838615 [details] Bug 1324661 part 1. When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables. https://reviewboard.mozilla.org/r/113460/#review115030
Attachment #8838615 -
Flags: review?(bobbyholley) → review+
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8838616 [details] Bug 1324661 part 2. Reenable various table reftests that were disabled before. They all seem to be passing, or at least failing for unrelated reasons, with the part 1 patch applied. https://reviewboard.mozilla.org/r/113462/#review115050 nice :)
Attachment #8838616 -
Flags: review+
Comment 22•8 years ago
|
||
mozreview-review |
Comment on attachment 8838616 [details] Bug 1324661 part 2. Reenable various table reftests that were disabled before. They all seem to be passing, or at least failing for unrelated reasons, with the part 1 patch applied. https://reviewboard.mozilla.org/r/113462/#review115052
Attachment #8838616 -
Flags: review?(bobbyholley) → review+
![]() |
Assignee | |
Comment 23•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8838615 [details] Bug 1324661 part 1. When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables. https://reviewboard.mozilla.org/r/113460/#review115046 > Probably `MOZ_UNLIKELY`? But not a big deal. I don't think it's worth it, esp because on some pages it's not too unlikely. ;)
![]() |
Assignee | |
Comment 24•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8838615 [details] Bug 1324661 part 1. When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables. https://reviewboard.mozilla.org/r/113460/#review115046 > I'd move this assert down given I'm writing a patch for display: contents right now. Though I can also remove it myself when rebasing, I guess :) Good catch, fixed.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
![]() |
Assignee | |
Comment 27•8 years ago
|
||
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8838198 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 28•8 years ago
|
||
Comment 29•8 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a300522006bc part 1. When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables. r=bholley,emilio https://hg.mozilla.org/integration/autoland/rev/c69baa105d70 part 2. Reenable various table reftests that were disabled before. They all seem to be passing, or at least failing for unrelated reasons, with the part 1 patch applied. r=bholley,emilio
Comment 30•8 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/abee64d2e5ef followup: check our child hint before trying to append it. r=bholley
Comment 31•8 years ago
|
||
You would also need to remove the corresponding failure patterns here: https://hg.mozilla.org/mozilla-central/file/92a932f9be3f/layout/style/test/stylo-failures.md#l584
Comment 32•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #31) > You would also need to remove the corresponding failure patterns here: > https://hg.mozilla.org/mozilla-central/file/92a932f9be3f/layout/style/test/ > stylo-failures.md#l584 Assuming they turn the job orange as unexpected passes, I'll do this in an hour or two when the results come in after the merge and I update test expectations.
Comment 33•8 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/67ef6204c398 another followup: just simplify the chnagehint thing for now, with a followup filed. r=bholley
Comment 34•8 years ago
|
||
Weird that this doesn't trigger any assertion count mismatch... I'll need to figure out why it doesn't take effect...
Comment 35•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a300522006bc https://hg.mozilla.org/mozilla-central/rev/c69baa105d70 https://hg.mozilla.org/mozilla-central/rev/abee64d2e5ef https://hg.mozilla.org/mozilla-central/rev/67ef6204c398
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•