Closed Bug 1376073 Opened 8 years ago Closed 8 years ago

stylo: We reframe too eagerly for ::before/::after

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

Bug 1376073. Don't reframe when going from no pseudo to pseudo with no content for ::before and ::after.
59 bytes, text/x-review-board-request
emilio
: review+
Details
The layout/base/tests/test_frame_reconstruction_for_pseudo_elements.html mochitest catches this. Simple testcase: <style> .x::before { color: green; } </style> <span onclick="this.className = 'x'">Some text; click me to reframe</span> What happens is that in match_pseudos (matching.rs) we discover we have rules for the pseudo and do: matches_different_pseudos |= !data.styles.pseudos.has(&pseudo); ... if matches_different_pseudos && data.restyle.is_restyle() { // Any changes to the matched pseudo-elements trigger // reconstruction. data.restyle.damage |= RestyleDamage::reconstruct(); which is not needed in this case, because the ::before has "content: none" and hence doesn't actually render in any way. I wonder whether we can just stop reframing here for ::before/::after and rely on compute_style_difference() triggering reframes as needed or something.
Hmm... I guess so, yeah. We only arrive there when there's an old pseudo-style right now, but we can probably massage a little the logic there to make it more general.
Assignee: nobody → bzbarsky
Comment on attachment 8891421 [details] Bug 1376073. Don't reframe when going from no pseudo to pseudo with no content for ::before and ::after. https://reviewboard.mozilla.org/r/162582/#review167988 Mostly semi-opinionated nits, feel free to address as you wish. It'd be nice to extend `layout/style/test/test_reframe_pseudo_element.html` to exercise this case, and shouldn't be too hard. ::: servo/components/style/matching.rs:550 (Diff revision 3) > + // It's possible that we're switching from not having > + // ::before/::after at all to having styles for them but not > + // actually having a useful pseudo-element. Check for that > + // case. > + let pseudo = PseudoElement::from_eager_index(i); > + if old.as_ref().map_or(false, nit: do you think this would benefit from splitting it in two different variables? ``` let new_pseudo_should_exist = ...; let old_pseudo_should_exist = ...; if new_pseudo_should_exist != new_pseudo_should_exist { // ... ``` ::: servo/components/style/matching.rs:802 (Diff revision 3) > > if pseudo.map_or(false, |p| p.is_before_or_after()) { > + // FIXME(bz) This duplicates some of the logic in > + // PseudoElement::should_exist, but it's not clear how best to share > + // that logic without redoing the "get the display" work. > let old_style_generates_no_pseudo = (I think getting the display value again is not a big deal, but your call) ::: servo/components/style/servo/selector_parser.rs:197 (Diff revision 3) > None > } > + > + /// Whether this pseudo-element should actually exist if it has > + /// the given styles. > + pub fn should_exist(&self, nit: This fits in the same line: ``` pub fn should_exist(&self, style: &ComputedValues) -> bool { ``` ::: servo/components/style/servo/selector_parser.rs:201 (Diff revision 3) > + /// the given styles. > + pub fn should_exist(&self, > + style: &ComputedValues) > + { > + let display = style.get_box().clone_display(); > + display != display::T::none && Do you think this would be clearer (even if slightly longer) written like: ``` if display == display::T::none { return false; } if pseudo.is_before_or_after() && style.ineffective_content_property() { return false; } true ``` Seems slightly easier to extend if needed. But not a big deal anyway.
Attachment #8891421 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8891421 [details] Bug 1376073. Don't reframe when going from no pseudo to pseudo with no content for ::before and ::after. https://reviewboard.mozilla.org/r/162582/#review167988 Yeah, I'll give extending that a shot. > nit: do you think this would benefit from splitting it in two different variables? > > ``` > let new_pseudo_should_exist = ...; > let old_pseudo_should_exist = ...; > > if new_pseudo_should_exist != new_pseudo_should_exist { > // ... > ``` Yeah, probably a good idea. Done. > (I think getting the display value again is not a big deal, but your call) I'm really not sure how hot this various code is and how slow the readback from the Gecko struct is... > nit: This fits in the same line: > > ``` > pub fn should_exist(&self, style: &ComputedValues) -> bool { > ``` Done. > Do you think this would be clearer (even if slightly longer) written like: > > ``` > if display == display::T::none { > return false; > } > > if pseudo.is_before_or_after() && style.ineffective_content_property() { > return false; > } > > true > ``` > > Seems slightly easier to extend if needed. But not a big deal anyway. I went back and forth a few times here. I guess it probably is clearer like that, will change it to that.
Comment on attachment 8891421 [details] Bug 1376073. Don't reframe when going from no pseudo to pseudo with no content for ::before and ::after. https://reviewboard.mozilla.org/r/162582/#review167988 Oh, actually, this case is already exercised by layout/base/tests/test_frame_reconstruction_for_pseudo_elements.html which we're failing without this patch. So not going to worry about the test bit.
https://hg.mozilla.org/mozilla-central/rev/a2cb823abab5
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: