Closed Bug 1343771 Opened 3 years ago Closed 3 years ago

stylo: Dynamic changes of various properties on table cells don't work right

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

Because we're not recomputing the style on the ::-moz-cell-content anon box.
Comment on attachment 8842897 [details]
Bug 1343771.  Fix stylo to properly update styles on the anonymous block inside a table cell.

https://reviewboard.mozilla.org/r/116634/#review118498

r=me, though I'd like your opinion on the following comment.

::: layout/generic/nsFrame.cpp:10067
(Diff revision 1)
> +
> +  // Figure out whether we have an actual change.  It's important that we do
> +  // this, even though all the child's changes are due to properties it inherits
> +  // from us, because it's possible that no one ever asked us for those style
> +  // structs and hence changes to them aren't reflected in aHintForThisFrame at
> +  // all.

Perhaps the alternative here is to do what we already do for text when requesting inherited structs[1], that is, just get them from the parent style context.

Not sure that'd be a huge win, though we wouldn't need the change hint, nor the change list, which simplifies a bit this API. That being said, we'd have to special-case the table wrapper frame again I guess... May be worth, your choice.

[1]: http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/layout/style/nsStyleContext.h#670
Attachment #8842897 - Flags: review?(emilio+bugs) → review+
> Perhaps the alternative here is to do what we already do for text when requesting
> inherited structs[1], that is, just get them from the parent style context.

No, that won't work right.  Unlike the textnode case, we _do_ have rules applying to the anonymous boxes that come through here.  So we can't just grab our parent's structs.

What the comment is saying is that the rules that match us don't change (well... unless extensions change them, I guess, so maybe I should reword the comment altogether).  But our style _can_ still change during a restyle of properties we inherit from our parent.  And that change may not be handled by changehints for the parent.

Anyway, given that I just realized that extensions can add/remove stylesheets that affect these anon boxes, I will just reword the comment to make it clear that we really have to recompute style correctly here...
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3)
> > Perhaps the alternative here is to do what we already do for text when requesting
> > inherited structs[1], that is, just get them from the parent style context.
> 
> No, that won't work right.  Unlike the textnode case, we _do_ have rules
> applying to the anonymous boxes that come through here.  So we can't just
> grab our parent's structs.

Ouch, you're completely right, for a moment I thought that we only did set reset stuff on these, but that's of course dumb. Sorry for the noise.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3)
> > Perhaps the alternative here is to do what we already do for text when requesting
> > inherited structs[1], that is, just get them from the parent style context.
> 
> No, that won't work right.  Unlike the textnode case, we _do_ have rules
> applying to the anonymous boxes that come through here.  So we can't just
> grab our parent's structs.

But couldn't we at least just poke them, (for inherited structs), so that the parent change hint handling would be correct?
Assignee: nobody → bzbarsky
> But couldn't we at least just poke them, (for inherited structs)

I'm not sure what you mean.  Poke them from where?
Flags: needinfo?(bobbyholley)
Comment on attachment 8842897 [details]
Bug 1343771.  Fix stylo to properly update styles on the anonymous block inside a table cell.

https://reviewboard.mozilla.org/r/116634/#review118498

> Perhaps the alternative here is to do what we already do for text when requesting inherited structs[1], that is, just get them from the parent style context.
> 
> Not sure that'd be a huge win, though we wouldn't need the change hint, nor the change list, which simplifies a bit this API. That being said, we'd have to special-case the table wrapper frame again I guess... May be worth, your choice.
> 
> [1]: http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/layout/style/nsStyleContext.h#670

The struct on the anon box doesn't have to match the one on the parent, because anon boxes can have rules applying to them.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7f79aa355e24 -d 53a53f671871: rebasing 379465:7f79aa355e24 "Bug 1343771.  Fix stylo to properly update styles on the anonymous block inside a table cell.  r=emilio" (tip)
merging layout/generic/nsFrame.cpp
merging layout/generic/nsFrame.h
merging layout/tables/nsTableCellFrame.cpp
merging layout/tables/nsTableCellFrame.h
warning: conflicts while merging layout/generic/nsFrame.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7f79aa355e24 -d 53a53f671871: rebasing 379467:7f79aa355e24 "Bug 1343771.  Fix stylo to properly update styles on the anonymous block inside a table cell.  r=emilio" (tip)
merging layout/generic/nsFrame.cpp
merging layout/generic/nsFrame.h
merging layout/tables/nsTableCellFrame.cpp
merging layout/tables/nsTableCellFrame.h
warning: conflicts while merging layout/generic/nsFrame.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1a760aee967
Fix stylo to properly update styles on the anonymous block inside a table cell.  r=emilio
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #6)
> > But couldn't we at least just poke them, (for inherited structs)
> 
> I'm not sure what you mean.  Poke them from where?

My suggestion is that the accessors for inherited style structs on anonymous box style contexts could mark the same style struct as accessed on the parent. This would avoid the need for a separate CalcStyleDifference call, at the cost of making the parent's CalcStyleDifference take slightly longer (since it might have to compare more actual structs rather than skipping them), and generating a possibly-wider change hint on the parent (rather than restricting change hints needed for the anonymous box to the box itself). But it still seems to me like it might be a net win. WDYT?
Flags: needinfo?(bobbyholley) → needinfo?(bzbarsky)
> This would avoid the need for a separate CalcStyleDifference call,

Oh. I forgot to update the comments as discussed above.  :(  I'll push a fixup changeset for that.

There are several problems with that approach.

1)  Extensions can dynamically add/remove stylesheets that can change the styles of anonymous boxes.  That doesn't change the parent at all, but does change the anon box.  Making this actually work probably requires more work too, because right now we wouldn't even reach the parent to restyle the anon box, because we think nothing is dirty.  :(

2)  Some change hints don't affect children.  If a hint like that happens for the parent, and the anon box inherits the relevant properties, we need to append a change for the same hint for the anon box.  In practice we could perhaps append the parent's hint for the anon box too in all cases.

3)  It's not clear to me that an anon box can't end with a _bigger_ changehint than the parent, even purely through inheritance, because properties in CSS can affect each other's values.  So say we have some property X such that setting X to some values forces display blockification (we have several props like this) but changes in the value of X don't force reframing per se (except via its possible effect on display).  If the parent has "display: block; X: blockifying-value" and the anon box has "display: inline; X: inherit" and then the value of X changes to a non-blockifying value, what should happen?  This might be a bit hypothetical, but I'm already slightly unhappy with the fragility of this code; I don't want to leave landmines in it if they can be avoided.
Flags: needinfo?(bzbarsky)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28ac9c36b726
Fix up a comment that I forgot to fix when addressing review feedback for bug 1343771. r=me, DONTBUILD
Oh, and I guess we could declare that #3 is pretty theoretical, #2 is addressable via using the same changehint for both, and #1 is not supported.  It's still not obvious to me that adding a branch and extra icache pressure in the style struct getter is a win over doing a small bit more work during restyling...  We'd want to measure.
Yeah, I'm fine with the current setup. Thanks for the analysis. :-)
https://hg.mozilla.org/mozilla-central/rev/a1a760aee967
https://hg.mozilla.org/mozilla-central/rev/28ac9c36b726
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.