Closed Bug 636336 Opened 9 years ago Closed 9 years ago

img/video/audio/source.setAttribute()/getAttribute() on src trims whitespace

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: ayg, Assigned: bzbarsky)

Details

Attachments

(1 file, 1 obsolete file)

Test case:

data:text/html,<!doctype html>
<script>
var img = document.createElement("img");
img.setAttribute("src", " ");
alert(img.getAttribute("src").length);
</script>

Alerts 0 in Firefox 4b11, 1 in Chrome 10 dev and Opera 11.  Should clearly be 1 -- setAttribute() followed by getAttribute() should always return exactly the same thing no matter what.

(found with my reflection tests: http://dvcs.w3.org/hg/html/raw-file/tip/tests/submission/AryehGregor/reflection/reflection-original.html)
Relevant hunk from nsHTMLImageElement::ParseAttribute:

372     if (aAttribute == nsGkAtoms::src) {
373       static const char* kWhitespace = " \n\r\t\b";
374       aResult.SetTo(nsContentUtils::TrimCharsInSet(kWhitespace, aValue));
375       return PR_TRUE;
376     }

Presumably that trimming is supposed to happen at some point, right?  If not at attr set time, then where?  Or is this not supposed to happen at all?
I guess this is supposed to happen somewhere in the "resolve an address" algorithm, but AFAIK, the IETF hasn't produced a useful spec yet.
It looks like this code was added in bug 87894.
Also looks like nowadays net_FilterURIString handles this sort of thing.  So I think we can just remove this code.
Assignee: nobody → bzbarsky
Priority: -- → P2
Whiteboard: [need review]
Er, wait.  This is about more than just images....  Should have read the summary.
Attachment #514780 - Attachment is obsolete: true
Comment on attachment 515809 [details] [diff] [review]
Don't mess with whitespace in the src attribute of images, now that necko can deal with it itself.

Please also test that something like

elem.setAttribute("src", " test ");

works as expected. Even better would be to also check that it gets reflected properly in .src (where the whitespace should be removed, right?)
Attachment #515809 - Flags: review?(jonas) → review+
Yes, reflecting is supposed to resolve the contents and return an absolute URL:

http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#reflecting-content-attributes-in-idl-attributes

Actually I'm not sure it's currently specced anywhere how you're supposed to resolve a URL, but any sane web algorithm for that will involve stripping whitespace fairly early on, I imagine.
> elem.setAttribute("src", " test ");

Added that test.

> Even better would be to also check that it gets reflected properly in .src 

Added that too.
Whiteboard: [need review] → [need gk2 ship]
Pushed http://hg.mozilla.org/mozilla-central/rev/383148dfab3e
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [need gk2 ship]
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.