Closed Bug 1245751 Opened 8 years ago Closed 8 years ago

Allow href attribute without xlink on SVG elements

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: bbrinza, Assigned: boris)

References

(Blocks 2 open bugs,
URL
)

Details

(4 keywords)

Attachments

(15 files, 2 obsolete files)

Bug 1245751 - Part 1: Allow href without xlink on SVG <a> elements.
58 bytes, text/x-review-board-request
jwatt
: review+
dholbert
: review+
Details
Bug 1245751 - Part 2: Allow href without xlink on SVG <use> elements.
58 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1245751 - Part 3: Allow href without xlink on SVG <image> elements.
58 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1245751 - Part 4: Allow href without xlink on SVG <filter> elements.
58 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1245751 - Part 5: Allow href without xlink on SVG <feImage> elements.
58 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1245751 - Part 6: Allow href without xlink on SVG <pattern> elements.
58 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1245751 - Part 7: Allow href without xlink on SVG <mpath> elements.
58 bytes, text/x-review-board-request
jwatt
: review+
dholbert
: review+
Details
Bug 1245751 - Part 8: Allow href without xlink on SVG <textPath> elements.
58 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1245751 - Part 9: Allow href without xlink on SVG <script> elements.
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1245751 - Part 10: Allow href without xlink on SVG Gradient elements.
58 bytes, text/x-review-board-request
jwatt
: review+
Details
Bug 1245751 - Part 11: Allow href without xlink on SVG Animation elements.
58 bytes, text/x-review-board-request
jwatt
: review+
dholbert
: review+
Details
Bug 1245751 - Part 12: Retrieve href from both None & XLink namespace in Browser & Android.
58 bytes, text/x-review-board-request
Gijs
: review+
sebastian
: review+
Details
Bug 1245751 - Part 13: Add href in kURLAttributesSVG.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1245751 - Part 14: Add reftests.
58 bytes, text/x-review-board-request
heycam
: review+
dholbert
: review+
Details
Bug 1245751 - Part 15: Add tests for a, script, mpath, and animate elements.
58 bytes, text/x-review-board-request
heycam
: review+
dholbert
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2486.0 Safari/537.36 Edge/13.14252

Steps to reproduce:

Simple test case: https://jsfiddle.net/pzrez3eb/

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


Actual results:

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

Chromium: https://code.google.com/p/chromium/issues/detail?id=584142
WebKit: https://bugs.webkit.org/show_bug.cgi?id=153854
Status: UNCONFIRMED → NEW
Component: Untriaged → SVG
Ever confirmed: true
Product: Firefox → Core
Lack of  support for href without xlink breaks rendering of many examples in the SVG 2 spec.

