Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REGRESSION(r258464): SVG use element doesn't render if it references a subsequent element after a style resolution #13857

Merged
merged 1 commit into from May 15, 2023

Conversation

rniwa
Copy link
Member

@rniwa rniwa commented May 13, 2023

a6a7aa5

REGRESSION(r258464): SVG use element doesn't render if it references a subsequent element after a style resolution
https://bugs.webkit.org/show_bug.cgi?id=216363

Reviewed by Said Abou-Hallawa.

The bug was caused by SVGPathElement::insertedIntoAncestor and a few other insertedIntoAncestor functions
ignoring the value returned by SVGElement::insertedIntoAncestor requesting a post insertion callback,
and SVGMPathElement::didFinishInsertingNode not calling SVGMPathElement::didFinishInsertingNode,
which updates SVG elements referencing the element.

This PR fixes all insertedIntoAncestor functions in SVG to respect the value returned by super class'
insertedIntoAncestor but many of them don't have any consequences at the moment.

* LayoutTests/svg/dom/use-element-refers-subsequent-image-element-after-style-update-expected.svg: Added.
* LayoutTests/svg/dom/use-element-refers-subsequent-image-element-after-style-update.svg: Added.
* LayoutTests/svg/dom/use-element-refers-subsequent-path-element-after-style-update-expected.svg: Added.
* LayoutTests/svg/dom/use-element-refers-subsequent-path-element-after-style-update.svg: Added.
* Source/WebCore/svg/SVGFEImageElement.cpp:
(WebCore::SVGFEImageElement::didFinishInsertingNode):
* Source/WebCore/svg/SVGFontFaceElement.cpp:
(WebCore::SVGFontFaceElement::insertedIntoAncestor):
* Source/WebCore/svg/SVGImageElement.cpp:
(WebCore::SVGImageElement::insertedIntoAncestor):
* Source/WebCore/svg/SVGMPathElement.cpp:
(WebCore::SVGMPathElement::insertedIntoAncestor):
(WebCore::SVGMPathElement::didFinishInsertingNode):
* Source/WebCore/svg/SVGPathElement.cpp:
(WebCore::SVGPathElement::insertedIntoAncestor):
* Source/WebCore/svg/SVGScriptElement.cpp:
(WebCore::SVGScriptElement::insertedIntoAncestor):
* Source/WebCore/svg/SVGTRefElement.cpp:
(WebCore::SVGTRefElement::insertedIntoAncestor):
(WebCore::SVGTRefElement::didFinishInsertingNode):
* Source/WebCore/svg/SVGTextPathElement.cpp:
(WebCore::SVGTextPathElement::didFinishInsertingNode):
* Source/WebCore/svg/SVGTitleElement.cpp:
(WebCore::SVGTitleElement::insertedIntoAncestor):
* Source/WebCore/svg/SVGUseElement.cpp:
(WebCore::SVGUseElement::insertedIntoAncestor):
(WebCore::SVGUseElement::didFinishInsertingNode):
* Source/WebCore/svg/animation/SVGSMILElement.cpp:
(WebCore::SVGSMILElement::didFinishInsertingNode):

Canonical link: https://commits.webkit.org/264085@main

424289b

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings   πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
❌ πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1   πŸ§ͺ gtk-wk2
  πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim
βœ… πŸ›  watch
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim

@rniwa rniwa requested a review from cdumez as a code owner May 13, 2023 20:34
@rniwa rniwa self-assigned this May 13, 2023
@rniwa rniwa added the SVG For bugs in the SVG implementation. label May 13, 2023
@rniwa rniwa requested a review from shallawa May 13, 2023 20:37
Comment on lines 252 to 254
if (insertionType.connectedToDocument || result == InsertedIntoAncestorResult::NeedsPostInsertionCallback)
return InsertedIntoAncestorResult::NeedsPostInsertionCallback;
return InsertedIntoAncestorResult::Done;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or this can be something like this:

    if (insertionType.connectedToDocument)
        return InsertedIntoAncestorResult::NeedsPostInsertionCallback;
    return result;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Fixed.

Comment on lines 1 to 3
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these lines can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, removed.

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg viewBox="0 0 100 100" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name space xmlns:xlink is not needed anymore. So it can be removed. And the version attribute has been deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. Removed.

<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg viewBox="0 0 100 100" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<use xlink:href="#x1" x="0" y="0" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xlink:href -> href

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 424289b. Live statuses available at the PR page, #13857

@rniwa rniwa added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 15, 2023
…a subsequent element after a style resolution

https://bugs.webkit.org/show_bug.cgi?id=216363

Reviewed by Said Abou-Hallawa.

The bug was caused by SVGPathElement::insertedIntoAncestor and a few other insertedIntoAncestor functions
ignoring the value returned by SVGElement::insertedIntoAncestor requesting a post insertion callback,
and SVGMPathElement::didFinishInsertingNode not calling SVGMPathElement::didFinishInsertingNode,
which updates SVG elements referencing the element.

This PR fixes all insertedIntoAncestor functions in SVG to respect the value returned by super class'
insertedIntoAncestor but many of them don't have any consequences at the moment.

* LayoutTests/svg/dom/use-element-refers-subsequent-image-element-after-style-update-expected.svg: Added.
* LayoutTests/svg/dom/use-element-refers-subsequent-image-element-after-style-update.svg: Added.
* LayoutTests/svg/dom/use-element-refers-subsequent-path-element-after-style-update-expected.svg: Added.
* LayoutTests/svg/dom/use-element-refers-subsequent-path-element-after-style-update.svg: Added.
* Source/WebCore/svg/SVGFEImageElement.cpp:
(WebCore::SVGFEImageElement::didFinishInsertingNode):
* Source/WebCore/svg/SVGFontFaceElement.cpp:
(WebCore::SVGFontFaceElement::insertedIntoAncestor):
* Source/WebCore/svg/SVGImageElement.cpp:
(WebCore::SVGImageElement::insertedIntoAncestor):
* Source/WebCore/svg/SVGMPathElement.cpp:
(WebCore::SVGMPathElement::insertedIntoAncestor):
(WebCore::SVGMPathElement::didFinishInsertingNode):
* Source/WebCore/svg/SVGPathElement.cpp:
(WebCore::SVGPathElement::insertedIntoAncestor):
* Source/WebCore/svg/SVGScriptElement.cpp:
(WebCore::SVGScriptElement::insertedIntoAncestor):
* Source/WebCore/svg/SVGTRefElement.cpp:
(WebCore::SVGTRefElement::insertedIntoAncestor):
(WebCore::SVGTRefElement::didFinishInsertingNode):
* Source/WebCore/svg/SVGTextPathElement.cpp:
(WebCore::SVGTextPathElement::didFinishInsertingNode):
* Source/WebCore/svg/SVGTitleElement.cpp:
(WebCore::SVGTitleElement::insertedIntoAncestor):
* Source/WebCore/svg/SVGUseElement.cpp:
(WebCore::SVGUseElement::insertedIntoAncestor):
(WebCore::SVGUseElement::didFinishInsertingNode):
* Source/WebCore/svg/animation/SVGSMILElement.cpp:
(WebCore::SVGSMILElement::didFinishInsertingNode):

Canonical link: https://commits.webkit.org/264085@main
@webkit-commit-queue
Copy link
Collaborator

Committed 264085@main (a6a7aa5): https://commits.webkit.org/264085@main

Reviewed commits have been landed. Closing PR #13857 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit a6a7aa5 into WebKit:main May 15, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 15, 2023
@rniwa rniwa deleted the fix216363 branch May 15, 2023 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SVG For bugs in the SVG implementation.
Projects
None yet
4 participants