RESOLVED FIXED 42087
Don’t handle empty URL attributes specially in reflection, even in cases such as <img src="">
https://bugs.webkit.org/show_bug.cgi?id=42087
Summary Don’t handle empty URL attributes specially in reflection, even in cases such...
Darin Adler
Reported 2010-07-12 09:28:43 PDT
Treat empty URL content attributes as invalid, as now-specified in HTML5
Attachments
Patch (23.65 KB, patch)
2010-07-12 09:30 PDT, Darin Adler
no flags
Patch (49.87 KB, patch)
2010-07-14 18:06 PDT, Darin Adler
eric.carlson: review+
Darin Adler
Comment 1 2010-07-12 09:30:36 PDT
Darin Adler
Comment 2 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.
Alexey Proskuryakov
Comment 3 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.
Alexey Proskuryakov
Comment 4 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.
Ian 'Hixie' Hickson
Comment 5 2010-07-12 15:49:59 PDT
Wait, hold on, what is this changing?
Darin Adler
Comment 6 2010-07-12 16:02:53 PDT
Before this change, if you had an element like this: <img src=""> In a document at http://host.com/index.html then: 1) It would try to load the image from the URL "http://host.com/index.html". 2) If you did javascript:alert(document.getElementsByTagName("img")[0].src) you would get "http://host.com/index.html". After the change: 1) It does not try to load an image. 2) The result of javascript:alert(document.getElementsByTagName("img")[0].src) is an empty string.
Ian 'Hixie' Hickson
Comment 7 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.
Darin Adler
Comment 8 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.
Ian 'Hixie' Hickson
Comment 9 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.
Darin Adler
Comment 10 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.
Ian 'Hixie' Hickson
Comment 11 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. :-)
Darin Adler
Comment 12 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!
Darin Adler
Comment 13 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.
Darin Adler
Comment 14 2010-07-14 18:06:09 PDT
Eric Carlson
Comment 15 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[0].src == '') OK > ** <video> with empty src attribute ** > EXPECTED (videos[1].error == 'null') OK > EXPECTED (videos[1].networkState == '0') OK > -EXPECTED (videos[1].src == '') OK > +EXPECTED (videos[1].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.
Darin Adler
Comment 16 2010-07-15 10:34:01 PDT
(In reply to comment #15) > > -EXPECTED (videos[1].src == '') OK > > +EXPECTED (videos[1].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.
Darin Adler
Comment 17 2010-07-26 15:58:52 PDT
Ademar Reis
Comment 18 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>
Note You need to log in before you can comment on or make changes to this bug.