Created attachment 119901 [details] repro (first one was broken)

Created attachment 120073 [details] patch

Also removes a related comment in SVGSwitchElement which to my eye looks completely false.

Comment on attachment 120073 [details] patch Attachment 120073 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10992386 New failing tests: http/tests/inspector/resource-tree/resource-tree-document-url.html

(In reply to comment #4) > (From update of attachment 120073 [details]) > Attachment 120073 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/10992386 > > New failing tests: > http/tests/inspector/resource-tree/resource-tree-document-url.html That seems exceedingly unlikely.

Comment on attachment 120073 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=120073&action=review Great catches, I still have some questions: > LayoutTests/svg/custom/requiredExtensions-runtime-changes.svg:36 > + var removeElements = document.getElementsByClassName("removeRequiredExtensions"); > + var addElements = document.getElementsByClassName("addRequiredExtensions"); Clever idea, I keep that in mind for further tests of mine :-) Makes the test itself much more readable/hackable. > Source/WebCore/ChangeLog:10 > + SVGStringList, because SVGStringList::reset() adds a single empty string to the list. Good catch - but I can't recall why reset() behaves this way. Can we just change the behavior of reset? > Source/WebCore/ChangeLog:13 > + Invalid elements inside <switch> are prevented from creating renderers, > + but are still attached; SVGTests assumes that invalid elements will always be detached. Why is this? I assumed that attached() returns false, if there's no renderer. I just checked the code, and indeed this assumption is just wrong. We're differentiating between two states in dom/Node.cpp (see attach()): * attach() called no renderer created: attached() returns true and renderer() zero. * attach() called renderer got created: attached() returns true and renderer() non-zero. Does anyone know where exactly the former is needed? When reading common code in eg. ContainerNode, it seems attached() is used to check whether the elements was attached to the rendering tree (which is what I assumed): ... // Add child to the rendering tree. if (attached() && !child->attached() && child->parentNode() == this) { if (shouldLazyAttach) child->lazyAttach(); else ... // Remove from rendering tree if (oldChild->attached()) oldChild->detach(); ... If theres a need to differentiate between those states, I'm fine with your approach, otherwise we should rethink it.

(In reply to comment #6) > (From update of attachment 120073 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120073&action=review > > Great catches, I still have some questions: > > > LayoutTests/svg/custom/requiredExtensions-runtime-changes.svg:36 > > + var removeElements = document.getElementsByClassName("removeRequiredExtensions"); > > + var addElements = document.getElementsByClassName("addRequiredExtensions"); > > Clever idea, I keep that in mind for further tests of mine :-) Makes the test itself much more readable/hackable. > > > Source/WebCore/ChangeLog:10 > > + SVGStringList, because SVGStringList::reset() adds a single empty string to the list. > > Good catch - but I can't recall why reset() behaves this way. Can we just change the behavior of reset? I'd love to! I couldn't come up with a good reason (as it stands, this makes the empty-hasn't-been-touched list and the empty-was-touched-but-got-cleared list different, and I can't see a good reason for that, at all). > > Source/WebCore/ChangeLog:13 > > + Invalid elements inside <switch> are prevented from creating renderers, > > + but are still attached; SVGTests assumes that invalid elements will always be detached. > > Why is this? I assumed that attached() returns false, if there's no renderer. > I just checked the code, and indeed this assumption is just wrong. > > We're differentiating between two states in dom/Node.cpp (see attach()): > * attach() called no renderer created: attached() returns true and renderer() zero. > * attach() called renderer got created: attached() returns true and renderer() non-zero. > > Does anyone know where exactly the former is needed? > > When reading common code in eg. ContainerNode, it seems attached() is used to check whether the elements was attached to the rendering tree (which is what I assumed): > ... > // Add child to the rendering tree. > if (attached() && !child->attached() && child->parentNode() == this) { > if (shouldLazyAttach) > child->lazyAttach(); > else > ... > > // Remove from rendering tree > if (oldChild->attached()) > oldChild->detach(); > ... > > If theres a need to differentiate between those states, I'm fine with your approach, otherwise we should rethink it. I have no idea if there's a reason for this. I assumed there was, and just wanted to make our assumptions fit. Do you know who might know?

Created attachment 212362 [details] testcase Here is another testcase. I can help with fixing that bug, since it is similar to the implementation of maction/semantics I'm currently working on.

So more precisely: - update to <switch> when foreignObject@requiredExtensions changes is similar to update to <semantics> when annotation-xml@encoding changes (bug 100626) - update to <switch> when its children or attributes change is similar to what I have done to update <semantics>/<maction> (bug 120058)

Finally this will be a bit more complicated than the MathML case, since SVG attributes can be animated. So I don't think I'll spend some time on it in the short term.

Still reproduces, in Safari 26.4 and STP 249. Chrome 153 does the same thing, so this is an interop bug rather than WebKit-only. Firefox 155 is the only engine that re-selects. Two things: * systemLanguage is broken the same way, not just requiredExtensions. * Making a passing child fail is worse than stale. Removing a failing attribute leaves the old child selected, but adding one makes the switch render nothing at all. Measured with a WPT test added at svg/struct/scripted/switch-conditional-processing-dynamic.html: 4 subtests pass, 5 fail. All the static rows pass, including requiredExtensions="" selecting the next child, so selection itself is fine and only re-selection is missing. Firefox re-selects on all five mutations. Chrome and Safari both keep the stale child when a failing requiredExtensions or systemLanguage is removed, and both render nothing when one is added or set to the empty string. Removing the switch and re-inserting it gives the correct answer in all three engines, so nothing wrong is stored — only the invalidation on attribute change is missing. Gecko does this in SVGSwitchElement::MaybeInvalidate, reached from the attribute-set path.