Bug 153854

Summary: Allow href attribute without xlink on SVG elements
Product: WebKit Reporter: bbrinza <bbrinza>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Minor CC: cam, cigitia, commit-queue, darin, dino, dvpdiner2, eoconnor, ews, krit, sabouhallawa, shayneosull, simon.fraser, webkit.20.phk, webkit-bug-importer, zach.silversmith, zimmermann
Priority: P2 Keywords: FromImplementor, InRadar
Version: Safari 9   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=188419
https://bugs.webkit.org/show_bug.cgi?id=201227
Bug Depends on:    
Bug Blocks: 191292    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews115 for mac-sierra
none
Archive of layout-test-results from ews113 for mac-sierra
none
Patch
none
Patch
none
Patch none

Description bbrinza@microsoft.com 2016-02-03 20:31:22 PST
SVG Working group has resolved to deprecate xlink and allow href attribute on relevant nodes (https://www.w3.org/2012/09/19-svg-minutes.html#item05), the according spec changes has been made (https://svgwg.org/svg2-draft/attindex.html), and this has been brought to attention today during Sydney face to face meeting - https://www.w3.org/2016/02/03-svg-minutes.html

At this point Edge supports that, Chrome doesn't.

Simple test case: https://jsfiddle.net/pzrez3eb/
Comment 1 bbrinza@microsoft.com 2016-02-03 20:31:58 PST
Copied from https://code.google.com/p/chromium/issues/detail?id=584142
Comment 2 Cameron McCormack 2016-08-31 20:27:14 PDT
FYI, we just landed support for this in Firefox, and Chrome has supported this since February this year.
Comment 3 Radar WebKit Bug Importer 2016-09-01 09:38:52 PDT
<rdar://problem/28116759>
Comment 4 Cigitia 2017-12-04 19:40:46 PST
I ran into this problem today. All major browser engines except WebKit now support plain href in SVG. This includes Firefox, Chrome/Opera, Edge, and even Internet Explorer (https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/href). With the multiyear-old deprecation of xlink:href in SVG 2.0, this is a pernicious web-compatibility issue. Thanks in advance to anyone who resolves this bug.
Comment 5 Said Abou-Hallawa 2018-06-05 12:37:33 PDT
*** Bug 186315 has been marked as a duplicate of this bug. ***
Comment 6 Said Abou-Hallawa 2018-08-06 20:19:20 PDT
Created attachment 346680 [details]
Patch
Comment 7 Build Bot 2018-08-06 22:25:48 PDT
Comment on attachment 346680 [details]
Patch

Attachment 346680 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8783756

New failing tests:
fast/selectors/any-link-basics-2.html
Comment 8 Build Bot 2018-08-06 22:25:49 PDT
Created attachment 346685 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 Build Bot 2018-08-07 00:02:41 PDT
Comment on attachment 346680 [details]
Patch

Attachment 346680 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8784674

New failing tests:
fast/selectors/any-link-basics-2.html
Comment 10 Build Bot 2018-08-07 00:02:43 PDT
Created attachment 346690 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 11 Simon Fraser (smfr) 2018-08-07 08:31:25 PDT
Comment on attachment 346680 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        SVG 2 removed the need for the xlink namespace, so instead of xlink:href
> +        href should be used. See https://www.w3.org/TR/SVG2/linking.html#XLinkRefAttrs.

Is this behavior independent of the xmlns attribute on the <svg> element, or are SVG 2 documents in a mode where bare "href" is allowed?
Comment 12 Said Abou-Hallawa 2018-08-07 10:37:03 PDT
Created attachment 346714 [details]
Patch
Comment 13 Said Abou-Hallawa 2018-08-07 10:40:18 PDT
Comment on attachment 346680 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        href should be used. See https://www.w3.org/TR/SVG2/linking.html#XLinkRefAttrs.
> 
> Is this behavior independent of the xmlns attribute on the <svg> element, or are SVG 2 documents in a mode where bare "href" is allowed?

The href attribute now is part of the svg namespace. If the attribute xmlns="http://www.w3.org/2000/svg" is added to the <svg> element, then you can write "href" instead of "svg:href".
Comment 14 Said Abou-Hallawa 2018-08-07 13:25:42 PDT
Created attachment 346725 [details]
Patch
Comment 15 Dean Jackson 2018-08-07 13:40:04 PDT
Comment on attachment 346725 [details]
Patch

Why not add some reference tests that exercise references to filters, patterns, images, use, anchors - with and without the xlink: prefix? I'd definitely like to see a test that <a> works.
Comment 16 Said Abou-Hallawa 2018-08-07 16:33:38 PDT
Created attachment 346742 [details]
Patch
Comment 17 WebKit Commit Bot 2018-08-07 17:55:58 PDT
Comment on attachment 346742 [details]
Patch

Clearing flags on attachment: 346742

Committed r234683: <https://trac.webkit.org/changeset/234683>
Comment 18 WebKit Commit Bot 2018-08-07 17:56:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Darin Adler 2018-08-08 09:13:44 PDT
Comment on attachment 346742 [details]
Patch

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

> Source/WebCore/ChangeLog:32
> +        (WebCore::Element::getAttribute const): This new template function with
> +        template pack parameter QualifiedNames is added to return the first none
> +        empty attribute value given a set of attributes' names. This should be
> +        useful for deprecated attributes. When we decide to remove the support 
> +        for the deprecated attribute, all we need is to remove it as a parameter
> +        to getAttribute(). In this case, the none template function will be called.

I would have expected this to return the first non-null attribute value, not the first non-empty. Typically we would not ignore an attribute with an empty value.

Do we have test cases covering the behavior of ignoring an empty attribute? Is it truly the behavior we want for this first use of the new getAttribute function?

I’d like us to follow up and change it to non-null unless there is a reason the non-empty behavior is preferred.
Comment 20 Said Abou-Hallawa 2018-08-08 14:31:08 PDT
Comment on attachment 346742 [details]
Patch

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

>> Source/WebCore/ChangeLog:32
>> +        to getAttribute(). In this case, the none template function will be called.
> 
> I would have expected this to return the first non-null attribute value, not the first non-empty. Typically we would not ignore an attribute with an empty value.
> 
> Do we have test cases covering the behavior of ignoring an empty attribute? Is it truly the behavior we want for this first use of the new getAttribute function?
> 
> I’d like us to follow up and change it to non-null unless there is a reason the non-empty behavior is preferred.

The followup bug is https://bugs.webkit.org/show_bug.cgi?id=188419.
Comment 21 Said Abou-Hallawa 2019-05-06 15:33:52 PDT
*** Bug 183748 has been marked as a duplicate of this bug. ***
Comment 22 Said Abou-Hallawa 2019-05-07 09:16:51 PDT
*** Bug 181283 has been marked as a duplicate of this bug. ***