|Summary:||Don’t handle empty URL attributes specially in reflection, even in cases such as <img src="">|
|Product:||WebKit||Reporter:||Darin Adler <darin>|
|Component:||DOM||Assignee:||Darin Adler <darin>|
|Severity:||Normal||CC:||ademar, ap, eric.carlson, ian, japhet|
|Version:||528+ (Nightly build)|
|Bug Depends on:||42040|
Description Darin Adler 2010-07-12 09:28:43 PDT
Treat empty URL content attributes as invalid, as now-specified in HTML5
Comment 2 Darin Adler 2010-07-12 09:32:14 PDT
Some things I need to resolve before landing this: The <form action=""> change breaks dom/html/level2/html/button03.html and dom/xhtml/level2/html/button03.xhtml. The <img src=""> change breaks fast/images/load-img-with-empty-src.html. The <iframe src=""> may be the thing that breaks http/tests/security/xssAuditor/full-block-iframe-no-inherit.html.
Comment 3 Alexey Proskuryakov 2010-07-12 11:28:35 PDT
See also: bug 30303 <img src=""> requests main document resource unnecessarily bug 42001 Update media element's handling of empty 'src' attribute bug 33337 fast/images/load-img-with-empty-src.html Timed out on Snow Leopard Release bot Specifically, bug 30303 has a patch that takes care of several cases, not just img.
Comment 4 Alexey Proskuryakov 2010-07-12 11:34:11 PDT
- readonly attribute long insertId - getter raises(DOMException); + readonly attribute long insertId getter raises(DOMException); I find the former variant more readable.
Comment 5 Ian 'Hixie' Hickson 2010-07-12 15:49:59 PDT
Wait, hold on, what is this changing?
Comment 6 Darin Adler 2010-07-12 16:02:53 PDT
Comment 7 Ian 'Hixie' Hickson 2010-07-12 16:42:10 PDT
Well for (1) that's good, the spec says to fire an 'error' event in that case. But why (2)? I don't see anything making an exception for the empty string in the reflection rules. The reason I looked at this bug is that the bug summary implies that authoring conformance criteria (the fact that src="" with an empty string as its value is invalid) has some relevance when considering how something is handled by the user agent (how it is treated), which is most definitely not the case. For the purposes of writing a browser, you should ignore anything in the spec that talks about what's valid or allowed or conforming or required for authors and documents, and only look at user agent requirements.
Comment 8 Darin Adler 2010-07-12 17:42:24 PDT
Thanks. It seems I had some things wrong. I need to explore reflection of URL attributes more. It's strange that getting one of these attributes resolves the URL against the base.
Comment 9 Ian 'Hixie' Hickson 2010-07-12 17:45:34 PDT
It's only strange until you realise that it'll only affect authors who are already violating the spec, since it's invalid to have a document in that state in the first place. There are a lot of areas where the API starts acting a bit weird when you do something invalid; that's usually why it's invalid.
Comment 10 Darin Adler 2010-07-13 16:06:38 PDT
I’ll probably send some mail to WhatWG about this. Before the empty URL change, there was something of an invariant where HTMLImageElement’s src IDL attribute would return the URL that will be loaded. Now it sometimes returns the URL that will be loaded, and sometimes returns a URL that will not be loaded. You could have two image elements that return the same value for the src IDL attribute but one will load the image and the other won’t. I think that’s a bit inelegant so it might be worth changing.
Comment 11 Ian 'Hixie' Hickson 2010-07-13 17:09:34 PDT
If you're going to ask to change everything that's inelegant in the Web platform, we might be there a while. :-)
Comment 12 Darin Adler 2010-07-13 17:11:11 PDT
(In reply to comment #11) > If you're going to ask to change everything that's inelegant in the Web platform, we might be there a while. :-) That’s not fair. This is a new, just-introduced inelegant thing created by changing things in one place and not another!
Comment 13 Darin Adler 2010-07-14 16:31:52 PDT
We can use bug 30303 to track changing the actual loading behavior. Since Hixie clarified that IDL reflection is not supposed to have a special case for the empty string, I’ll repurpose this bug to roll back the changes related to that.
Comment 15 Eric Carlson 2010-07-15 10:22:52 PDT
Comment on attachment 61594 [details] Patch > > Set 'poster' to ''. > EXPECTED (video.getAttribute('poster') == '') OK > -EXPECTED (relativeURL(video.poster) == '') OK > +EXPECTED (relativeURL(video.poster) == 'video-poster.html') OK > I understand that this is what the spec says is "correct", but it makes no sense at all. What user expects video.poster to be the document url? > -EVENT(error) from <source id='empty-src' src=''> OK > +EVENT(error) from <source id='empty-src' src='video-source-error-no-candidate.html'> OK > EXPECTED (video.error == 'null') OK Ditto. > Index: LayoutTests/media/video-src-none-expected.txt > =================================================================== > --- LayoutTests/media/video-src-none-expected.txt (revision 63382) > +++ LayoutTests/media/video-src-none-expected.txt (working copy) > @@ -8,7 +8,7 @@ EXPECTED (videos.src == '') OK > ** <video> with empty src attribute ** > EXPECTED (videos.error == 'null') OK > EXPECTED (videos.networkState == '0') OK > -EXPECTED (videos.src == '') OK > +EXPECTED (videos.src == 'relativeURL('video-src-none.html')'), OBSERVED 'file:///Volumes/Home/darin/Safari/OpenSource/LayoutTests/media/video-src-none.html' FAIL > Probably not good to check in the result with a failure, or with a path to a file on your machine. r=me with a fix for the failure above, although I strongly disagree with the spec's mandated behavior. We should get this changed.
Comment 16 Darin Adler 2010-07-15 10:34:01 PDT
(In reply to comment #15) > > -EXPECTED (videos.src == '') OK > > +EXPECTED (videos.src == 'relativeURL('video-src-none.html')'), OBSERVED 'file:///Volumes/Home/darin/Safari/OpenSource/LayoutTests/media/video-src-none.html' FAIL > > > Probably not good to check in the result with a failure, or with a path to a file on your machine. Oops! I should't have quoted relativeURL there. Will fix! > r=me with a fix for the failure above, although I strongly disagree with the spec's mandated behavior. We should get this changed. Given your comments, I think I will leave the bindings support for “NonEmpty” in for now. In case we do get this changed, we can just add the attribute back and fix the test cases, without having to change the bindings generator yet again. We can always remove this later if we end up not changing the HTML specification.
Comment 17 Darin Adler 2010-07-26 15:58:52 PDT
Committed r64085: <http://trac.webkit.org/changeset/64085>
Comment 18 Ademar Reis 2011-01-24 07:41:40 PST
Revision r64085 cherry-picked into qtwebkit-2.2 with commit 6cc7b07 <http://gitorious.org/webkit/qtwebkit/commit/6cc7b07>