Bug 63390 – SVG1.1SE test text-tref-03-b.svg fails

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Rob Buis]

We don't support tref that forward references content yet.

[Rob Buis]

Created attachment 98609 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 98609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98609&action=review

Brilliant idea to listen to subtree modified events! Some comments

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25927
> +				GCC_VERSION = com.apple.compilers.llvmgcc42;

You need to remove that from the patch.

> Source/WebCore/svg/SVGTRefElement.cpp:56
> +        { return adoptRef(new SubtreeModificationEventListener(trefElement)); }

Wrong indentation.

> Source/WebCore/svg/SVGTRefElement.cpp:61
> +        return listener->type() == CPPEventListenerType
> +            ? static_cast<const SubtreeModificationEventListener*>(listener)
> +            : 0;

No need tow rap lines like this.

> Source/WebCore/svg/SVGTRefElement.cpp:64
> +    virtual bool operator==(const EventListener& other);

You can omit the "other".

> Source/WebCore/svg/SVGTRefElement.cpp:81
> +    if (const SubtreeModificationEventListener* imageEventListener = SubtreeModificationEventListener::cast(&listener))
> +        return m_trefElement == imageEventListener->m_trefElement;

imageEvent? Where did you copy that from? ;-)

> Source/WebCore/svg/SVGTRefElement.cpp:215
> +    RefPtr<EventListener> listener = SubtreeModificationEventListener::create(this);

Don't you want to destruct this listener if the SVGTRefElement gets destructed? Otherwhise a dangling pointer is still stored in SubtreeModificationEventListener.

[Rob Buis]

(In reply to comment #2)
> (From update of attachment 98609 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98609&action=review
> 
> Brilliant idea to listen to subtree modified events! Some comments

Thanks!
 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25927
> > +				GCC_VERSION = com.apple.compilers.llvmgcc42;
> 
> You need to remove that from the patch.

Fixed.

> > Source/WebCore/svg/SVGTRefElement.cpp:56
> > +        { return adoptRef(new SubtreeModificationEventListener(trefElement)); }
> 
> Wrong indentation.
Fixed.
 
> > Source/WebCore/svg/SVGTRefElement.cpp:61
> > +        return listener->type() == CPPEventListenerType
> > +            ? static_cast<const SubtreeModificationEventListener*>(listener)
> > +            : 0;
> 
> No need tow rap lines like this.

Fixed.
 
> > Source/WebCore/svg/SVGTRefElement.cpp:64
> > +    virtual bool operator==(const EventListener& other);
> 
> You can omit the "other".

Fixed.
 
> > Source/WebCore/svg/SVGTRefElement.cpp:81
> > +    if (const SubtreeModificationEventListener* imageEventListener = SubtreeModificationEventListener::cast(&listener))
> > +        return m_trefElement == imageEventListener->m_trefElement;
> 
> imageEvent? Where did you copy that from? ;-)

Correct :) I used ImageDocument.cpp as inspiration. Fixed.

> > Source/WebCore/svg/SVGTRefElement.cpp:215
> > +    RefPtr<EventListener> listener = SubtreeModificationEventListener::create(this);
> 
> Don't you want to destruct this listener if the SVGTRefElement gets destructed? Otherwhise a dangling pointer is still stored in SubtreeModificationEventListener.

Right, fixed that now.
But I don't know how to act when the xlink:href changes ; AFAICS we need the "old" target to remove the old event listener, but I think we lost the target element at that point in svgAttributeChanged. Do you see the problem? Only quickfix I see is storing the target element pointer :(

Submitting patch anyway :)
Cheers,

Rob.

[Rob Buis]

Created attachment 98615 [details]
Patch

[Rob Buis]

Created attachment 98626 [details]
Patch

[Rob Buis]

(In reply to comment #5)
> Created an attachment (id=98626) [details]
> Patch

This patch adds a test for changing the href attribute. Also I set the listener now on the target's parent, so we are informed of the target's removal as well. To reflect this I made the removal tests have pixel tests as well, showing the text is really gone after removing tref or referenced content. Finally the event listener creation/deletion management should now be fixed.
Cheers,

Rob.

[WebKit Review Bot]

Comment on attachment 98626 [details]
Patch

Attachment 98626 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8944162

New failing tests:
svg/W3C-SVG-1.1-SE/text-tref-03-b.svg
svg/custom/text-tref-03-b-referenced-element-removal.svg
svg/custom/text-tref-03-b-tref-removal.svg
svg/custom/text-tref-03-b-change-href.svg

[WebKit Review Bot]

Created attachment 98634 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 98626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98626&action=review

Looks much better, still some comments:

> Source/WebCore/svg/SVGTRefElement.cpp:59
> +    }
> +    static const SubtreeModificationEventListener* cast(const EventListener* listener)

