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

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

({good-first-bug})

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

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.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Attachment #8805117 - Flags: review?(bkelly)

Comment 2

3 years ago
mozreview-review
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

3 years ago
mozreview-review
Comment on attachment 8805117 [details]
Bug 1312742 - Match spec/other browsers for ping reflection

https://reviewboard.mozilla.org/r/88964/#review88158
Attachment #8805117 - Flags: review?(bkelly) → review+
Comment hidden (mozreview-request)

Comment 5

3 years ago
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/autoland/rev/18ff1472dae9
Match spec/other browsers for ping reflection r=bkelly
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
...
Flags: needinfo?(ayg)
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.
Flags: needinfo?(ayg)
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

3 years ago
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0cbdabb751
Match spec/other browsers for ping reflection; r=bkelly

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc0cbdabb751
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.