Chrome Canary has support.
Assignee: nobody → boris.chiou
Boris, do you need some pointers to help get you started, or have you just been busy with other things?
Flags: needinfo?(boris.chiou)
(In reply to Jonathan Watt [:jwatt] from comment #2)
> Boris, do you need some pointers to help get you started, or have you just
> been busy with other things?

Yes. I really need some pointers. I checked this bug before and didn't have a good idea to start it. Could you please give me some advice? Thank you very much.
Flags: needinfo?(boris.chiou)
We need to keep the current 'xlink:href' support, so rather than removing the current behavior we need to add extra behavior for the 'href' attribute without the xlink namespace.

I think the three search terms you need to look at are:

https://dxr.mozilla.org/mozilla-central/search?q=kNameSpaceID_XLink
https://dxr.mozilla.org/mozilla-central/search?q=[HREF]
https://dxr.mozilla.org/mozilla-central/search?q=getAttributeNS+xlink+path%3Abrowser

In the cases like this:

https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGFilterFrame.cpp#174

where we invalidate, we would need to invalidate for 'href' too.

In cases like this:

https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGAElement.cpp#77

where we get and use the href, we'd need to prioritize whichever attribute takes precidence.
Status: NEW → ASSIGNED
Attachment #8767634 - Attachment is obsolete: true
Attachment #8767637 - Attachment is obsolete: true
Review commit: https://reviewboard.mozilla.org/r/62636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62636/
Attachment #8768710 - Flags: review?(jwatt)
Attachment #8768711 - Flags: review?(jwatt)
Attachment #8768712 - Flags: review?(jwatt)
Attachment #8768713 - Flags: review?(jwatt)
Attachment #8768714 - Flags: review?(jwatt)
Attachment #8768715 - Flags: review?(jwatt)
Attachment #8768716 - Flags: review?(jwatt)
Attachment #8768717 - Flags: review?(jwatt)
Attachment #8768718 - Flags: review?(jwatt)
Attachment #8768719 - Flags: review?(jwatt)
Attachment #8768720 - Flags: review?(jwatt)
(In reply to Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) from comment #4)
> We need to keep the current 'xlink:href' support, so rather than removing
> the current behavior we need to add extra behavior for the 'href' attribute
> without the xlink namespace.
> 
> I think the three search terms you need to look at are:
> 
> https://dxr.mozilla.org/mozilla-central/search?q=kNameSpaceID_XLink
> https://dxr.mozilla.org/mozilla-central/search?q=[HREF]
> https://dxr.mozilla.org/mozilla-central/
> search?q=getAttributeNS+xlink+path%3Abrowser

Thanks, Jonathan. I haven't written the test cases yet. Do you know where is the most suitable place I can put test cases?
(In reply to Boris Chiou [:boris] from comment #19)
> Thanks, Jonathan. I haven't written the test cases yet. Do you know where is
> the most suitable place I can put test cases?

BTW. I think we don't need to allow href attribute without xlink for MathML elements, right? So, I didn't revise MathML in these patches.
https://reviewboard.mozilla.org/r/62804/#review60176

::: browser/base/content/browser.js:5374
(Diff revision 1)
>      if (node.nodeType == Node.ELEMENT_NODE &&
>          (node.localName == "a" ||
>           node.namespaceURI == "http://www.w3.org/1998/Math/MathML")) {
> -      href = node.getAttributeNS("http://www.w3.org/1999/xlink", "href");
> +      href = node.getAttributeNS("", "href") ||
> +             node.getAttributeNS("http://www.w3.org/1999/xlink", "href");
> +

use getAttribute or pass null rather than "" for the null namespace case.
(In reply to Boris Chiou [:boris] from comment #20)
> (In reply to Boris Chiou [:boris] from comment #19)
> > Thanks, Jonathan. I haven't written the test cases yet. Do you know where is
> > the most suitable place I can put test cases?
> 
> BTW. I think we don't need to allow href attribute without xlink for MathML
> elements, right? So, I didn't revise MathML in these patches.

right, this is just for SVG.
https://reviewboard.mozilla.org/r/62804/#review60176

> use getAttribute or pass null rather than "" for the null namespace case.

Thanks, Robert. I will update it.
Comment on attachment 8768721 [details]
Bug 1245751 - Part 12: Retrieve href from both None & XLink namespace in Browser & Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62804/diff/1-2/
Attachment #8768721 - Flags: review?(gijskruitbosch+bugs)
Attachment #8768721 - Flags: review?(s.kaspari)
Attachment #8768721 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8768721 [details]
Bug 1245751 - Part 12: Retrieve href from both None & XLink namespace in Browser & Android.

https://reviewboard.mozilla.org/r/62804/#review60180

r=me for the browser/ changes - pinged sebastian for the mobile one.
https://reviewboard.mozilla.org/r/62636/#review60204

::: dom/base/Link.cpp
(Diff revision 1)
>  Link::ElementHasHref() const
>  {
> -  return ((!mElement->IsSVGElement() &&
> -           mElement->HasAttr(kNameSpaceID_None, nsGkAtoms::href))
> -        || (!mElement->IsHTMLElement() &&
> +  return mElement->HasAttr(kNameSpaceID_None, nsGkAtoms::href) ||
> +         (!mElement->IsHTMLElement() &&
> +          mElement->HasAttr(kNameSpaceID_XLink, nsGkAtoms::href));
> -            mElement->HasAttr(kNameSpaceID_XLink, nsGkAtoms::href)));

You need to check for SVG and html here otherwise you may change things for mathml.
https://reviewboard.mozilla.org/r/62638/#review60206

::: dom/svg/SVGUseElement.cpp:402
(Diff revision 1)
>  SVGUseElement::LookupHref()
>  {
>    nsAutoString href;
>    mStringAttributes[HREF].GetAnimValue(href, this);
> -  if (href.IsEmpty())
> +  if (href.IsEmpty()) {
> +    mStringAttributes[XLINK_HREF].GetAnimValue(href, this);

This needs to use IsExplicitlySet to choose HREF vs XLINK_HREF and once that's done check for empty as href="" xlink:href="/something" should be interpreted as href="" and not use the xlink:href value.
https://reviewboard.mozilla.org/r/62638/#review60206

> This needs to use IsExplicitlySet to choose HREF vs XLINK_HREF and once that's done check for empty as href="" xlink:href="/something" should be interpreted as href="" and not use the xlink:href value.

Got it. Thanks.
Comment on attachment 8768710 [details]
Bug 1245751 - Part 1: Allow href without xlink on SVG <a> elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62636/diff/1-2/
Comment on attachment 8768711 [details]
Bug 1245751 - Part 2: Allow href without xlink on SVG <use> elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62638/diff/1-2/
Comment on attachment 8768712 [details]
Bug 1245751 - Part 3: Allow href without xlink on SVG <image> elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62640/diff/1-2/
Comment on attachment 8768713 [details]
Bug 1245751 - Part 4: Allow href without xlink on SVG <filter> elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62642/diff/1-2/
Comment on attachment 8768714 [details]
Bug 1245751 - Part 5: Allow href without xlink on SVG <feImage> elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62644/diff/1-2/
Comment on attachment 8768715 [details]
Bug 1245751 - Part 6: Allow href without xlink on SVG <pattern> elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62646/diff/1-2/
Comment on attachment 8768716 [details]
Bug 1245751 - Part 7: Allow href without xlink on SVG <mpath> elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62648/diff/1-2/
Comment on attachment 8768717 [details]
Bug 1245751 - Part 8: Allow href without xlink on SVG <textPath> elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62768/diff/1-2/
Comment on attachment 8768719 [details]
Bug 1245751 - Part 10: Allow href without xlink on SVG Gradient elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62772/diff/1-2/
Comment on attachment 8768720 [details]
Bug 1245751 - Part 11: Allow href without xlink on SVG Animation elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62798/diff/1-2/
Attachment #8768721 - Flags: review?(s.kaspari) → review+
Comment on attachment 8768721 [details]
Bug 1245751 - Part 12: Retrieve href from both None & XLink namespace in Browser & Android.

https://reviewboard.mozilla.org/r/62804/#review60298
Comment on attachment 8768710 [details]
Bug 1245751 - Part 1: Allow href without xlink on SVG <a> elements.

https://reviewboard.mozilla.org/r/62636/#review61922

r+ with the two issues fixed.

::: dom/base/Link.cpp:48
(Diff revision 2)
> -  return ((!mElement->IsSVGElement() &&
> -           mElement->HasAttr(kNameSpaceID_None, nsGkAtoms::href))
> -        || (!mElement->IsHTMLElement() &&
> -            mElement->HasAttr(kNameSpaceID_XLink, nsGkAtoms::href)));
> +  return ((mElement->IsHTMLElement() || mElement->IsSVGElement()) &&
> +          mElement->HasAttr(kNameSpaceID_None, nsGkAtoms::href)) ||
> +         (!mElement->IsHTMLElement() &&
> +          mElement->HasAttr(kNameSpaceID_XLink, nsGkAtoms::href));

This change would stop plain 'href' being a link on non-SVG and non-HTML elements. Instead make this code:

  return mElement->HasAttr(kNameSpaceID_None, nsGkAtoms::href) ||
         (!mElement->IsHTMLElement() &&
          mElement->HasAttr(kNameSpaceID_XLink, nsGkAtoms::href));

If you want to make this consistent with other parts of the code then best to spin off another bug to be fixed after this one is done.

::: dom/svg/SVGAElement.cpp:234
(Diff revision 2)
>  
>    // Optimization: check for href first for early return
> -  const nsAttrValue* href = mAttrsAndChildren.GetAttr(nsGkAtoms::href,
> -                                                      kNameSpaceID_XLink);
> +  bool useXLink = false;
> +  const nsAttrValue* href = nullptr;
> +  if (HasAttr(kNameSpaceID_None, nsGkAtoms::href)) {
> +    useXLink = true;

This should be in the |else| branch, no?
Attachment #8768710 - Flags: review?(jwatt) → review+
Comment on attachment 8768711 [details]
Bug 1245751 - Part 2: Allow href without xlink on SVG <use> elements.

https://reviewboard.mozilla.org/r/62638/#review61928
Attachment #8768711 - Flags: review?(jwatt) → review+
Should this also update nsTreeSanitizer? It seems like it needs to call SanitizeURL() for the non-xlink'd attribute, and maybe remove it depending on flags (much less sure about the latter, not too familiar with the implementation details).
Flags: needinfo?(boris.chiou)
https://reviewboard.mozilla.org/r/62636/#review61922

> This change would stop plain 'href' being a link on non-SVG and non-HTML elements. Instead make this code:
> 
>   return mElement->HasAttr(kNameSpaceID_None, nsGkAtoms::href) ||
>          (!mElement->IsHTMLElement() &&
>           mElement->HasAttr(kNameSpaceID_XLink, nsGkAtoms::href));
> 
> If you want to make this consistent with other parts of the code then best to spin off another bug to be fixed after this one is done.

OK. Thanks. I should remove the check of IsHTMLElement and IsSVGElement for plain href, as your example.

> This should be in the |else| branch, no?

Yes. It should be in |else| branch. Sorry about this. I will update soon.
(In reply to :Gijs Kruitbosch from comment #44)
> Should this also update nsTreeSanitizer? It seems like it needs to call
> SanitizeURL() for the non-xlink'd attribute, and maybe remove it depending
> on flags (much less sure about the latter, not too familiar with the
> implementation details).

Thanks for your reminder. I guess we have to add &nsGkAtom::href in kURLAttributesSVG [1], just as what we do for kURLAttributeHTML, so we can check non-xlink SVG elements in IsURL(). I will upload an extra patch.

[1] https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/base/nsTreeSanitizer.cpp#594-596
Flags: needinfo?(boris.chiou)
Comment on attachment 8772273 [details]
Bug 1245751 - Part 13: Add href in kURLAttributesSVG.

https://reviewboard.mozilla.org/r/65132/#review62394

Part 13 looks fine; I'm happy to r+, with the caveat that I've never looked at nsTreeSanitizer.cpp before today. However, I spent some time reading the code surrounding the usages of this variable (kURLAttributesSVG) and its HTML analog, and looking at other changes to this file, and I'm confident that this change makes sense, given that we're allowing `href="<url>"` on SVG elements now.

(I was initially tempted to just punt this to hsivonen, as he's the principal author/reviewer on non-trivial changes to this file -- but it looks like he's not doing reviews a few weeks. So, I've simply CC'd him instead, so that he can sanity-check this one-liner if he likes.)
Attachment #8772273 - Flags: review?(dholbert) → review+
Comment on attachment 8768712 [details]
Bug 1245751 - Part 3: Allow href without xlink on SVG <image> elements.

https://reviewboard.mozilla.org/r/62640/#review63386
Attachment #8768712 - Flags: review?(jwatt) → review+
Comment on attachment 8768713 [details]
Bug 1245751 - Part 4: Allow href without xlink on SVG <filter> elements.

https://reviewboard.mozilla.org/r/62642/#review63388
Attachment #8768713 - Flags: review?(jwatt) → review+
Comment on attachment 8768714 [details]
Bug 1245751 - Part 5: Allow href without xlink on SVG <feImage> elements.

https://reviewboard.mozilla.org/r/62644/#review63390
Attachment #8768714 - Flags: review?(jwatt) → review+
Attachment #8768715 - Flags: review?(jwatt) → review+
Comment on attachment 8768715 [details]
Bug 1245751 - Part 6: Allow href without xlink on SVG <pattern> elements.

https://reviewboard.mozilla.org/r/62646/#review63392
Attachment #8768716 - Flags: review?(jwatt) → review+
Comment on attachment 8768716 [details]
Bug 1245751 - Part 7: Allow href without xlink on SVG <mpath> elements.

https://reviewboard.mozilla.org/r/62648/#review63394
Attachment #8768717 - Flags: review?(jwatt) → review+
Comment on attachment 8768717 [details]
Bug 1245751 - Part 8: Allow href without xlink on SVG <textPath> elements.

https://reviewboard.mozilla.org/r/62768/#review63396
Comment on attachment 8768718 [details]
Bug 1245751 - Part 9: Allow href without xlink on SVG <script> elements.

https://reviewboard.mozilla.org/r/62770/#review63398

::: dom/svg/SVGScriptElement.cpp:172
(Diff revision 3)
> -  return (mFrozen ? mExternal : mStringAttributes[HREF].IsExplicitlySet()) ||
> +  return (mFrozen ? mExternal
> +                  : mStringAttributes[HREF].IsExplicitlySet() ||
> +                    mStringAttributes[XLINK_HREF].IsExplicitlySet()) ||
>           nsContentUtils::HasNonEmptyTextContent(this);

This ends up being weildly indented. It would seem logical to indent the nsContentUtils::HasNonEmptyTextContent too, but instead I think it would be better to write this as:

if (mFrozen) {
  return mExternal;
}
return mStringAttributes[HREF].IsExplicitlySet() ||
       mStringAttributes[XLINK_HREF].IsExplicitlySet()) ||
       nsContentUtils::HasNonEmptyTextContent(this);
Attachment #8768718 - Flags: review?(jwatt) → review+
https://reviewboard.mozilla.org/r/62770/#review63398

> This ends up being weildly indented. It would seem logical to indent the nsContentUtils::HasNonEmptyTextContent too, but instead I think it would be better to write this as:
> 
> if (mFrozen) {
>   return mExternal;
> }
> return mStringAttributes[HREF].IsExplicitlySet() ||
>        mStringAttributes[XLINK_HREF].IsExplicitlySet()) ||
>        nsContentUtils::HasNonEmptyTextContent(this);

Sorry, I misread this code. Ignore that comment.
Comment on attachment 8768719 [details]
Bug 1245751 - Part 10: Allow href without xlink on SVG Gradient elements.

https://reviewboard.mozilla.org/r/62772/#review63402
Attachment #8768719 - Flags: review?(jwatt) → review+
Comment on attachment 8768720 [details]
Bug 1245751 - Part 11: Allow href without xlink on SVG Animation elements.

https://reviewboard.mozilla.org/r/62798/#review63404
Attachment #8768720 - Flags: review?(jwatt) → review+
Jonathan, did you want to address comment 19?
Flags: needinfo?(jwatt)
Thanks for the reminder, Robert.

(In reply to Boris Chiou [:boris] from comment #19)
> Thanks, Jonathan. I haven't written the test cases yet. Do you know where is
> the most suitable place I can put test cases?

I'm not sure what would be best, but I think it would be good to put the tests in:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/svg

For documentation see:

https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests#Creating_new_tests
http://testthewebforward.org/docs/reftests.html

I think it would perhaps make most sense to create a 'href' subdirectory and, for each element type that you've added support for 'href', add a simple test (that fills the screen with green when possible) that uses 'href' and make the reference file the exact same except use 'xlink:href'.

Cameron, I see that you're in the OWNERS file for that directory. Does the above seem sane to you?
Flags: needinfo?(jwatt) → needinfo?(cam)
(In reply to Jonathan Watt [:jwatt] (catching up after vacation) from comment #74)
> I think it would perhaps make most sense to create a 'href' subdirectory
> and, for each element type that you've added support for 'href', add a
> simple test (that fills the screen with green when possible) that uses
> 'href' and make the reference file the exact same except use 'xlink:href'.
> 
> Cameron, I see that you're in the OWNERS file for that directory. Does the
> above seem sane to you?

Thanks, Jonathan and Robert. I am going to write some reftests in testing/web-platform/tests/svg/href and wait for Cameron's comments before sending review requests.
(In reply to Jonathan Watt [:jwatt] (catching up after vacation) from comment #74)
> I think it would perhaps make most sense to create a 'href' subdirectory
> and, for each element type that you've added support for 'href', add a
> simple test (that fills the screen with green when possible) that uses
> 'href' and make the reference file the exact same except use 'xlink:href'.
> 
> Cameron, I see that you're in the OWNERS file for that directory. Does the
> above seem sane to you?

Adding tests to web-platform-tests would be great.  Let's put these in a directory named testing/web-platform/tests/svg/linking/reftests/.  (In the README.md it suggests a directory per chapter of the spec, and then either a reftests/ or scripted/ subdirectory depending on the type of the test.)
Flags: needinfo?(cam)
Attachment #8768716 - Flags: review?(dholbert)
Attachment #8768720 - Flags: review?(dholbert)
Attachment #8779646 - Flags: review?(cam)
Attachment #8779647 - Flags: review?(cam)
Hi dholbert,

I sent review requests to you for part 7 (mpath) and part 11 (Animation) because I found a potential bug in these patches. ParseAttribute() will check the condition and then call UpdateHrefTarget(). However, there is a use case:

1. mpath.setAttribute('href', '#a');
2. mpath.setAttributeNS('xxx/xlink', 'xlink:href', '#b');

If we have |href| attribute already, I think we shouldn't call UpdateHrefTarget() while trying to set |xlink:href| attribute, or the animation target won't be correct.

Could you please take a look at these two patches? Thanks.
(In reply to Cameron McCormack (:heycam) from comment #76)
> (In reply to Jonathan Watt [:jwatt] (catching up after vacation) from
> comment #74)
> > I think it would perhaps make most sense to create a 'href' subdirectory
> > and, for each element type that you've added support for 'href', add a
> > simple test (that fills the screen with green when possible) that uses
> > 'href' and make the reference file the exact same except use 'xlink:href'.
> > 
> > Cameron, I see that you're in the OWNERS file for that directory. Does the
> > above seem sane to you?
> 
> Adding tests to web-platform-tests would be great.  Let's put these in a
> directory named testing/web-platform/tests/svg/linking/reftests/.  (In the
> README.md it suggests a directory per chapter of the spec, and then either a
> reftests/ or scripted/ subdirectory depending on the type of the test.)

I add both reftests/ and scripted/ because some elements, e.g. animate, mpath, script, are easier to be tested by mochitest.
Comment on attachment 8779646 [details]
Bug 1245751 - Part 14: Add reftests.

https://reviewboard.mozilla.org/r/69858/#review67964

r=me with these comments addressed.

::: testing/web-platform/tests/svg/linking/reftests/href-feImage-element.html:10
(Diff revision 1)
> +<link rel="match" href="href-feImage-element-ref.html">
> +<body>
> +  <svg width="300" height="300" viewBox="0 0 300 300"
> +       xmlns:xlink="http://www.w3.org/1999/xlink">
> +    <filter id="Fitted" primitiveUnits="objectBoundingBox">
> +      <feImage href="images/firefox.png" xlink:href="images/chrome.png"

The browser logos look nice, but it's probably best to avoid using them in the tests we check in here.  Can you use any of the images in web-platform/tests/images/ (which you can reference in the test files using "/images/...")?

::: testing/web-platform/tests/svg/linking/reftests/href-gradient-element.html:15
(Diff revision 1)
> +      <stop offset="5%"  stop-color="green"/>
> +      <stop offset="95%" stop-color="gold"/>
> +    </linearGradient>
> +    <linearGradient id="MyGradient2">
> +      <stop offset="5%"  stop-color="red"/>
> +      <stop offset="95%" stop-color="blud"/>

"blue"
Attachment #8779646 - Flags: review?(cam) → review+
Comment on attachment 8779647 [details]
Bug 1245751 - Part 15: Add tests for a, script, mpath, and animate elements.

https://reviewboard.mozilla.org/r/70614/#review67970

::: testing/web-platform/tests/svg/linking/scripted/href-animate-element.html:18
(Diff revision 1)
> +async_test(function(t) {
> +  var svg = document.getElementById('svg');
> +  var rect1 = createSVGElement(t, 'rect', svg,
> +                               { 'width': '10px',
> +                                 'height': '10px',
> +                                'id': 'rect1' });

Nit: align this with the line above.
Comment on attachment 8779647 [details]
Bug 1245751 - Part 15: Add tests for a, script, mpath, and animate elements.

https://reviewboard.mozilla.org/r/70614/#review67972

s/mapth/mpath/ in the commit message.
Attachment #8779647 - Flags: review?(cam) → review+
Comment on attachment 8779646 [details]
Bug 1245751 - Part 14: Add reftests.

https://reviewboard.mozilla.org/r/69858/#review67964

> The browser logos look nice, but it's probably best to avoid using them in the tests we check in here.  Can you use any of the images in web-platform/tests/images/ (which you can reference in the test files using "/images/...")?

Got it. I will remove these two png file and try to use images in web-platform/tests/images/. Thanks.
(In reply to Boris Chiou [:boris] from comment #92)
> Hi dholbert,
[...]
> If we have |href| attribute already, I think we shouldn't call
> UpdateHrefTarget() while trying to set |xlink:href| attribute, or the
> animation target won't be correct.

It looks like |href| is supposed to "win" over |xlink:href|, per comment 27, so I think you're correct about this.
Comment on attachment 8768716 [details]
Bug 1245751 - Part 7: Allow href without xlink on SVG <mpath> elements.

https://reviewboard.mozilla.org/r/62648/#review68540

::: dom/svg/SVGMPathElement.cpp:144
(Diff revisions 3 - 4)
>      // on next BindToTree call.
> +    if (aNamespaceID == kNameSpaceID_XLink &&
> +        mStringAttributes[HREF].IsExplicitlySet()) {
> +      // We don't have to update the target by xlink:href if we have href
> +      // attribute already, so early return.
> +      return returnVal;

The logic structure is a bit confusing here.

Right now you're structuring the new code as a new special-case (a new early-return) inside of an existing special case.  Instead, let's just further-narrow the existing special-case, so we can share the existing "return" value and avoid having too many branching codepaths to follow.

So: please flip the logic here -- e.g.: wrap the existing UpdateHrefTarget call in a layer of nesting like so:

    // Note: "href" takes priority over xlink:href. So if "xlink:href" is being
    // set here, we only let that update our target if "href" is *unset*.
    if (aNamespaceID != kNameSpaceID_XLink ||
        !mStringAttributes[HREF].IsExplicitlySet()) {
      UpdateHrefTarget(GetParent(), aValue);
    }

r=me with that.
Attachment #8768716 - Flags: review?(dholbert) → review+
Comment on attachment 8768716 [details]
Bug 1245751 - Part 7: Allow href without xlink on SVG <mpath> elements.

https://reviewboard.mozilla.org/r/62648/#review68542

er, sorry -- noticed a few more things, one of which (in UnsetAttr) means this needs a bit of fixing.  Hence, setting this as r- for the moment.

::: dom/svg/SVGMPathElement.cpp:102
(Diff revision 4)
>                                                  aCompileEventHandlers);
>    NS_ENSURE_SUCCESS(rv,rv);
>  
>    if (aDocument) {
> -    const nsAttrValue* hrefAttrValue =
> +    const nsAttrValue* hrefAttrValue = nullptr;
> +    if (HasAttr(kNameSpaceID_None, nsGkAtoms::href)) {

Two nits:
 (1) Should this be using IsExplicitlySet() instead of HasAttr? That's what you're using elsewhere, but you're using HasAttr here. (I don't recall the distinction offhand.)

 (2) Could you structure this as a ternary expression, so it's more compact? (and so we don't have to repeat the variable name quite as much)  This should do it:
   const nsAttrValue* hrefAttrValue =
     HasAttr(kNameSpaceID_None, nsGkAtoms::href)
     ?  mAttrsAndChildren.GetAttr(nsGkAtoms::href, kNameSpaceID_None)
     :  mAttrsAndChildren.GetAttr(nsGkAtoms::href, kNameSpaceID_XLink);

That makes it easier to see the difference between the two GetAttr calls, too (just the namespace is different), since they'll now be directly adjacent to each other, on consecutive lines.

::: dom/svg/SVGMPathElement.cpp:162
(Diff revision 4)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  if (aNamespaceID == kNameSpaceID_XLink &&
> +  if ((aNamespaceID == kNameSpaceID_XLink ||
> +       aNamespaceID == kNameSpaceID_None) &&
>        aAttribute == nsGkAtoms::href) {
>      UnlinkHrefTarget(true);

After this UnlinkHrefTarget call, we need to *add back* a new HrefTarget if the other attribute is set here, right?

e.g. supposing we have...
 <mpath href="#foo" xlink:href="#bar">
...and someone unsets the "href" attribute, then UnlinkHrefTarget() will leave us thinking we have no href target.  But really we need to fall back to honoring our "xlink:href" attribute at that point!

Please make sure we've got a testcase that covers this scenario (dynamically removing the href attribute from an mpath element that has xlink:href set, in a way where it observably should affect behavior)

::: dom/svg/SVGMPathElement.cpp:206
(Diff revision 4)
>  {
> -  if (!HasAttr(kNameSpaceID_XLink, nsGkAtoms::href)) {
> +  if (!HasAttr(kNameSpaceID_XLink, nsGkAtoms::href) &&
> +      !HasAttr(kNameSpaceID_None, nsGkAtoms::href)) {
>      MOZ_ASSERT(!mHrefTarget.get(),
>                 "We shouldn't have an xlink:href target "
> -               "if we don't have an xlink:href attribute");
> +               "if we don't have an xlink:href or href attribute");

Please broaden the first line of this assertion-message to cover 'href', too.

(In the first line of the assertion-message, please replace "xlink:href target" with just "href target" -- that should be clearer / more general.)
Attachment #8768716 - Flags: review+ → review-
Comment on attachment 8768720 [details]
Bug 1245751 - Part 11: Allow href without xlink on SVG Animation elements.

https://reviewboard.mozilla.org/r/62798/#review68546

::: dom/svg/SVGAnimationElement.cpp:84
(Diff revision 4)
>  
>  Element*
>  SVGAnimationElement::GetTargetElementContent()
>  {
> -  if (HasAttr(kNameSpaceID_XLink, nsGkAtoms::href)) {
> +  if (HasAttr(kNameSpaceID_XLink, nsGkAtoms::href) ||
> +      HasAttr(kNameSpaceID_None, nsGkAtoms::href)) {

(I guess jwatt already reviewed this part, but I'm wondering whether we should be using IsExplicitlySet here, instead of HasAttr... I don't recall how HasAttr() and IsExplicitlySet differ semantically and whether it's relevant here. Perhaps longsonr remembers -- Robert, am I getting worried over nothing?)

::: dom/svg/SVGAnimationElement.cpp:328
(Diff revision 4)
>    if (!aValue) {
>      mHrefTarget.Unlink();
>      AnimationTargetChanged();
> -  } else if (IsInUncomposedDoc()) {
> +  } else if (IsInUncomposedDoc() &&
> +             !(aNamespaceID == kNameSpaceID_XLink &&
> +               HasAttr(kNameSpaceID_None, nsGkAtoms::href))) {

Please add a code-comment somewhere here (like the one in my sample-code in comment 101) to explain these new checks.
Flags: needinfo?(longsonr)
Comment on attachment 8768720 [details]
Bug 1245751 - Part 11: Allow href without xlink on SVG Animation elements.

https://reviewboard.mozilla.org/r/62798/#review68552

::: dom/svg/SVGAnimationElement.cpp:324
(Diff revision 4)
> +        aName == nsGkAtoms::href)) {
>      return rv;
> +  }
>  
>    if (!aValue) {
>      mHrefTarget.Unlink();

It looks like this (existing) code clears mHrefTarget when a href-flavored attribute is unset.

Now that we have two href attributes, we may need to fall back to the other attribute at this point, if we've just unset "href" and we still have a "xlink:href" attribute.  We'll need code here to handle that. (and a test for that behavior, too.)
Comment on attachment 8768716 [details]
Bug 1245751 - Part 7: Allow href without xlink on SVG <mpath> elements.

https://reviewboard.mozilla.org/r/62648/#review68542

> Two nits:
>  (1) Should this be using IsExplicitlySet() instead of HasAttr? That's what you're using elsewhere, but you're using HasAttr here. (I don't recall the distinction offhand.)
> 
>  (2) Could you structure this as a ternary expression, so it's more compact? (and so we don't have to repeat the variable name quite as much)  This should do it:
>    const nsAttrValue* hrefAttrValue =
>      HasAttr(kNameSpaceID_None, nsGkAtoms::href)
>      ?  mAttrsAndChildren.GetAttr(nsGkAtoms::href, kNameSpaceID_None)
>      :  mAttrsAndChildren.GetAttr(nsGkAtoms::href, kNameSpaceID_XLink);
> 
> That makes it easier to see the difference between the two GetAttr calls, too (just the namespace is different), since they'll now be directly adjacent to each other, on consecutive lines.

(1) I use HasAttr() because I think we will call mAttrsAndChildren.GetAttr(...), and using HasAttr, which also checks mAttrsAndChildren in Element, might be better.
(2) Sure. Thanks for your suggestion! It's much better

> After this UnlinkHrefTarget call, we need to *add back* a new HrefTarget if the other attribute is set here, right?
> 
> e.g. supposing we have...
>  <mpath href="#foo" xlink:href="#bar">
> ...and someone unsets the "href" attribute, then UnlinkHrefTarget() will leave us thinking we have no href target.  But really we need to fall back to honoring our "xlink:href" attribute at that point!
> 
> Please make sure we've got a testcase that covers this scenario (dynamically removing the href attribute from an mpath element that has xlink:href set, in a way where it observably should affect behavior)

OK. I will extend the test case in Part 15. Thanks.

> Please broaden the first line of this assertion-message to cover 'href', too.
> 
> (In the first line of the assertion-message, please replace "xlink:href target" with just "href target" -- that should be clearer / more general.)

Got it. Thanks.
IsExplicitlySet becomes true if an attribute is set via SMIL (or via setAttribute). HasAttribute is only true if the attribute is set via setAttribute).

Animation attributes however cannot themselves be set via SMIL so IsExplicitlySet and HasAttribute amount to the same thing in that case. The patch as it stands is fine.
Flags: needinfo?(longsonr)
Comment on attachment 8768720 [details]
Bug 1245751 - Part 11: Allow href without xlink on SVG Animation elements.

https://reviewboard.mozilla.org/r/62798/#review68552

> It looks like this (existing) code clears mHrefTarget when a href-flavored attribute is unset.
> 
> Now that we have two href attributes, we may need to fall back to the other attribute at this point, if we've just unset "href" and we still have a "xlink:href" attribute.  We'll need code here to handle that. (and a test for that behavior, too.)

Thanks, Daniel. This is just like what we have to do for mpath. I will also add a test in part 15.
(In reply to Robert Longson from comment #106)
> IsExplicitlySet becomes true if an attribute is set via SMIL (or via
> setAttribute). HasAttribute is only true if the attribute is set via
> setAttribute).
> 
> Animation attributes however cannot themselves be set via SMIL so
> IsExplicitlySet and HasAttribute amount to the same thing in that case. The
> patch as it stands is fine.

Thanks, Robert. I will keep using HasAttr() here.
Attachment #8779647 - Flags: review?(dholbert)
(In reply to Boris Chiou [:boris] from comment #117)
> Comment on attachment 8779647 [details]
> Bug 1245751 - Part 15: Add tests for a, script, mpath, and animate elements.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/70614/diff/2-3/

Hi Daniel, Could you please take a look at href-mpath-element.html and href-animate-element.html? I added two test case for removing the href attribute but there is still an xlink:href attribute.

Thanks.
Comment on attachment 8768716 [details]
Bug 1245751 - Part 7: Allow href without xlink on SVG <mpath> elements.

https://reviewboard.mozilla.org/r/62648/#review68782

Thanks! The updates to part 7 look good. r=me
Attachment #8768716 - Flags: review?(dholbert) → review+
Comment on attachment 8768720 [details]
Bug 1245751 - Part 11: Allow href without xlink on SVG Animation elements.

https://reviewboard.mozilla.org/r/62798/#review68784

Updates to part 11 look good; r=me
Attachment #8768720 - Flags: review?(dholbert) → review+
Comment on attachment 8779647 [details]
Bug 1245751 - Part 15: Add tests for a, script, mpath, and animate elements.

https://reviewboard.mozilla.org/r/70614/#review68788

The new tests look good - thanks!

Before landing, could you also test the opposite removal scenario? (Same setup -- with xlink:href & href both set.  Then, remove xlink:href, and verify that targetElement is unchanged -- it should still be whatever "href" points to.)

That's another edge case where we could conceivably get this wrong.  (It looks like your patch should gets this case right, via a commented "else" explanation at the end of each if/else-cascade, but it's nice to have verification that we do in fact handle it correctly via a test.)

Thanks!  r=me with that -- I won't bother re-reviewing with those changes (unless you'd like me to), since I expect the new test code will look 80-90% identical to the test code that I've just reviewed here.
Attachment #8779647 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #121)
> Before landing, could you also test the opposite removal scenario? (Same
> setup -- with xlink:href & href both set.  Then, remove xlink:href, and
> verify that targetElement is unchanged -- it should still be whatever "href"
> points to.)
> 
> That's another edge case where we could conceivably get this wrong.  (It
> looks like your patch should gets this case right, via a commented "else"
> explanation at the end of each if/else-cascade, but it's nice to have
> verification that we do in fact handle it correctly via a test.)
> 

Sure. Thanks for your review.

However, I found two tests failed in try, href-attr-change-restyles.svg, dynamic-link-style-01.svg, so I begin to check what happened to them.
(In reply to Boris Chiou [:boris] from comment #122)
> However, I found two tests failed in try, href-attr-change-restyles.svg,
> dynamic-link-style-01.svg, so I begin to check what happened to them.

Oops. I found the root cause. It's just a typo in part 1. Anyway, after re-checking all the setAttribute/removeAttribute combinations for all elements and passing all the test cases, I will land this bug.
(In reply to Boris Chiou [:boris] from comment #124)
> Comment on attachment 8768710 [details]
> Bug 1245751 - Part 1: Allow href without xlink on SVG <a> elements.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/62636/diff/4-5/

Hi, Daniel,

I checked all the elements after calling removeAttribute('href') or removeAttribute('xlink:href'), and I think the patch for <a> element also needs to be updated. In UnsetAttr(), we have to check if there is still href or xlink:href attribute and pass correct aHasHref argument into Link::ResetLinkState to make sure the link status is correct [1]. BTW, I also added a reftest in part 14. You can just check href-a-element-attr-change.html and href-a-element-ref.html.

In addition to <a> element, <script> element also has a similar problem.
e.g.
<script href="abc.js" xlink:href="def.js" id='s'/>
then call "document.getElementById('s').removeAttribute('href');"

nsScriptElement::MaybeProcessScript() has a flag, mAlreadyStarted [2], and it seems that if we have started the |href| script (i.e. abc.js) already, the |xlink:href| script (i.e. def.js) will not be loaded after removing 'href' attribute. Does it make sense? I'm not sure whether we should load def.js after removing href attribute or not, so keep it unchanged.
Thanks.

[1] http://searchfox.org/mozilla-central/rev/b92ae9a263a05efefc0110eaad5e56369c9f1b10/dom/base/Link.cpp#546-550
[2] http://searchfox.org/mozilla-central/rev/b92ae9a263a05efefc0110eaad5e56369c9f1b10/dom/base/nsScriptElement.cpp#118-125
Comment on attachment 8768710 [details]
Bug 1245751 - Part 1: Allow href without xlink on SVG <a> elements.

https://reviewboard.mozilla.org/r/62636/#review70414

r=me on updates to part 1
Attachment #8768710 - Flags: review?(dholbert) → review+
Comment on attachment 8779646 [details]
Bug 1245751 - Part 14: Add reftests.

https://reviewboard.mozilla.org/r/69858/#review70418

r=me on updates to reftest patch
Attachment #8779646 - Flags: review?(dholbert) → review+
(In reply to Boris Chiou [:boris] from comment #139)
> In addition to <a> element, <script> element also has a similar problem.
> e.g.
> <script href="abc.js" xlink:href="def.js" id='s'/>
> [...] I'm not sure whether we should load
> def.js after removing href attribute or not, so keep it unchanged.

I think we should load it, yeah.  In other words, given your <script> quoted above, the following two sequences of operations should be indistinguishable from each other:
  (1) remove xlink:href, then remove href, then add back xlink:href.
  (2) remove href.

Right now, it sounds like the first one would have us load def.js and the second would not -- but they should behave the same (both loading def.js), I think.

Similarly, given <script href="abc.js">, I'd expect the following two sequences to have the same result:
 (1) remove href, then add xlink:href
 (2) add xlink:href, then remove href
(In reply to Daniel Holbert [:dholbert] from comment #142)
> (In reply to Boris Chiou [:boris] from comment #139)
> > In addition to <a> element, <script> element also has a similar problem.
> > e.g.
> > <script href="abc.js" xlink:href="def.js" id='s'/>
> > [...] I'm not sure whether we should load
> > def.js after removing href attribute or not, so keep it unchanged.
> 
> I think we should load it, yeah.  In other words, given your <script> quoted
> above, the following two sequences of operations should be indistinguishable
> from each other:
>   (1) remove xlink:href, then remove href, then add back xlink:href.
>   (2) remove href.
> 
> Right now, it sounds like the first one would have us load def.js and the
> second would not -- but they should behave the same (both loading def.js), I
> think.
> 
> Similarly, given <script href="abc.js">, I'd expect the following two
> sequences to have the same result:
>  (1) remove href, then add xlink:href
>  (2) add xlink:href, then remove href

Thanks for the explanation. I see, and I will update part 9, so we can load the new script after adding/removing href or xlink:href attribute.
(In reply to Daniel Holbert [:dholbert] from comment #142)
> (In reply to Boris Chiou [:boris] from comment #139)
> > In addition to <a> element, <script> element also has a similar problem.
> > e.g.
> > <script href="abc.js" xlink:href="def.js" id='s'/>
> > [...] I'm not sure whether we should load
> > def.js after removing href attribute or not, so keep it unchanged.
> 
> I think we should load it, yeah.  In other words, given your <script> quoted
> above, the following two sequences of operations should be indistinguishable
> from each other:
>   (1) remove xlink:href, then remove href, then add back xlink:href.
>   (2) remove href.
> 
> Right now, it sounds like the first one would have us load def.js and the
> second would not -- but they should behave the same (both loading def.js), I
> think.
> 

I was trying to make sure if removeAttribute really works for xlink:href based on our current m-c.
e.g.
<script xlink:href="def.js" id='s'/>

var script = document.getElementById('s');
assert_equal(script.href.baseVal, 'def.js');  // success

script.removeAttribute('xlink:href');
assert_equal(script.href.baseVal, '');  // success


However, when I tried to test the context of the loaded script, it failed.
e.g.

in def.js:
function hello() { return 'hello'; }

in xxx.html:
<script xlink:href="def.js" id='s'/>

var script = document.getElementById('s');
assert_equal(!!hello, true);  // sucess

script.removeAttribute('xlink:href');
// wait for a while, about 10 frames
assert_equal(!!hello, false);  // failed

My question is: when removing an external script, I think that we don't really clear the context of the loaded script based on current m-c, right?

> Similarly, given <script href="abc.js">, I'd expect the following two
> sequences to have the same result:
>  (1) remove href, then add xlink:href
>  (2) add xlink:href, then remove href

Therefore, according to comment 142, looks like clearing the context of the old external script in these cases when removing href/xlink:href is necessary. I checked nsScriptElement and nsScriptLoader, and looks like we don't support clearing the loaded external script, so could you please give me some hints/tips to do this? Thank you very much.
Flags: needinfo?(dholbert)
Sorry, I'm not really familiar with <script> handling & representation of the script contexts you mentioned, so I'm afraid I don't have any hints/tips on that.

(My main concern from the perspective of this bug is that we should produce consistent results when following different sets of steps that produce the same eventual DOM / attribute-values.  That's where my comment 142 came from - not from any particular expertise I have in <script> handling.)
Flags: needinfo?(dholbert)
dropping a script href or src does not unload a script so I think that's fine.

>  (1) remove href, then add xlink:href
>  (2) add xlink:href, then remove href

In both of these cases you should have both scripts loaded afterwards.
(In reply to Robert Longson from comment #146)
> dropping a script href or src does not unload a script so I think that's
> fine.
> 
> >  (1) remove href, then add xlink:href
> >  (2) add xlink:href, then remove href
> 
> In both of these cases you should have both scripts loaded afterwards.

Thanks for explanation, Robert. I thought we have to unload the removed script, and looks like I misunderstood this. I will focus on making sure we have consistent results in both cases.
Comment on attachment 8779647 [details]
Bug 1245751 - Part 15: Add tests for a, script, mpath, and animate elements.

Hi, Daniel,

Could you please take a look at href-script-element.html? I added two promise_test into it.

(P.S. I use promise_test, instead of async_test, so I can make sure each test starts after the previous one is done.)
Attachment #8779647 - Flags: review+ → review?(dholbert)
Comment on attachment 8779647 [details]
Bug 1245751 - Part 15: Add tests for a, script, mpath, and animate elements.

https://reviewboard.mozilla.org/r/70614/#review71490

r=me on the test changes - nits below:

::: testing/web-platform/tests/svg/linking/scripted/href-animate-element.html:10
(Diff revisions 4 - 5)
>  <script src='/resources/testharnessreport.js'></script>
>  <script src='testcommon.js'></script>
>  <body>
>  <div id='log'></div>
> -<svg id='svg' width='100' height='100' viewBox='0 0 100 100'></svg>
> +<svg id='svg' width='100' height='100' viewBox='0 0 100 100'
> +     xmlns:xlink='http://www.w3.org/1999/xlink'></svg>

This change [made to several .html files in this revision], declaring <svg...xmlns:xlink=...>, should be unnecessary -- you shouldn't need to explicitly declare xmlns:xlink for <svg> inside of <!DOCTYPE html>.

So if you like, feel free to remove this attribute (to simplify the test).

(Or if you'd rather, it's fine with me to keep this as-is, too.)

::: testing/web-platform/tests/svg/linking/scripted/href-script-element.html:50
(Diff revisions 4 - 5)
> +
> +    script.removeAttribute('href');
> +    return waitEvent(script, 'load');
> +  }).then(function() {
> +    assert_equals(script.href.baseVal, 'testScripts/externalScript2.js');
> +    assert_equals(!!script1, true, 'script1 is still loaded');

I haven't used testharness.js much, but I think this wants to be "assert_true(script1, ...)"

Same goes for all the other assert_equals(!!whatever, true, ...) lines in this patch.

::: testing/web-platform/tests/svg/linking/scripted/href-script-element.html:83
(Diff revisions 4 - 5)
> +    assert_equals(!!script2, true, 'script2 is loaded');
> +    assert_equals(loadedScript(), 'externalScript2',
> +                  'Link to the external script by xlink:href');
> +  });
> +}, 'Test for loading external script from xlink:href by removing href and ' +
> +   'then adding xlink:href');

Could you add one more promise_test here, to exercise one more edge case? I think we should test this sequence of steps:
 - set xlink:href (and make sure that takes effect)
 - set href, and make sure that overrides the earlier xlink:href setting.

I don't think your other tests here cover that particular sequence, with href stepping in and supplanting a preexisting xlink:href.

::: testing/web-platform/tests/svg/linking/scripted/testcommon.js:33
(Diff revisions 4 - 5)
>    });
>    return elem;
>  }
> +
> +/**
> + * Create a Promise object for a specific event.

"for a specific event" is vague here.

Please replace with:
"which resolves when a specific event fires"

(With that change and your subsequent @param documentation, the behavior is clear I think.)
Attachment #8779647 - Flags: review?(dholbert) → review+
Comment on attachment 8768718 [details]
Bug 1245751 - Part 9: Allow href without xlink on SVG <script> elements.

https://reviewboard.mozilla.org/r/62770/#review71528

::: dom/svg/SVGScriptElement.cpp:222
(Diff revisions 4 - 7)
> +      // In order to force to load the script, we have to re-initialize
> +      // mAlreadyStarted flag, and disable mFrozen state.
> +      if (aNamespaceID == kNameSpaceID_None &&
> +          mStringAttributes[XLINK_HREF].IsExplicitlySet()) {
> +        mAlreadyStarted = false;
> +        mFrozen = false;

I can't find any other place in the codebase where we reset mAlreadyStarted[1].  So, it concerns me a bit that we need to reset it (and mFrozen) here.

For example: suppose we simply have....
  <script xlink:href="foo.js">
...and we clear xlink:href and then set it to to "bar.js".  How do we make sure bar.js gets loaded right now, in that case? (It looks to me like we don't reset mAlreadyStarted for that scenario -- or if we do, I'm not seeing where that happens.)

[1] my search for potential clearings of this variable:
https://dxr.mozilla.org/mozilla-central/search?q=mAlreadyStarted

::: dom/svg/SVGScriptElement.cpp:234
(Diff revisions 4 - 7)
> +      //    might set href before, or never set it.
> +      if (aNamespaceID == kNameSpaceID_None ||
> +          (aNamespaceID == kNameSpaceID_XLink &&
> +           !mStringAttributes[HREF].IsExplicitlySet())) {
> +        mAlreadyStarted = false;
> +        mFrozen = false;

Both of the "special cases" here actually do the same thing -- they set these two obscure bool member-vars to false.

SO: for understandability & maintainability (if we realize there's something else we have to do in this case), I'd prefer we *merge* these two (admittedly-short) special cases into a single "if" statement, and refactor your existing logic to control a nicely-named local boolean.  And then if that boolean is true, then clear whatever we need to clear.

Something like:

    // Indicates whether this attribute-change should make us switch between
    // honoring the "href" vs. "xlink:href" attribute:
    bool isChangingWhichHrefIsHonored = false;
    if (whatever) {
      // [explanation of this scenario]
      isChangingWhichHrefIsHonored = true;
    } else {
      // [explanation of this other scenario]
      isChangingWhichHrefIsHonored = true;
    }

    if (isChangingWhichHrefIsHonored) {
      // [explanation for why these bools need tweaks]
      mAlreadyStarted = false;
      mFrozen = false;
    }

(Note that, per my other comment above, I'm still unclear on why these particular bools need tweaking here, when they don't (?) get tweaked during the normal single-attribute set/clear/set-again codepath.  My sample-code here is tweaking them simply because that's what your patch does, but I'm not yet convinced that's the right thing to do.)
Flags: needinfo?(boris.chiou)
Comment on attachment 8779647 [details]
Bug 1245751 - Part 15: Add tests for a, script, mpath, and animate elements.

https://reviewboard.mozilla.org/r/70614/#review71490

> This change [made to several .html files in this revision], declaring <svg...xmlns:xlink=...>, should be unnecessary -- you shouldn't need to explicitly declare xmlns:xlink for <svg> inside of <!DOCTYPE html>.
> 
> So if you like, feel free to remove this attribute (to simplify the test).
> 
> (Or if you'd rather, it's fine with me to keep this as-is, too.)

Thanks for explanation, I will remove xmlns:xlink from this .html files.

> I haven't used testharness.js much, but I think this wants to be "assert_true(script1, ...)"
> 
> Same goes for all the other assert_equals(!!whatever, true, ...) lines in this patch.

Oh yes, I should use assert_true().

> Could you add one more promise_test here, to exercise one more edge case? I think we should test this sequence of steps:
>  - set xlink:href (and make sure that takes effect)
>  - set href, and make sure that overrides the earlier xlink:href setting.
> 
> I don't think your other tests here cover that particular sequence, with href stepping in and supplanting a preexisting xlink:href.

OK, I didn't cover this edge case. I will add this test.

> "for a specific event" is vague here.
> 
> Please replace with:
> "which resolves when a specific event fires"
> 
> (With that change and your subsequent @param documentation, the behavior is clear I think.)

Got it. Thanks.
Comment on attachment 8768718 [details]
Bug 1245751 - Part 9: Allow href without xlink on SVG <script> elements.

https://reviewboard.mozilla.org/r/62770/#review71528

> I can't find any other place in the codebase where we reset mAlreadyStarted[1].  So, it concerns me a bit that we need to reset it (and mFrozen) here.
> 
> For example: suppose we simply have....
>   <script xlink:href="foo.js">
> ...and we clear xlink:href and then set it to to "bar.js".  How do we make sure bar.js gets loaded right now, in that case? (It looks to me like we don't reset mAlreadyStarted for that scenario -- or if we do, I'm not seeing where that happens.)
> 
> [1] my search for potential clearings of this variable:
> https://dxr.mozilla.org/mozilla-central/search?q=mAlreadyStarted

Actually, I have another idea. Instead of tweaking mAlreadStarted and mFrozen, I introduce a new argument to MaybeProcessScript, aForce. e.g. MaybeProcessScript(bool aForce = false). Although the function name has "Maybe", we still can force it to try to load the script. Hence, we can set this flag and tell MaybeProcessScript to try to force to load the new script.

> Both of the "special cases" here actually do the same thing -- they set these two obscure bool member-vars to false.
> 
> SO: for understandability & maintainability (if we realize there's something else we have to do in this case), I'd prefer we *merge* these two (admittedly-short) special cases into a single "if" statement, and refactor your existing logic to control a nicely-named local boolean.  And then if that boolean is true, then clear whatever we need to clear.
> 
> Something like:
> 
>     // Indicates whether this attribute-change should make us switch between
>     // honoring the "href" vs. "xlink:href" attribute:
>     bool isChangingWhichHrefIsHonored = false;
>     if (whatever) {
>       // [explanation of this scenario]
>       isChangingWhichHrefIsHonored = true;
>     } else {
>       // [explanation of this other scenario]
>       isChangingWhichHrefIsHonored = true;
>     }
> 
>     if (isChangingWhichHrefIsHonored) {
>       // [explanation for why these bools need tweaks]
>       mAlreadyStarted = false;
>       mFrozen = false;
>     }
> 
> (Note that, per my other comment above, I'm still unclear on why these particular bools need tweaking here, when they don't (?) get tweaked during the normal single-attribute set/clear/set-again codepath.  My sample-code here is tweaking them simply because that's what your patch does, but I'm not yet convinced that's the right thing to do.)

This patch changes the single-attribute set/clear/set-again codepath, and it may have some potential problems. Let me think it more and try to fix this. Thanks.
Comment on attachment 8768718 [details]
Bug 1245751 - Part 9: Allow href without xlink on SVG <script> elements.

https://reviewboard.mozilla.org/r/62770/#review71528

> Actually, I have another idea. Instead of tweaking mAlreadStarted and mFrozen, I introduce a new argument to MaybeProcessScript, aForce. e.g. MaybeProcessScript(bool aForce = false). Although the function name has "Maybe", we still can force it to try to load the script. Hence, we can set this flag and tell MaybeProcessScript to try to force to load the new script.

MaybeProcessScript uses mAlreadyStarted to check if we really need to process the script, but we may need to force to process the script if we try to manipulate href or xlink:href attribute. Tweaking mAlreadyStarted and mFrozen, or adding a different data member/function argurment is a simple way to achieve this purpose. Another way is adding a different flag, or factor out the ProcessScript from MaybeProcessScript, so we can call ProcessScript directly, instead of MaybeProcessScript. I am not sure which one is better now.
Flags: needinfo?(boris.chiou)
Attachment #8768718 - Flags: review?(dholbert)
Attachment #8768718 - Flags: review+
(In reply to Daniel Holbert [:dholbert] from comment #158)
> (Note that, per my other comment above, I'm still unclear on why these
> particular bools need tweaking here, when they don't (?) get tweaked during
> the normal single-attribute set/clear/set-again codepath.  My sample-code
> here is tweaking them simply because that's what your patch does, but I'm
> not yet convinced that's the right thing to do.)

Looks like the problem happens on mAlreadyStarted. I think we have to do something different for HTML5 and SVG, so I'd like to factor out the setting/getting of mAlreadyStarted in MaybeProcessScript() and write a different setting/getting for SVG Script element, e.g. We need two more mAlreadyStartedxxx flags, one for href, and another one for xlink:href.
(In reply to Daniel Holbert [:dholbert] from comment #158)
> For example: suppose we simply have....
>   <script xlink:href="foo.js">
> ...and we clear xlink:href and then set it to to "bar.js".  How do we make
> sure bar.js gets loaded right now, in that case? (It looks to me like we
> don't reset mAlreadyStarted for that scenario -- or if we do, I'm not seeing
> where that happens.)

I think we should not reload href or xlink:href for single attribute setting/removing/setting-again. The current patch will reload href, and I think it is a bug because the comment of MaybeProcessScript() says "Once it has been evaluated there is no way to make it reevaluate the script" [1]. However, when we set a different attribute, it should load it.

[1] http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/dom/base/nsIScriptElement.h#248-252

e.g.
<script xlink:href="foo.js">

script.removeAttribute('xlink:href');
script.setAttributeNS(..., 'xlink:href', 'bar.js');  // don't re-load, nothing happened
script.setAttribute('href', 'aaa.js');               // load aaa.js
// now, we load both script files, and we will *not* load any script more even if you remove and set it again.

script.removeAttribute('href');                      // nothing happened
script.setAttribute('href', 'bbb.js');               // nothing happened


The spec of SVG <script> doesn't say anything about this, so I think this could the most reasonable behavior for supporting both href and xlink:href. Does it make sense?

Thanks.
Flags: needinfo?(dholbert)
Thanks for looking into (& explaining) that! Sounds like the "nothing happened" is the behavior we should be matching here, then.  (To the extent possible, we should make special cases with xlink:href/href behave like similar cases with setting/unsetting/updating xlink:href on its own in the current Firefox version.)
Flags: needinfo?(dholbert)
(It's still useful to keep all the new complex <script xlink:href=... href=...> testcases you've added, though! Their expected results would just need to be updated to be a bit less interesting. :))
(In reply to Daniel Holbert [:dholbert] from comment #164)
> Thanks for looking into (& explaining) that! Sounds like the "nothing
> happened" is the behavior we should be matching here, then.  (To the extent
> possible, we should make special cases with xlink:href/href behave like
> similar cases with setting/unsetting/updating xlink:href on its own in the
> current Firefox version.)

Oops, do you mean we don't need to "reload" the href or xlink:href script? Therefore, the behavior is like the single attribute setting/unsetting/updating xlink:href? If so, the implementation will be very simple, and I will be very happy. :)

e.g.
1)
<script xlink:href="abc.js" href="def.js" id='s'> 

var script = document.getElementById('s');
script.removeAttribute('xlink:href'); // nothing happened, because we loaded abc.js already. just like single attribute behaves.

2)
<script id='s'>
var script = document.getElementById('s');
script.setAttributeNS(..., 'xlink:href', 'def.js'); // load def.js
script.setAttribute(..., 'href', 'abc.js');         // nothing happend, because we loaded def.js already.


Sorry, I want to make sure which behavior is more reasonable and more suitable for current firefox version because our current implementation doesn't want us reload any script and it freezes the script after loading it, so we can get the script line number and other information correctly. But it makes the implementation becomes much harder to support "reloading a new script".
Sorry for asking you again and again. Thanks.
Flags: needinfo?(dholbert)
According to current design for nsIScriptElement.h on Firefox, it's better to load one external script per <script> element [1], and after checking the current implementation for HTML <script> and SVG <script> element, I think we shouldn't load multiple external scripts on a <script> element; otherwise, the design becomes complicated and we may have some potential async problems (even though SVG script element doesn't support async flag). If we need to support multiple external scripts (and the spec suggests to do so), we can file another bug to fix it.

[1] http://searchfox.org/mozilla-central/rev/44f6964ba95b8ddd8ebf70c55b34cd2323afeef4/dom/base/nsIScriptElement.h#247-252
(In reply to Daniel Holbert [:dholbert] from comment #165)
> (It's still useful to keep all the new complex <script xlink:href=...
> href=...> testcases you've added, though! Their expected results would just
> need to be updated to be a bit less interesting. :))

Yes, I will try to do some revisions on these tests to reflect the change. Thanks.
(In reply to Boris Chiou [:boris] from comment #166)
> Oops, do you mean we don't need to "reload" the href or xlink:href script?

I'm not sure, without doing extensive testing myself.  I don't have any particular knowledge about <script> behavior, and I came into this bug quite late in its history so I may be missing some history as well. :)

I'll just reiterate comment 145 and comment 164 -- i.e. it seems intuitively like (for all of these elements), set/unset operations on xlink:href & href attributes *should have the same effect* as the equivalent operations would've had on the single attribute that we've always supported.  (where "equivalent" takes precedence into account, of course).  So:
 - if we have "href", and we get an setAttribute for "xlink:href", then nothing should happen.
 - if we have "xlink:href", and we get a setAttribute for "href", then we should do *whatever we would've done* if xlink:href had been updated (and maybe that's also "nothing happens").  Since the assignment to href is basically overriding/updating the href target.

Does that makes sense?
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #169)
> (In reply to Boris Chiou [:boris] from comment #166)
> > Oops, do you mean we don't need to "reload" the href or xlink:href script?
> 
> I'm not sure, without doing extensive testing myself.  I don't have any
> particular knowledge about <script> behavior, and I came into this bug quite
> late in its history so I may be missing some history as well. :)
> 
> I'll just reiterate comment 145 and comment 164 -- i.e. it seems intuitively
> like (for all of these elements), set/unset operations on xlink:href & href
> attributes *should have the same effect* as the equivalent operations
> would've had on the single attribute that we've always supported.  (where
> "equivalent" takes precedence into account, of course).  So:
>  - if we have "href", and we get an setAttribute for "xlink:href", then
> nothing should happen.
>  - if we have "xlink:href", and we get a setAttribute for "href", then we
> should do *whatever we would've done* if xlink:href had been updated (and
> maybe that's also "nothing happens").  Since the assignment to href is
> basically overriding/updating the href target.
> 
> Does that makes sense?

Yes. IMO, I think <script> element is a little bit different from other kinds of elements - it will be frozen after loaded. i.e. it is not like animation elements, which can be updated many times. In addition, I checked the current implementation for xlink:href, and it seems setting/unsetting/setting-again xlink:href attribute will load only once. This is why I left comment 167. Therefore, I think "nothing happens" in these two cases you mentioned in comment 169 should be the most reasonable way. If you want to load more scripts, please add more script elements, instead of setting an extra href attribute. I'd like to update my patch and test cases according to this conclusion, "no matter what you set, we only load one external script per <script> element". Thank you very much.
That sounds good to me! Thanks.
(In reply to Boris Chiou [:boris] from comment #170)
> (In reply to Daniel Holbert [:dholbert] from comment #169)
> > (In reply to Boris Chiou [:boris] from comment #166)
> > > Oops, do you mean we don't need to "reload" the href or xlink:href script?
> > 
> > I'm not sure, without doing extensive testing myself.  I don't have any
> > particular knowledge about <script> behavior, and I came into this bug quite
> > late in its history so I may be missing some history as well. :)
> > 
> > I'll just reiterate comment 145 and comment 164 -- i.e. it seems intuitively
> > like (for all of these elements), set/unset operations on xlink:href & href
> > attributes *should have the same effect* as the equivalent operations
> > would've had on the single attribute that we've always supported.  (where
> > "equivalent" takes precedence into account, of course).  So:
> >  - if we have "href", and we get an setAttribute for "xlink:href", then
> > nothing should happen.
> >  - if we have "xlink:href", and we get a setAttribute for "href", then we
> > should do *whatever we would've done* if xlink:href had been updated (and
> > maybe that's also "nothing happens").  Since the assignment to href is
> > basically overriding/updating the href target.
> > 
> > Does that makes sense?
> 
> Yes. IMO, I think <script> element is a little bit different from other
> kinds of elements - it will be frozen after loaded. i.e. it is not like
> animation elements, which can be updated many times. In addition, I checked
> the current implementation for xlink:href, and it seems
> setting/unsetting/setting-again xlink:href attribute will load only once.
> This is why I left comment 167. Therefore, I think "nothing happens" in
> these two cases you mentioned in comment 169 should be the most reasonable
> way. If you want to load more scripts, please add more script elements,
> instead of setting an extra href attribute. I'd like to update my patch and
> test cases according to this conclusion, "no matter what you set, we only
> load one external script per <script> element". Thank you very much.

And the results by performing the sequences in comment 142 are still consistent.
Comment on attachment 8779647 [details]
Bug 1245751 - Part 15: Add tests for a, script, mpath, and animate elements.

Hi Daniel,
Still need you to check href-script-element.html and href-script-element-markup.html again. Thanks.
Attachment #8779647 - Flags: review+ → review?(dholbert)
Comment on attachment 8779647 [details]
Bug 1245751 - Part 15: Add tests for a, script, mpath, and animate elements.

https://reviewboard.mozilla.org/r/70614/#review72948

r- for the time being, for arbitrary-millisecond setTimeout concerns expressed below (I'm hoping we can avoid those).

::: testing/web-platform/tests/svg/linking/scripted/href-script-element-markup.html:24
(Diff revision 6)
> +test(function(t) {
> +  var script = document.getElementById('script');
> +  assert_equals(script.href.baseVal, 'testScripts/externalScript1.js');
> +  assert_equals(loadedScript(), 'externalScript1');
> +}, 'Test for loading external script by markup when setting both href and ' +
> +   ' xlink:href');

Drop the space between the single-quote and xlink:href on this line. (You've already got a space at the end of the previous line's string.)

::: testing/web-platform/tests/svg/linking/scripted/href-script-element.html:17
(Diff revision 6)
> +'use strict';
> +
> +// Note:
> +// 1. The order of these tests shouldn't be changed because we don't unload
> +// the external script file even if removing the <script> element by
> +// removeAttribute(). So I intentionally make them load externalScript1 and

This phrase doesn't make sense & looks like a typo:
 "...even if removing the <script> element by removeAttribute()"

In particular -- there's no way the <script> *element* would get "removed" by a removeAttribute() call.  The element would still exist in the document; it just would lack a particular attribute. So, this phrase doesn't make sense.

Maybe you meant s/removing/modifying/? That would make a bit more sense, I think.

::: testing/web-platform/tests/svg/linking/scripted/href-script-element.html:21
(Diff revision 6)
> +// the external script file even if removing the <script> element by
> +// removeAttribute(). So I intentionally make them load externalScript1 and
> +// externalScript2 alternately, and we can check if the results are changed
> +// after reloading the other script.
> +//
> +// 2. Use setTimeout(..., 32) to check "nothing happens" because I think 32ms

The reliance on setTimeout(..., 32) feels problematic here, and it is strongly discouraged in our tests, in general. (See e.g. bug 649012.)  We can't assume that any arbitrary setTimeout time-value is "correct" here. (And even if 32ms happened to be the exact right threshold for *your* platform, the test probably wouldn't catch real failures on slower platforms like Android emulator, and it would waste time & take longer than it needs to on fast platforms where the load would've happened much sooner.)

Occasionally/rarely, setTimeout() is unavoidably necessary for a test -- but I don't think that's the case here.

This might be a more reliable/justifiable way of determining that "nothing happened": Since the thing we're watching out for is an unwanted script-load, let's just intentionally trigger *another* script-load, and wait for that to happen, and then check if the original (unwanted) script load happened as well.  Something like this:
 0) Modify href/xlink:href/etc on the main <script> element.
 1) create a *new* <script> element href="some-other-JS-file". (I think this will trigger some-other-js-file to be loaded...?)
 2) wait for that new script to load (either via "onload" or via some JS inside of the script that gets run when it's loaded)
 3) At that point, assert that "nothing happened" for the original <script> element that we *actually* care about testing.

Would that work?

(As you note later on in this comment, requestAnimationFrame() is an alternative, but I don't think you can guarantee that a script would have been loaded in any arbitrary number of requestAnimationFrame() calls.  It seems strictly better to just use another actual script-load as our benchmark for "how long do we have to wait to decide that this script didn't load".)
Attachment #8779647 - Flags: review?(dholbert) → review-
So, for Part 9, I'm reviewing the latest version as compared to revision 4, to skip all the intermediate versions where we were adding ultimately-unwanted extra loads.  i.e. I'm using this interdiff:
 https://reviewboard.mozilla.org/r/62770/diff/4-8/

As you can see from that ^ interdiff link, the only new thing there is this new "mLinkingSrc" attribute. Can you explain what problem you're trying to solve with that member-variable?  I think I see what problem you're trying to solve, but correct me if I'm wrong...

So, I can see this member-variable helping in a very narrow set of cases -- for example, it'd help here:
 CASE 1:
  - author sets <script xlink:href="...">
  - author later sets the "href" attribute as well, for some reason.
  - Author queries elem.href().
Here, "mLinkSrc" would tell us to return the xlink:href attribute-value, which maybe is what the author expects since that's the script that executed.

BUT, there are plenty of cases where the author could do similar things where href() would *NOT* return the script that executed.  So it seems arbitrary that we're trying to help in just that one specific scenario, when we can't/shouldn't help in other very-similar scenarios.

For example, mLinkSrc doesn't help us at all in this case:
 CASE 2:
  - Author sets <script xlink:href="...">
  - Author updates xlink:href to some other string value.
  - Author queries elem.href().
I think here, they'll get the new string value, which is not the script that is actually running.

It also doesn't help in this case:
  - Author sets <script href="a" xlink:href="b">
  - Author unsets "href" attribute.
  - Author queries href().
Here, they'll get the empty string, I think (?), because mLinkSrc will dutifully tell us that's where our script came from, even though that attribute's no longer explicitly set.

ALSO, in static cases where the author doesn't do any silly dynamic tweaks to their <script> element's attributes (or at least doesn't set conflicting values), we should be just fine without needing mLinkSrc.

SO, BASICALLY: To the extent that we try to make the JS-exposed href() API do anything fancy here, the behavior needs to be very carefully thought through, and it would need to be specced.  I don't see any useful spec-text at https://www.w3.org/TR/SVG2/interact.html#ScriptElementHrefAttribute that would support the behavior in the latest revision of Part 9, so at this point, I suspect Revision 4 of that patch (the version r+'d by jwatt) is preferable to the latest revision.
(In reply to Daniel Holbert [:dholbert] from comment #190)
> So, for Part 9, I'm reviewing the latest version as compared to revision 4,
> to skip all the intermediate versions where we were adding
> ultimately-unwanted extra loads.  i.e. I'm using this interdiff:
>  https://reviewboard.mozilla.org/r/62770/diff/4-8/
> 
> As you can see from that ^ interdiff link, the only new thing there is this
> new "mLinkingSrc" attribute. Can you explain what problem you're trying to
> solve with that member-variable?  I think I see what problem you're trying
> to solve, but correct me if I'm wrong...
> 
> So, I can see this member-variable helping in a very narrow set of cases --
> for example, it'd help here:
>  CASE 1:
>   - author sets <script xlink:href="...">
>   - author later sets the "href" attribute as well, for some reason.
>   - Author queries elem.href().
> Here, "mLinkSrc" would tell us to return the xlink:href attribute-value,
> which maybe is what the author expects since that's the script that executed.

Yes, this case is what I want to fix.

> 
> BUT, there are plenty of cases where the author could do similar things
> where href() would *NOT* return the script that executed.  So it seems
> arbitrary that we're trying to help in just that one specific scenario, when
> we can't/shouldn't help in other very-similar scenarios.
> 
> For example, mLinkSrc doesn't help us at all in this case:
>  CASE 2:
>   - Author sets <script xlink:href="...">
>   - Author updates xlink:href to some other string value.
>   - Author queries elem.href().
> I think here, they'll get the new string value, which is not the script that
> is actually running.

This is another problem. At first, I want to store the original string, but after checking the current implementation of href() API [1], it always returns the newest string of xlink:href, so I think it would be ok in this case.

[1] http://searchfox.org/mozilla-central/rev/0dd403224a5acb0702bdbf7ff405067f5d29c239/dom/svg/SVGScriptElement.cpp#112

> 
> It also doesn't help in this case:
>   - Author sets <script href="a" xlink:href="b">
>   - Author unsets "href" attribute.
>   - Author queries href().
> Here, they'll get the empty string, I think (?), because mLinkSrc will
> dutifully tell us that's where our script came from, even though that
> attribute's no longer explicitly set.

Yes, it returns an empty string. Just like our current implementation [1], if we unset xlink:href, and then query href(), we always return an empty string. 

> 
> ALSO, in static cases where the author doesn't do any silly dynamic tweaks
> to their <script> element's attributes (or at least doesn't set conflicting
> values), we should be just fine without needing mLinkSrc.
> 
> SO, BASICALLY: To the extent that we try to make the JS-exposed href() API
> do anything fancy here, the behavior needs to be very carefully thought
> through, and it would need to be specced.  I don't see any useful spec-text
> at https://www.w3.org/TR/SVG2/interact.html#ScriptElementHrefAttribute that
> would support the behavior in the latest revision of Part 9, so at this
> point, I suspect Revision 4 of that patch (the version r+'d by jwatt) is
> preferable to the latest revision.

I know your concerns, but what I think here is: have a similar behavior with the current firefox implementation. If we use Revision 4, case 1 will return the string from href, not from xlink:href, this may be weird. I'm not sure which implementation is better because just as you mentioned, the spec doesn't say anything about this, so I think we can choose the one which we both agree with. If Revision 4 is preferable, we can land revision 4 now.
Thank you.
Comment on attachment 8779647 [details]
Bug 1245751 - Part 15: Add tests for a, script, mpath, and animate elements.

https://reviewboard.mozilla.org/r/70614/#review72948

> This phrase doesn't make sense & looks like a typo:
>  "...even if removing the <script> element by removeAttribute()"
> 
> In particular -- there's no way the <script> *element* would get "removed" by a removeAttribute() call.  The element would still exist in the document; it just would lack a particular attribute. So, this phrase doesn't make sense.
> 
> Maybe you meant s/removing/modifying/? That would make a bit more sense, I think.

Sorry. I shouldn't write "removeAttribute()". Just want to say that removing <script> element by GC and script.remove(), which is defined in testcommon.js, after the current test has been finished.

> The reliance on setTimeout(..., 32) feels problematic here, and it is strongly discouraged in our tests, in general. (See e.g. bug 649012.)  We can't assume that any arbitrary setTimeout time-value is "correct" here. (And even if 32ms happened to be the exact right threshold for *your* platform, the test probably wouldn't catch real failures on slower platforms like Android emulator, and it would waste time & take longer than it needs to on fast platforms where the load would've happened much sooner.)
> 
> Occasionally/rarely, setTimeout() is unavoidably necessary for a test -- but I don't think that's the case here.
> 
> This might be a more reliable/justifiable way of determining that "nothing happened": Since the thing we're watching out for is an unwanted script-load, let's just intentionally trigger *another* script-load, and wait for that to happen, and then check if the original (unwanted) script load happened as well.  Something like this:
>  0) Modify href/xlink:href/etc on the main <script> element.
>  1) create a *new* <script> element href="some-other-JS-file". (I think this will trigger some-other-js-file to be loaded...?)
>  2) wait for that new script to load (either via "onload" or via some JS inside of the script that gets run when it's loaded)
>  3) At that point, assert that "nothing happened" for the original <script> element that we *actually* care about testing.
> 
> Would that work?
> 
> (As you note later on in this comment, requestAnimationFrame() is an alternative, but I don't think you can guarantee that a script would have been loaded in any arbitrary number of requestAnimationFrame() calls.  It seems strictly better to just use another actual script-load as our benchmark for "how long do we have to wait to decide that this script didn't load".)

Thanks. I also don't like to use setTimeout. I will add a emptyScript.js (no content) to test this. It's really helpful.
(In reply to Boris Chiou [:boris] from comment #191)
> I'm not sure
> which implementation is better because just as you mentioned, the spec
> doesn't say anything about this, so I think we can choose the one which we
> both agree with. If Revision 4 is preferable, we can land revision 4 now.
> Thank you.

Yeah, I think revision 4 of part 9 is best here.

> Thanks. I also don't like to use setTimeout. I will add a emptyScript.js (no content) to test this. It's really helpful.

Thanks! But, once concern on this -- I worry that a *completely* empty script might load too quickly, and hence wouldn't be an effective "timeout".  It could easily "fire" too soon, if e.g. the file-read operation completes immediately while the other script's file-read takes a few ticks due to hard-disk contention or something.

Ideally, we should use a script that's similar in size/structure to the script(s) we're hoping not to load (scripts 1 and 2), so that it's likely to be an effective measuring-stick.
Comment on attachment 8779647 [details]
Bug 1245751 - Part 15: Add tests for a, script, mpath, and animate elements.

https://reviewboard.mozilla.org/r/70614/#review73272

r=me with the following addressed.

::: testing/web-platform/tests/svg/linking/scripted/href-script-element.html:20
(Diff revisions 6 - 7)
> -// 1. The order of these tests shouldn't be changed because we don't unload
> -// the external script file even if removing the <script> element by
> -// removeAttribute(). So I intentionally make them load externalScript1 and
> +// The order of these tests shouldn't be changed because we don't unload
> +// the external script file even if we expect the <script> element will be
> +// removed by childNode.remove() and Garbage Collection after a test has been
> +// finished. Therefore, I intentionally make them load externalScript1 and
>  // externalScript2 alternately, and we can check if the results are changed
>  // after reloading the other script.

Please add a comment here to clarify the "dummy" script-loads throughout this test. Something like this perhaps:

    // Throughout this test, we periodically need to verify that a script
    // *does not load* after we've made a tweak.  To do that, we have to
    // wait "long enough for it to have loaded", and then make sure nothing
    // has changed.  We estimate "long enough" by adding an extra dummy
    // <script> element and watching for its load event.

::: testing/web-platform/tests/svg/linking/scripted/href-script-element.html:38
(Diff revisions 6 - 7)
>  
>      script.setAttributeNS(XLINKNS, 'xlink:href',
>                            'testScripts/externalScript2.js');
> -    return new Promise(function(resolve) {
> -      window.setTimeout(resolve, 32);
> -    });
> +
> +    // Load an empty script to trigger a load event.
> +    var emptyScript = createSVGElement(t, 'script', svg);

s/empty/dummy/ on the comment, variable name, and script filename.  (since it's problematic for it be empty, per comment 200.)

This gets repeated several times.

::: testing/web-platform/tests/svg/linking/scripted/href-script-element.html:70
(Diff revisions 6 - 7)
> -    });
> +    var emptyScript = createSVGElement(t, 'script', svg);
> +    emptyScript.setAttribute('href', 'testScripts/emptyScript.js');
> +    return waitEvent(emptyScript, 'load');
>    }).then(function() {
> -    assert_equals(script.href.baseVal, 'testScripts/externalScript2.js');
> +    assert_equals(script.href.baseVal, 'testScripts/externalScript1.js',
> +                  'href() returns href attribute');

Please clarify this test message, to something like the following:
 "href() should prefer href attribute over xlink:href"

...to make it slightly clearer what you're asserting here (i.e. what the expectation is about the tested value, & why)

::: testing/web-platform/tests/svg/linking/scripted/href-script-element.html:94
(Diff revisions 6 - 7)
>                    'Link to the external script by href');
>  
>      script.removeAttribute('href');
> -    assert_equals(script.href.baseVal, '',
> -                  'Remove loaded href so this API return empty string');
> -    return new Promise(function(resolve) {
> +    assert_equals(script.href.baseVal, 'testScripts/externalScript2.js',
> +                  'href() returns xlink:href attribute because it is still ' +
> +                  'set');

s/because it is still set/because href was unset/

(It doesn't matter whether xlink:href is "still set", when deciding which attribute we return here. What matters is that href is *not* set.)
Attachment #8779647 - Flags: review?(dholbert) → review+
Comment on attachment 8768718 [details]
Bug 1245751 - Part 9: Allow href without xlink on SVG <script> elements.

https://reviewboard.mozilla.org/r/62770/#review73300

[I'm canceling the review request for me on Part 9, since it looks like the latest revision there is the same as revision 4, which has already been r+'d by jwatt. I do think that's the right version of Part 9 that we should take here, as I've expressed above, and I don't think it needs another round of review; r=jwatt is sufficient.]

[I also am not wanting to mark it r+ myself, since I haven't really reviewed it -- for all of your revisions of Part 9 that I've reviewed here, I've been reviewing the *interdiff from* revision 4, to avoid re-reviewing already-reviewed-by-jwatt changes stuff.  The latest revision has an empty interdiff with respect to part 4 -- and r+'ing an empty interdiff doesn't seem right. :)]
Attachment #8768718 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #202)
> Comment on attachment 8768718 [details]
> Bug 1245751 - Part 9: Allow href without xlink on SVG <script> elements.
> 
> https://reviewboard.mozilla.org/r/62770/#review73300
> 
> [I'm canceling the review request for me on Part 9, since it looks like the
> latest revision there is the same as revision 4, which has already been r+'d
> by jwatt. I do think that's the right version of Part 9 that we should take
> here, as I've expressed above, and I don't think it needs another round of
> review; r=jwatt is sufficient.]
> 
> [I also am not wanting to mark it r+ myself, since I haven't really reviewed
> it -- for all of your revisions of Part 9 that I've reviewed here, I've been
> reviewing the *interdiff from* revision 4, to avoid re-reviewing
> already-reviewed-by-jwatt changes stuff.  The latest revision has an empty
> interdiff with respect to part 4 -- and r+'ing an empty interdiff doesn't
> seem right. :)]

And really thanks for discussing part 9 with me. :)
Comment on attachment 8768718 [details]
Bug 1245751 - Part 9: Allow href without xlink on SVG <script> elements.

carry jwatt's r+
Attachment #8768718 - Flags: review?(dholbert) → review+
Attachment #8768718 - Flags: review+
Attachment #8768718 - Flags: review+
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d8f2b7f87e62
Part 1: Allow href without xlink on SVG <a> elements. r=dholbert,jwatt
https://hg.mozilla.org/integration/autoland/rev/956acf17e34a
Part 2: Allow href without xlink on SVG <use> elements. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/762f8d60e8ab
Part 3: Allow href without xlink on SVG <image> elements. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/5993b97f7e6a
Part 4: Allow href without xlink on SVG <filter> elements. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/efe30b2a9e75
Part 5: Allow href without xlink on SVG <feImage> elements. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/2e2a26c95fdc
Part 6: Allow href without xlink on SVG <pattern> elements. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/d06cc4faed7f
Part 7: Allow href without xlink on SVG <mpath> elements. r=dholbert,jwatt
https://hg.mozilla.org/integration/autoland/rev/d8aa6982af06
Part 8: Allow href without xlink on SVG <textPath> elements. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/5cf65f3308ce
Part 9: Allow href without xlink on SVG <script> elements. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/783d4ea506fc
Part 10: Allow href without xlink on SVG Gradient elements. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/50fdfe9fa0a4
Part 11: Allow href without xlink on SVG Animation elements. r=dholbert,jwatt
https://hg.mozilla.org/integration/autoland/rev/6cc6e3dcd97d
Part 12: Retrieve href from both None & XLink namespace in Browser & Android. r=Gijs,sebastian
https://hg.mozilla.org/integration/autoland/rev/6f360d0b19c6
Part 13: Add href in kURLAttributesSVG. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ed1de30eda0f
Part 14: Add reftests. r=dholbert,heycam
https://hg.mozilla.org/integration/autoland/rev/efacded89fb1
Part 15: Add tests for a, script, mpath, and animate elements. r=dholbert,heycam
Added a description for the 'href' attribute at https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/href and mentioned it in the release notes at https://developer.mozilla.org/en-US/Firefox/Releases/51#SVG.

Boris, can you please have a quick look if the description is ok.

Sebastian
Flags: needinfo?(boris.chiou)
(In reply to Sebastian Zartner [:sebo] from comment #223)
> Added a description for the 'href' attribute at
> https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/href and
> mentioned it in the release notes at
> https://developer.mozilla.org/en-US/Firefox/Releases/51#SVG.
> 
> Boris, can you please have a quick look if the description is ok.
> 
> Sebastian

The release notes looks good to me.
However, I didn't implement href attribute for <discard> element on the current m-c, so I'm not sure if the description of it is ok for href attribute. Other parts in mdn are good to me.
Hi, heycam, could you please also take a quick look at the description? Thank you.
Flags: needinfo?(boris.chiou) → needinfo?(cam)
(In reply to Boris Chiou [:boris] from comment #224)
> (In reply to Sebastian Zartner [:sebo] from comment #223)
> > Added a description for the 'href' attribute at
> > https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/href and
> > mentioned it in the release notes at
> > https://developer.mozilla.org/en-US/Firefox/Releases/51#SVG.
> > 
> > Boris, can you please have a quick look if the description is ok.
> > 
> > Sebastian
> 
> The release notes looks good to me.
> However, I didn't implement href attribute for <discard> element on the
> current m-c

Ok, sounds like a follow-up is needed then, as it's defined at https://svgwg.org/specs/animations/#DiscardElementHrefAttribute.
Should I file a new bug?

> so I'm not sure if the description of it is ok for href
> attribute.

The description is basically taken from the spec. linked above, so I guess it is ok.

> Other parts in mdn are good to me.

Good! Thank you for the review!

Let me also ask teoli whether it's ok to have that huge list of specifications.
Flags: needinfo?(jypenator)
We don't implement discard yet (bug 1069931) No need for a follow up, we can tackle things in that bug.
Thanks for the reference, Robert! I already forgot about that bug.

Sebastian
From a quick look the docs look good, thanks.  (Though there is a double period at the end of the feImage description.)
Flags: needinfo?(cam)
Removed the period. Thank you for the review!
I set this to dev-doc-complete now. Jean-Yves, it would still be good to get some feedback if the long list of specifications is ok, though.

Sebastian
Keywords: dev-doc-needed → dev-doc-complete
(In reply to Sebastian Zartner [:sebo] from comment #229)
> Jean-Yves, it would still be good to get some feedback if the long list of specifications is ok, though.

I redirect the request to Jérémie, because Jean-Yves seems to be too busy.

Sebastian
Flags: needinfo?(jypenator) → needinfo?(jeremie.patonnier)
:sebo: I think it is not optimal, but I think we can live with it.
Flags: needinfo?(jeremie.patonnier)
Blocks: svg2
So I added the related parity keywords according to comment 232.

Sebastian
Keywords: parity-chrome, parity-edge, parity-safari
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: