Bug 230008

<rdar://problem/82975766>

hmm transform animations also affect the <dialog> itself

Created attachment 441256 [details] Patch

Comment on attachment 441256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441256&action=review > Source/WebCore/ChangeLog:3 > + Accelerated animations on ::backdrop shouldn't affect <dialog> (backdrop-animate-002.html fails) Not sure this is the best title for this change. > Source/WebCore/style/Styleable.cpp:55 > + // FIXME: handle ::marker. I think you can implement marker as: ``` if (renderer.style().styleType() == PseudoId::Marker) { ASSERT(renderer.parent()->markerRenderer() == &renderer); return Styleable(renderer.parent()->markerRenderer(), PseudoId::Marker); } ``` ::marker is a child in the render tree of RenderListItem: https://webkit-search.igalia.com/webkit/source/Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp#88

Comment on attachment 441256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441256&action=review >> Source/WebCore/style/Styleable.cpp:55 >> + // FIXME: handle ::marker. > > I think you can implement marker as: > > ``` > if (renderer.style().styleType() == PseudoId::Marker) { > ASSERT(renderer.parent()->markerRenderer() == &renderer); > return Styleable(renderer.parent()->markerRenderer(), PseudoId::Marker); > } > ``` > > ::marker is a child in the render tree of RenderListItem: https://webkit-search.igalia.com/webkit/source/Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp#88 well: return Styleable(renderer.parent()->element(), PseudoId::Marker); I *think*

Comment on attachment 441256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441256&action=review > Source/WebCore/style/Styleable.cpp:54 > +{ One thing that would be nice is to `ASSERT` of expected PseudoId types in this function. So future bugs like this can be caught easily in the future, since the test would just crash and assert at the right place: ``` auto pseudoId = renderer.style().styleType(); ASSERT(pseudoId == PseudoId::None || pseudoId == PseudoId::Before || pseudoId == PseudoId::After || [...], "PseudoId should be handled by Styleable); ```

Comment on attachment 441256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441256&action=review > Source/WebCore/style/Styleable.cpp:86 > + if (pseudoId == PseudoId::After) { > + if (auto* afterPseudoElement = element.afterPseudoElement()) > + return afterPseudoElement->renderer(); > + } else if (pseudoId == PseudoId::Backdrop) { > + if (auto* hostRenderer = element.renderer()) > + return hostRenderer->backdropRenderer().get(); > + } else if (pseudoId == PseudoId::Before) { > + if (auto* beforePseudoElement = element.beforePseudoElement()) > + return beforePseudoElement->renderer(); > + } else if (pseudoId == PseudoId::Marker) { > + if (is<RenderListItem>(element.renderer())) > + return downcast<RenderListItem>(*element.renderer()).markerRenderer(); > + } else if (pseudoId == PseudoId::None) > + return element.renderer(); Switch

(In reply to Simon Fraser (smfr) from comment #7) > Comment on attachment 441256 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441256&action=review > > > Source/WebCore/style/Styleable.cpp:86 > > + if (pseudoId == PseudoId::After) { > > + if (auto* afterPseudoElement = element.afterPseudoElement()) > > + return afterPseudoElement->renderer(); > > + } else if (pseudoId == PseudoId::Backdrop) { > > + if (auto* hostRenderer = element.renderer()) > > + return hostRenderer->backdropRenderer().get(); > > + } else if (pseudoId == PseudoId::Before) { > > + if (auto* beforePseudoElement = element.beforePseudoElement()) > > + return beforePseudoElement->renderer(); > > + } else if (pseudoId == PseudoId::Marker) { > > + if (is<RenderListItem>(element.renderer())) > > + return downcast<RenderListItem>(*element.renderer()).markerRenderer(); > > + } else if (pseudoId == PseudoId::None) > > + return element.renderer(); > > Switch Use ASSERT_NOT_REACHED(); in unhandled cases too :)

Created attachment 441362 [details] Patch for landing

(In reply to Tim Nguyen (:ntim) from comment #6) > Comment on attachment 441256 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441256&action=review > > > Source/WebCore/style/Styleable.cpp:54 > > +{ > > One thing that would be nice is to `ASSERT` of expected PseudoId types in > this function. > > So future bugs like this can be caught easily in the future, since the test > would just crash and assert at the right place: > > ``` > auto pseudoId = renderer.style().styleType(); > ASSERT(pseudoId == PseudoId::None || pseudoId == PseudoId::Before || > pseudoId == PseudoId::After || [...], "PseudoId should be handled by > Styleable); > ``` In the case of Styleable::renderer(), it currently makes sense to have an ASSERT_NOT_REACHED() call for unhandled pseudo-elements. However, in the case of Styleable::fromRenderer() it doesn't, because that function is called under RenderLayerCompositor::requiresCompositingForAnimation() which may be called for any potentially-composited element, which can be the case for :first-letter for instance.

Created attachment 441374 [details] Patch for landing

(In reply to Antoine Quint from comment #10) > However, in the case of Styleable::fromRenderer() it doesn't, because that > function is called under > RenderLayerCompositor::requiresCompositingForAnimation() which may be called > for any potentially-composited element, which can be the case for > :first-letter for instance. I think we should at least warn, it's fishy to end up in an unhandled case.

Landed in r284313.