1312742 - ping attribute should reflect as regular string, not list of URLs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Per <https://github.com/whatwg/html/issues/1780>.  Per discussion there, this behavior matches the spec, upstream tests, Chrome, and WebKit (Edge doesn't support ping), and bz and I agreed that we didn't feel strongly.  We match an older version of the spec, I think.  I sort of doubt there will be compat implications, but I guess you never know.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Bug 1312742 - Match spec/other browsers for ping reflection

The spec used to say that getting the .ping property on
HTMLAnchorElement or HTMLAreaElement should return a DOMTokenList of
parsed URLs, but it now says to return a plain string.  All other UAs
that implement ping match the new spec, and it's hopefully unlikely that
any sites depend on our old behavior.

Discussion: https://github.com/whatwg/html/issues/1780

Review commit: https://reviewboard.mozilla.org/r/88964/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88964/

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8805117 [details]
Bug 1312742 - Match spec/other browsers for ping reflection

https://reviewboard.mozilla.org/r/88964/#review88156

::: dom/html/HTMLAnchorElement.cpp:350
(Diff revision 1)
>  HTMLAnchorElement::ToString(nsAString& aSource)
>  {
>    return GetHref(aSource);
>  }
>  
>  NS_IMETHODIMP    

nit: Can you remove this trailing whitespace while you are here?

::: dom/html/HTMLAreaElement.cpp:224
(Diff revision 1)
>  HTMLAreaElement::ToString(nsAString& aSource)
>  {
>    return GetHref(aSource);
>  }
>  
>  NS_IMETHODIMP    

nit: Can you remove this trailing whitespace while you are here?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8805117 [details]
Bug 1312742 - Match spec/other browsers for ping reflection

https://reviewboard.mozilla.org/r/88964/#review88158

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8805117 [details]
Bug 1312742 - Match spec/other browsers for ping reflection

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88964/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18ff1472dae9
Match spec/other browsers for ping reflection r=bkelly

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for not updating the expected results of wpt test /html/dom/reflection-embedded.html and more:

https://hg.mozilla.org/integration/autoland/rev/9a3b8780710b720148346572aec2bf2c235602c2

Push with unexpected passes: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=18ff1472dae91c2b75e0ca843d52ad9a34c438cf
Test log: https://treeherder.mozilla.org/logviewer.html#?job_id=5886136&repo=autoland

TEST-UNEXPECTED-PASS | /html/dom/reflection-embedded.html | area.ping: setAttribute() to " \0\x01\x02\x03\x04\x05\x06\x07 \b\t\n\v\f\r\x0e\x0f \x10\x11\x12\x13\x14\x15\x16\x17 \x18\x19\x1a\x1b\x1c\x1d\x1e\x1f foo " followed by IDL get - expected FAIL
...
TEST-UNEXPECTED-PASS | /html/dom/reflection-text.html | a.ping: setAttribute() to " \0\x01\x02\x03\x04\x05\x06\x07 \b\t\n\v\f\r\x0e\x0f \x10\x11\x12\x13\x14\x15\x16\x17 \x18\x19\x1a\x1b\x1c\x1d\x1e\x1f foo " followed by IDL get - expected FAIL
...

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

These are all green on the try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d927871dc12a

Although the test file changes show up on Review Board, they didn't wind up in the Autoland commit:

https://reviewboard.mozilla.org/r/88964/diff/2#index_header
https://hg.mozilla.org/integration/autoland/rev/18ff1472dae9

I filed bug 1313915 on the Autoland problem, and will reland manually on inbound.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I see the problem -- bug 1309931 updated the test in the interim and added unexpected fails that my commit needed to remove.  Easy to fix now that I know what happened.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0cbdabb751
Match spec/other browsers for ping reflection; r=bkelly

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc0cbdabb751