Bug 218792

<rdar://problem/71274485>

Created attachment 413798 [details] Patch

<rdar://problem/71274485>

Created attachment 413804 [details] Patch

Created attachment 413942 [details] Patch

To whomever ends up reviewing this: this patch makes http://wpt.live/css/css-pseudo/marker-font-variant-numeric-default.html regress since relying on ::before::marker and ::after::marker in the user-agent stylesheet seems to no longer apply `font-variant-numeric: tabular-nums` whereas it would when that property was hard-coded in RenderListItem. However, not making that change would cause some other WPT failures with `font-variant-numeric` not being able to be set to `reset` for `::marker` since it would not have a user-agent-defined value to revert to and it would use `normal`. I think we mark this failure as expected and file a dedicated bug to deal with ::before::marker and ::after:marker.

Created attachment 414007 [details] Patch

Created attachment 414041 [details] Patch

Comment on attachment 414041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414041&action=review > Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:302 > -void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::ElementUpdate& update) > +void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::ElementUpdates* updates) > { > + ASSERT(updates); Why is this now taking a pointer if it can't be null? Please switch back to a reference. > Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:110 > +void RenderTreeUpdater::GeneratedContent::updatePseudoElement(Element& current, const Style::ElementUpdates* updates, PseudoId pseudoId) Why is this taking a ElementUpdates pointer? The call sites null test so it should be a reference. > Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:117 > + auto* update = [updates, pseudoId]() -> const Style::ElementUpdate* { could just use [&] > Source/WebCore/style/StyleTreeResolver.cpp:251 > - auto beforeUpdate = resolvePseudoStyle(element, update, PseudoId::Before); > - auto afterUpdate = resolvePseudoStyle(element, update, PseudoId::After); > + PseudoIdToElementUpdateMap pseudoUpdates; > + for (PseudoId pseudoId = PseudoId::FirstPublicPseudoId; pseudoId < PseudoId::FirstInternalPseudoId; pseudoId = static_cast<PseudoId>(static_cast<unsigned>(pseudoId) + 1)) { > + if (auto elementUpdate = resolvePseudoStyle(element, update, pseudoId)) > + pseudoUpdates.set(pseudoId, WTFMove(*elementUpdate)); > + } At least::marker (anything else?) matches every element (since it is in the UA sheet) but only those with display:list-item care about it. It seems wasteful (and potential a perf regression) to resolve it eagerly in all cases. Maybe there should be a filter here that tests if a given pseudo element is relevant before resolving? > Source/WebCore/style/StyleTreeResolver.cpp:269 > - return { }; > + return WTF::nullopt; { } would have still worked in these

(In reply to Antti Koivisto from comment #9) > Comment on attachment 414041 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=414041&action=review > > > Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:302 > > -void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::ElementUpdate& update) > > +void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::ElementUpdates* updates) > > { > > + ASSERT(updates); > > Why is this now taking a pointer if it can't be null? Please switch back to > a reference. Will fix in new patch. > > Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:110 > > +void RenderTreeUpdater::GeneratedContent::updatePseudoElement(Element& current, const Style::ElementUpdates* updates, PseudoId pseudoId) > > Why is this taking a ElementUpdates pointer? The call sites null test so it > should be a reference. Will fix in new patch. > > Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:117 > > + auto* update = [updates, pseudoId]() -> const Style::ElementUpdate* { > > could just use [&] Indeed, will fix in new patch. > > Source/WebCore/style/StyleTreeResolver.cpp:251 > > - auto beforeUpdate = resolvePseudoStyle(element, update, PseudoId::Before); > > - auto afterUpdate = resolvePseudoStyle(element, update, PseudoId::After); > > + PseudoIdToElementUpdateMap pseudoUpdates; > > + for (PseudoId pseudoId = PseudoId::FirstPublicPseudoId; pseudoId < PseudoId::FirstInternalPseudoId; pseudoId = static_cast<PseudoId>(static_cast<unsigned>(pseudoId) + 1)) { > > + if (auto elementUpdate = resolvePseudoStyle(element, update, pseudoId)) > > + pseudoUpdates.set(pseudoId, WTFMove(*elementUpdate)); > > + } > > At least::marker (anything else?) matches every element (since it is in the > UA sheet) but only those with display:list-item care about it. It seems > wasteful (and potential a perf regression) to resolve it eagerly in all > cases. Maybe there should be a filter here that tests if a given pseudo > element is relevant before resolving? resolvePseudoStyle() bails if !elementUpdate.style->hasPseudoStyle(pseudoId) but we can move this up to the call site. > > Source/WebCore/style/StyleTreeResolver.cpp:269 > > - return { }; > > + return WTF::nullopt; > > { } would have still worked in these Oh, cool! Will move back to { } to make a smaller diff.

Created attachment 414046 [details] Patch

> resolvePseudoStyle() bails if !elementUpdate.style->hasPseudoStyle(pseudoId) > but we can move this up to the call site. That won't help since hasPseudoStyle(marker) is true for every element.

Created attachment 414049 [details] Patch

(In reply to Antti Koivisto from comment #12) > > resolvePseudoStyle() bails if !elementUpdate.style->hasPseudoStyle(pseudoId) > > but we can move this up to the call site. > > That won't help since hasPseudoStyle(marker) is true for every element. Right! I added a check for `::marker` to check that `display: list-item` is set for the element.

Comment on attachment 414049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414049&action=review > Source/WebCore/style/StyleTreeResolver.cpp:273 > + if (pseudoId == PseudoId::Marker && elementUpdate.style->display() != DisplayType::ListItem) > + return { }; > if (elementUpdate.style->display() == DisplayType::None) > return { }; > if (!elementUpdate.style->hasPseudoStyle(pseudoId)) this stuff could be factored into a function (shouldResolvePseudoElementStyle or similar).

Comment on attachment 414049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414049&action=review > Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:117 > + auto* update = [&]() -> const Style::ElementUpdate* { This can probably be just [&] {

(In reply to Antti Koivisto from comment #16) > Comment on attachment 414049 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=414049&action=review > > > Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:117 > > + auto* update = [&]() -> const Style::ElementUpdate* { > > This can probably be just [&] { The compiler wants an explicit return type.

Created attachment 414070 [details] Patch

Created attachment 414085 [details] Patch

Committed r269813: <https://trac.webkit.org/changeset/269813>

This appears to have caused bug 220652.