Bug 195802 – Prefer null namespace 'href' over 'xlink:href' on SVG elements

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

[Masataka Yakura]

https://trac.webkit.org/changeset/234683/webkit added support for null-namespace 'href' attribute on SVG elements; however, it doesn't look to conform the spec when there are both 'href' and 'xlink:href' attributes.

The spec says:

> When the ‘href’ attribute is present in both the XLink namespace and without a namespace, the value of the attribute without a namespace shall be used. The attribute in the XLink namespace shall be ignored.

- https://svgwg.org/svg2-draft/linking.html#XLinkRefAttrs

But https://w3c-test.org/svg/linking/reftests/href-use-element.html , which is the wpt for, falis on STP (77).

[Radar WebKit Bug Importer]

<rdar://problem/48955175>

[Binyamin]

Safari/iOS are the only ones missing this support, even old IE supports it. Details in https://css-tricks.com/on-xlinkhref-being-deprecated-in-svg/

And FYI "xlink:href is deprecated" https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/xlink:href, meaning xlink:href eventually must be dropped.

Support testcase https://codepen.io/laukstein/full/MRjPxW

[Said Abou-Hallawa]

Created attachment 377846 [details]
Patch

[EWS Watchlist]

Comment on attachment 377846 [details]
Patch

Attachment 377846 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12990812

New failing tests:
fast/selectors/any-link-basics-2.html

[EWS Watchlist]

Created attachment 377849 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Said Abou-Hallawa]

Created attachment 377898 [details]
Patch

[Darin Adler]

Comment on attachment 377898 [details]
Patch

Is it correct to have special cases for empty strings? What about a string with a single space in it, for example? I agree that null strings, which are used to represent lack of the attribute, are a special case. But I don’t understand why we would special case empty strings, which indicate the attribute without a value or with a value that is explicitly "".

[Darin Adler]

Comment on attachment 377898 [details]
Patch

Aren’t there other call sites where we call getAttribute on href? I seem to remember one using the multiple-argument getAttribute function.

[Nikolas Zimmermann]

(In reply to Darin Adler from comment #8)
> Comment on attachment 377898 [details]
> Patch
> 
> Aren’t there other call sites where we call getAttribute on href? I seem to
> remember one using the multiple-argument getAttribute function.

Hi Darin, nice to see you again :-)

Oh yes, plenty of call sites:

dom/Element.cpp:        linkAttribute = getAttribute(SVGNames::hrefAttr, XLinkNames::hrefAttr);
dom/VisitedLinkState.cpp:        return &element.getAttribute(SVGNames::hrefAttr, XLinkNames::hrefAttr);
svg/SVGAElement.cpp:        m_storedVisitedLinkHash = computeVisitedLinkHash(document().baseURL(), getAttribute(SVGNames::hrefAttr, XLinkNames::hrefAttr));
svg/SVGAltGlyphElement.cpp:    auto target = targetElementFromIRIString(getAttribute(SVGNames::hrefAttr, XLinkNames::hrefAttr), document());
svg/SVGFontFaceUriElement.cpp:    auto src = CSSFontFaceSrcValue::create(getAttribute(SVGNames::hrefAttr, XLinkNames::hrefAttr), LoadedFromOpaqueSource::No);
svg/SVGFontFaceUriElement.cpp:    const AtomString& href = getAttribute(SVGNames::hrefAttr, XLinkNames::hrefAttr);
svg/SVGGlyphRefElement.cpp:    auto target = targetElementFromIRIString(getAttribute(SVGNames::hrefAttr, XLinkNames::hrefAttr), document());
svg/SVGImageElement.cpp:    return getAttribute(SVGNames::hrefAttr, XLinkNames::hrefAttr);
svg/animation/SVGSMILElement.cpp:    auto& href = getAttribute(SVGNames::hrefAttr, XLinkNames::hrefAttr);

[Nikolas Zimmermann]

