Closed
Bug 1343771
Opened 7 years ago
Closed 7 years ago
stylo: Dynamic changes of various properties on table cells don't work right
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
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 hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 3•7 years ago
|
||
> 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...
Comment 4•7 years ago
|
||
(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.
Comment 5•7 years ago
|
||
(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 | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Assignee | ||
Comment 6•7 years ago
|
||
> 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 hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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.
Comment 9•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
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
Comment 13•7 years ago
|
||
(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)
Assignee | ||
Comment 14•7 years ago
|
||
> 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)
Comment 15•7 years ago
|
||
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
Assignee | ||
Comment 16•7 years ago
|
||
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.
Comment 17•7 years ago
|
||
Yeah, I'm fine with the current setup. Thanks for the analysis. :-)
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a1a760aee967 https://hg.mozilla.org/mozilla-central/rev/28ac9c36b726
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•