Closed Bug 1324661 Opened 7 years ago Closed 7 years ago

stylo: several tests non-fatally assert with "Table wrapper frames cannot have borders or paddings"

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: heycam, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Bug 1324661 part 1. When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.
59 bytes, text/x-review-board-request
bholley
: review+
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.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Part 1 merge on top of autoland, since mozreview won't let me push this becuase it's dumb
79.18 KB, patch
Details | Diff | Splinter Review
Part 2 merged on top of autoland
14.61 KB, 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
This is fixed in the latest mozilla-central -> stylo merge.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
This didn't get fixed after all (see bug 1334938); sorry for the noise.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
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: nobody → bzbarsky
Status: REOPENED → ASSIGNED
Blocks: 1337700
Blocks: 1337703
Blocks: 1292609
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 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 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?
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 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 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 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 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+
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.  ;)
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.
Attachment #8838198 - Attachment is obsolete: true
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
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
(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.
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
Weird that this doesn't trigger any assertion count mismatch... I'll need to figure out why it doesn't take effect...
You need to log in before you can comment on or make changes to this bug.