(In reply to Darin Adler from comment #7)
> Comment on attachment 377898 [details]
> Patch
> 
> Is it correct to have special cases for empty strings? What about a string
> with a single space in it, for example? I agree that null strings, which are
> used to represent lack of the attribute, are a special case. But I don’t
> understand why we would special case empty strings, which indicate the
> attribute without a value or with a value that is explicitly "".

I agree, this seems awkward.

Currently both SVGNames::hrefAttr and XLinkNames::hrefAttr are mapped to the same SVGURIReference::m_href object:

        PropertyRegistry::registerProperty<SVGNames::hrefAttr, &SVGURIReference::m_href>();
        PropertyRegistry::registerProperty<XLinkNames::hrefAttr, &SVGURIReference::m_href>();

In an ideal world, we would split this into "m_href" and "m_xlinkHref" and indicate the attribute presence as non-null (opposed to non-empty) strings.
Then we could implement the desired SVG2 behaviour right in a custom href() accessor that either returns m_href or m_xlinkHref...

[youenn fablet]

Comment on attachment 377898 [details]
Patch

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

> Source/WebCore/svg/SVGURIReference.cpp:50
> +    SVGElement* contextElement = m_href->contextElement();

auto*

> Source/WebCore/svg/SVGURIReference.cpp:52
> +        String xLinkHref = contextElement->getAttribute(XLinkNames::hrefAttr);

auto& might be more efficient if possible.

> Source/WebCore/svg/SVGURIReference.cpp:53
> +        if (value.isEmpty() && !xLinkHref.isEmpty())

Should it be isEmpty or isNull?
Looking at https://svgwg.org/svg2-draft/linking.html#XLinkRefAttrs, the spec mostly talks about attribute presence, which I interpret as an isNull check.
If so, we could simplify the check here to write:
if (name.matches(SVGNames::hrefAttr)) {
    m_href->setBaseValInternal(value);
    return;
}

> Source/WebCore/svg/SVGURIReference.cpp:56
> +            m_href->setBaseValInternal(value);

We could return early here since the if XLinkNames::hrefAttr check should always be false if name is SVGNames::hrefAttr.

> Source/WebCore/svg/SVGURIReference.cpp:60
> +        String href = contextElement->getAttribute(SVGNames::hrefAttr);

auto& might be more efficient if possible.

> Source/WebCore/svg/SVGURIReference.cpp:62
> +            m_href->setBaseValInternal(value);

Should it be isNull?

> LayoutTests/svg/custom/href-xlink-href-gradient-element.svg:12
> +        <linearGradient id="gradient4" xlink:href="#red-gradient" href="#green-gradient"/>

Nice tests!
Can we also add tests for href/xlink:href attributes with empty values, and maybe check what other browsers are doing in that specific case?

[Said Abou-Hallawa]

Created attachment 378144 [details]
Patch

[Said Abou-Hallawa]

(In reply to Darin Adler from comment #7)
> Comment on attachment 377898 [details]
> Patch
> 
> Is it correct to have special cases for empty strings? What about a string
> with a single space in it, for example? I agree that null strings, which are
> used to represent lack of the attribute, are a special case. But I don’t
> understand why we would special case empty strings, which indicate the
> attribute without a value or with a value that is explicitly "".

You are right. I changed the code to check for the existence of the attribute, i.e. checking the string isNull().

[Said Abou-Hallawa]

(In reply to Nikolas Zimmermann from comment #10)
> (In reply to Darin Adler from comment #7)
> > Comment on attachment 377898 [details]
> > Patch
> > 
> > Is it correct to have special cases for empty strings? What about a string
> > with a single space in it, for example? I agree that null strings, which are
> > used to represent lack of the attribute, are a special case. But I don’t
> > understand why we would special case empty strings, which indicate the
> > attribute without a value or with a value that is explicitly "".
> 
> I agree, this seems awkward.
> 
> Currently both SVGNames::hrefAttr and XLinkNames::hrefAttr are mapped to the
> same SVGURIReference::m_href object:
> 
>         PropertyRegistry::registerProperty<SVGNames::hrefAttr,
> &SVGURIReference::m_href>();
>         PropertyRegistry::registerProperty<XLinkNames::hrefAttr,
> &SVGURIReference::m_href>();
> 
> In an ideal world, we would split this into "m_href" and "m_xlinkHref" and
> indicate the attribute presence as non-null (opposed to non-empty) strings.
> Then we could implement the desired SVG2 behaviour right in a custom href()
> accessor that either returns m_href or m_xlinkHref...

I do not think this solution will work well with the href property. Consider this example:

<use id="use" xlink:href="#red-rect"/>
<script>
    var use = document.getElementById("use");

    // href1 will be linked to m_xlinkHref whose baseVal is equal to "#red-rect"
    var href1 = use.href;

    use.setAttribute("href", "#green-rect");

    // href2 will be linked to m_href which is equal to "#green-rect"
    var href2 = use.href;
    assert_false(href1.baseVal != href2.baseVal);
</script>

So in this example, although href1 and href2 are initialized with the same value "use.href", they are referencing two different SVGAnimatedStrings. This is because the custom SVGURIReference::href() will decide to return m_xlinkHref for href1 but will return m_href for href2.

[youenn fablet]

Comment on attachment 378144 [details]
Patch

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

> Source/WebCore/svg/SVGURIReference.cpp:55
> +            m_href->setBaseValInternal(value);

We could write this if/else it as a ternary maybe.

> Source/WebCore/svg/SVGURIReference.cpp:58
> +    if (name.matches(XLinkNames::hrefAttr)) {

We should either use 'else if' or return inside the next if check.

> Source/WebCore/svg/SVGURIReference.cpp:59
> +        if (contextElement->getAttribute(SVGNames::hrefAttr).isNull())

Can we use contextElement->hasAttribute(SVGNames::hrefAttr)?

[Said Abou-Hallawa]

Comment on attachment 377898 [details]
Patch

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

>> Source/WebCore/svg/SVGURIReference.cpp:50
>> +    SVGElement* contextElement = m_href->contextElement();
> 
> auto*

Done.

>> Source/WebCore/svg/SVGURIReference.cpp:52
>> +        String xLinkHref = contextElement->getAttribute(XLinkNames::hrefAttr);
> 
> auto& might be more efficient if possible.

I changed the if-statement below such that it does not need to check xLinkHref.isEmpty(). So contextElement->getAttribute(...) was inlined inside setBaseValInternal().

>> Source/WebCore/svg/SVGURIReference.cpp:53
>> +        if (value.isEmpty() && !xLinkHref.isEmpty())
> 
> Should it be isEmpty or isNull?
> Looking at https://svgwg.org/svg2-draft/linking.html#XLinkRefAttrs, the spec mostly talks about attribute presence, which I interpret as an isNull check.
> If so, we could simplify the check here to write:
> if (name.matches(SVGNames::hrefAttr)) {
>     m_href->setBaseValInternal(value);
>     return;
> }

I do not think this suggestion is correct. We need to set m_href to the Xlink::hrefAttr if value.isNull(). Otherwise a case like this will fail

<use id="use8" xlink:href="#green-rect" href="#red-rect"/>
<script>
    var use8 = document.getElementById("use8");
    // Before this statement, m_herf has the value of the href attribute which is "#red-rect"
    // After this statement, m_herf has to be set to the value of Xlink::href which is "#green-rect"
    use8.removeAttribute("href");
</script>

>> Source/WebCore/svg/SVGURIReference.cpp:56
>> +            m_href->setBaseValInternal(value);
> 
> We could return early here since the if XLinkNames::hrefAttr check should always be false if name is SVGNames::hrefAttr.

Done.

>> Source/WebCore/svg/SVGURIReference.cpp:62
>> +            m_href->setBaseValInternal(value);
> 
> Should it be isNull?

Done.

>> LayoutTests/svg/custom/href-xlink-href-gradient-element.svg:12
>> +        <linearGradient id="gradient4" xlink:href="#red-gradient" href="#green-gradient"/>
> 
> Nice tests!
> Can we also add tests for href/xlink:href attributes with empty values, and maybe check what other browsers are doing in that specific case?

Done. ast/selectors/any-link-basics-2.html covers many error cases for both href and Xlink::href.

[Nikolas Zimmermann]

(In reply to Said Abou-Hallawa from comment #14)
> So in this example, although href1 and href2 are initialized with the same
> value "use.href", they are referencing two different SVGAnimatedStrings.
> This is because the custom SVGURIReference::href() will decide to return
> m_xlinkHref for href1 but will return m_href for href2.

Fully correct, "in an ideal world" was meant to read: it is not possible due to some constraints (which you correctly pointed out).

The current solution - last patch - is just fine!

[Darin Adler]

(In reply to Said Abou-Hallawa from comment #16)
> I do not think this suggestion is correct. We need to set m_href to the
> Xlink::hrefAttr if value.isNull(). Otherwise a case like this will fail
> 
> <use id="use8" xlink:href="#green-rect" href="#red-rect"/>
> <script>
>     var use8 = document.getElementById("use8");
>     // Before this statement, m_herf has the value of the href attribute
> which is "#red-rect"
>     // After this statement, m_herf has to be set to the value of
> Xlink::href which is "#green-rect"
>     use8.removeAttribute("href");
> </script>

Do we have a test like this?

[Said Abou-Hallawa]

Created attachment 378195 [details]
Patch

[Said Abou-Hallawa]

(In reply to Darin Adler from comment #18)
> (In reply to Said Abou-Hallawa from comment #16)
> > I do not think this suggestion is correct. We need to set m_href to the
> > Xlink::hrefAttr if value.isNull(). Otherwise a case like this will fail
> > 
> > <use id="use8" xlink:href="#green-rect" href="#red-rect"/>
> > <script>
> >     var use8 = document.getElementById("use8");
> >     // Before this statement, m_herf has the value of the href attribute
> > which is "#red-rect"
> >     // After this statement, m_herf has to be set to the value of
> > Xlink::href which is "#green-rect"
> >     use8.removeAttribute("href");
> > </script>
> 
> Do we have a test like this?

Yes. I copied it from the new test svg/custom/href-xlink-href-use-element.svg. And I confirmed this test case fails in WebKit without this patch while it passes in Chrome and FireFox.

[WebKit Commit Bot]

Comment on attachment 378195 [details]
Patch

Rejecting attachment 378195 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 378195, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13006551

[Said Abou-Hallawa]

Created attachment 378220 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 378220 [details]
Patch

Clearing flags on attachment: 378220

Committed r249593: <https://trac.webkit.org/changeset/249593>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 378220 [details]
Patch

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

I really like the final version of this patch. The idiom seems good!

> Source/WebCore/svg/SVGURIReference.cpp:50
> +    auto* contextElement = m_href->contextElement();

What guarantees that contextElement is non-null? And if it’s guaranteed to be non-null, why is this a pointer rather than a reference?

[Said Abou-Hallawa]

Comment on attachment 378220 [details]
Patch

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

>> Source/WebCore/svg/SVGURIReference.cpp:50
>> +    auto* contextElement = m_href->contextElement();
> 
> What guarantees that contextElement is non-null? And if it’s guaranteed to be non-null, why is this a pointer rather than a reference?

Yes it is guaranteed that contextElement is not null. In fact m_href->contextElement() is equal to 'this'. SVGURIReference has to be created as a base class for a concrete SVGElement like SVGImageElement. When the concrete SVGElement is created it passes 'this' as the contextElement to SVGURIReference. SVGURIReference passes the contextElement to its m_href. m_href keeps this back pointer as long as the contextElement lives. 

The purpose of having contextElement owned by m_href is to give it the ability to synchronize its new baseVal to the contextElement.

<script>
    var use = document.getElementById("use");
    var href = use.href;
    href.baseVal = "http://example.com";
    console.log(use.getAttribute("href")); // This should show "http://example.com".
    use.remove(); // href should exist after removing its contextElement but no attribute synchronization will be done from now on.
</script>

-- Why doesn't m_href->contextElement() return SVGElement& instead of SVGElement*? Because the m_href can be detached if the contextElement is deleted. In this case m_href->m_contextElement has to be set to null.
-- Why doesn't SVGURIReference retains a SVGElement& instead of getting it through the contextElement() method of any property it owns? It was done this way to reduce the memory size of SVGURIReference by the size of a raw pointer.
-- Can't SVGURIReference use downcast<SVGElement>(this) to get the contextElement? No it can't because SVGElement is not a superclass of SVGURIReference?
-- Can't we ASSERT(this == m_href->contextElement())? No we can't because the compiler does not send the pointer of the concrete class, e.g. SVGImageElement, as the value of 'this' when SVGURIReference::parseAttribute() is called. It sends a shifted pointer such that it points to the beginning of the data block of SVGURIReference. So although the debugger may show 'this' and m_href->contextElement() have the same value, the expression 'this == m_href->contextElement()' will be devalued to false.