Bug 42087 - Don’t handle empty URL attributes specially in reflection, even in cases such as <img src="">
Summary: Don’t handle empty URL attributes specially in reflection, even in cases such...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on: 42040
Blocks: 51249
  Show dependency treegraph
 
Reported: 2010-07-12 09:28 PDT by Darin Adler
Modified: 2011-01-24 07:41 PST (History)
5 users (show)

See Also:


Attachments
Patch (23.65 KB, patch)
2010-07-12 09:30 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (49.87 KB, patch)
2010-07-14 18:06 PDT, Darin Adler
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2010-07-12 09:28:43 PDT
Treat empty URL content attributes as invalid, as now-specified in HTML5
Comment 1 Darin Adler 2010-07-12 09:30:36 PDT
Created attachment 61238 [details]
Patch
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
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.
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 14 Darin Adler 2010-07-14 18:06:09 PDT
Created attachment 61594 [details]
Patch
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[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.
Comment 16 Darin Adler 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.
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>