Style, add a newline here inbetween.

> Source/WebCore/svg/SVGTRefElement.cpp:68
> +        target->parentNode()->removeEventListener(eventNames().DOMSubtreeModifiedEvent, this, false);

Can you guarantee target is always valid? ASSERT(target) then; or add an early return.
Same for the parentNode. ASSERT(target->parentNode()) or early return.
I guess you want to add assertions.

> Source/WebCore/svg/SVGTRefElement.cpp:136
> +    String id = SVGURIReference::getTarget(href());
> +    Element* oldTarget = treeScope()->getElementById(id);
>      if (SVGURIReference::parseMappedAttribute(attr)) {
> +        id = SVGURIReference::getTarget(href());
> +        Element* target = treeScope()->getElementById(id);
> +        if (!target) {
> +            document()->accessSVGExtensions()->addPendingResource(id, this);
> +            return;
> +        }
> +        if (m_eventListener) {
> +            m_eventListener->removeFromTarget(oldTarget);
> +            m_eventListener = 0;
> +        }
> +        m_eventListener = SubtreeModificationEventListener::create(this);
> +        target->parentNode()->addEventListener(eventNames().DOMSubtreeModifiedEvent, m_eventListener.get(), false);

This logic belongs in "svgAttributeChanged", not in parseMappedAttribute, as you only handle XML DOM changes now.
Remember that you can also change the xlink:href attribute via SVG DOM, tref.href.baseVal = "someRef";

> Source/WebCore/svg/SVGTRefElement.cpp:137
>          updateReferencedText();

This also has to go into svgAttributeChanged. It clearly shows xlink:href changes on <tref> via SVG DOM don't work as expected (never trigger a change) even without your patch in trunk.
Please move it as well, and duplicate your test, but use SVG DOM to modify that href property.

> Source/WebCore/svg/SVGTRefElement.cpp:230
> +    // set up subtree listener

This seems obvious, I think you can omit this.

Any specific reason you have to call updateReferencedText() _first? ? You did it the other way round in parseMappedAttribute.

> Source/WebCore/svg/SVGTRefElement.cpp:239
> +    SVGStyledElement::removedFromDocument();

Is it safe to call into the base class first and then destruct your event listener? Just asking to be sure.

> Source/WebCore/svg/SVGTRefElement.cpp:246
> +    Element* target = treeScope()->getElementById(SVGURIReference::getTarget(href()));
> +    if (target) {

Combine this into one line.

> LayoutTests/svg/custom/text-tref-03-b-change-href.svg:19
> +         tref.setAttributeNS("http://www.w3.org/1999/xlink", "href", "#hello");

You want to duplicate this test using SVG DOM.

[Rob Buis]

Moin Niko,

(In reply to comment #9)
> (From update of attachment 98626 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98626&action=review
> 
> Looks much better, still some comments:
> 
> > Source/WebCore/svg/SVGTRefElement.cpp:59
> > +    }
> > +    static const SubtreeModificationEventListener* cast(const EventListener* listener)
> 
> Style, add a newline here inbetween.

Fixed.

> > Source/WebCore/svg/SVGTRefElement.cpp:68
> > +        target->parentNode()->removeEventListener(eventNames().DOMSubtreeModifiedEvent, this, false);
> 
> Can you guarantee target is always valid? ASSERT(target) then; or add an early return.

Yes, except when the referenced element is deleted ; we get notified of that through the parent, and the event listener delegates to updateReferencedText. This function does check that parentNode() exists, the other places should be safe to assert.

> Same for the parentNode. ASSERT(target->parentNode()) or early return.
> I guess you want to add assertions.

See above.

> > Source/WebCore/svg/SVGTRefElement.cpp:136
> > +    String id = SVGURIReference::getTarget(href());
> > +    Element* oldTarget = treeScope()->getElementById(id);
> >      if (SVGURIReference::parseMappedAttribute(attr)) {
> > +        id = SVGURIReference::getTarget(href());
> > +        Element* target = treeScope()->getElementById(id);
> > +        if (!target) {
> > +            document()->accessSVGExtensions()->addPendingResource(id, this);
> > +            return;
> > +        }
> > +        if (m_eventListener) {
> > +            m_eventListener->removeFromTarget(oldTarget);
> > +            m_eventListener = 0;
> > +        }
> > +        m_eventListener = SubtreeModificationEventListener::create(this);
> > +        target->parentNode()->addEventListener(eventNames().DOMSubtreeModifiedEvent, m_eventListener.get(), false);
> 
> This logic belongs in "svgAttributeChanged", not in parseMappedAttribute, as you only handle XML DOM changes now.
> Remember that you can also change the xlink:href attribute via SVG DOM, tref.href.baseVal = "someRef";

That is probably right. I think there is a bug that makes above line not end up in svgAttributeChanged, whereas for SVGLength I assume it works. I think this is because in the generated JS we do not have binding code for updating the animated properties, maybe because we use DOMString?

> > Source/WebCore/svg/SVGTRefElement.cpp:137
> >          updateReferencedText();
> 
> This also has to go into svgAttributeChanged. It clearly shows xlink:href changes on <tref> via SVG DOM don't work as expected (never trigger a change) even without your patch in trunk.
> Please move it as well, and duplicate your test, but use SVG DOM to modify that href property.

See above.

> > Source/WebCore/svg/SVGTRefElement.cpp:230
> > +    // set up subtree listener
> 
> This seems obvious, I think you can omit this.

Agreed, fixed.

> Any specific reason you have to call updateReferencedText() _first? ? You did it the other way round in parseMappedAttribute.

I don't think it matters, they are quite independent.

> > Source/WebCore/svg/SVGTRefElement.cpp:239
> > +    SVGStyledElement::removedFromDocument();
> 
> Is it safe to call into the base class first and then destruct your event listener? Just asking to be sure.

Yes, again quite independent.

> > Source/WebCore/svg/SVGTRefElement.cpp:246
> > +    Element* target = treeScope()->getElementById(SVGURIReference::getTarget(href()));
> > +    if (target) {
> 
> Combine this into one line.

Fixed.

> > LayoutTests/svg/custom/text-tref-03-b-change-href.svg:19
> > +         tref.setAttributeNS("http://www.w3.org/1999/xlink", "href", "#hello");
> 
> You want to duplicate this test using SVG DOM.

I did, which lead to encountering this new bug :} Let me know what is the best action to follow here, I don't really want to see this bug blocked by a JS bindings code problem IMHO. May be better to file 2 new bugs, one for the JS bindings bug and 1 for fixing/adding the testcase.
Cheers,

Rob.

[Rob Buis]

Created attachment 98762 [details]
Patch

[WebKit Review Bot]

Comment on attachment 98762 [details]
Patch

Attachment 98762 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8945460

New failing tests:
svg/W3C-SVG-1.1-SE/text-tref-03-b.svg
svg/custom/text-tref-03-b-change-href-dom.svg
svg/custom/text-tref-03-b-referenced-element-removal.svg
svg/custom/text-tref-03-b-tref-removal.svg
svg/custom/text-tref-03-b-change-href.svg

[WebKit Review Bot]

Created attachment 98765 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 98762 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98762&action=review

Looks good to me except some issues:

> Source/WebCore/svg/SVGTRefElement.cpp:131
> +        String id = SVGURIReference::getTarget(href());
> +        Element* target = treeScope()->getElementById(id);
> +        if (!target) {
> +            document()->accessSVGExtensions()->addPendingResource(id, this);
> +            return;
> +        }

That shouldn't be needed, as svgAttributeChanged takes care of that already.

> Source/WebCore/svg/SVGTRefElement.cpp:159
> +        String id = SVGURIReference::getTarget(href());
> +        Element* oldTarget = treeScope()->getElementById(id);
> +        if (m_eventListener) {
> +            m_eventListener->removeFromTarget(oldTarget);
> +            m_eventListener = 0;

What happens now: the event listener is created while parsing the <tref> in parseMappedAttribute, right afterwards svgAttributeChanged is called and you're doing the work again.
The code in parseMappedAttribute seems superfluous now, you now handle SVG/XML dom changes both through svgAttributeChanged.

[Rob Buis]

Created attachment 98922 [details]
Patch

[WebKit Review Bot]

Comment on attachment 98922 [details]
Patch

Attachment 98922 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8950761

New failing tests:
svg/W3C-SVG-1.1-SE/text-tref-03-b.svg
svg/custom/text-tref-03-b-change-href-dom.svg
svg/custom/text-tref-03-b-referenced-element-removal.svg
svg/custom/text-tref-03-b-tref-removal.svg
svg/custom/text-tref-03-b-change-href.svg

[WebKit Review Bot]

Created attachment 98927 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 98922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98922&action=review

Looks great, r=me with some comments:

> Source/WebCore/svg/SVGTRefElement.cpp:160
> +        if (renderer())
> +            RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer());

You could use: if (RenderObject* renderer = this->renderer()) to save calling it twice.

> Source/WebCore/svg/SVGTRefElement.cpp:234
> +    if (Element* target = treeScope()->getElementById(id)) {

I'd rather use a early return here.

> Source/WebCore/svg/SVGTRefElement.cpp:249
> +    if (m_eventListener) {

This condition is superfluous.

[Rob Buis]

Committed r89951: <http://trac.webkit.org/changeset/89951>

[Tim Horton]

I believe this caused https://bugs.webkit.org/show_bug.cgi?id=69385.