Bug 42087 – Don’t handle empty URL attributes specially in reflection, even in cases such as <img src="">

• Component: DOM
• Status: RESOLVED FIXED

[Darin Adler]

Treat empty URL content attributes as invalid, as now-specified in HTML5

[Darin Adler]

Created attachment 61238 [details]
Patch

[Darin Adler]

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]

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]

-        readonly attribute long insertId
-            getter raises(DOMException);
+        readonly attribute long insertId getter raises(DOMException);

I find the former variant more readable.

[Ian 'Hixie' Hickson]

Wait, hold on, what is this changing?

[Darin Adler]

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]

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]

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]

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]

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]

If you're going to ask to change everything that's inelegant in the Web platform, we might be there a while. :-)

[Darin Adler]

(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]

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]

Created attachment 61594 [details]
Patch

[Eric Carlson]

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]

(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]

Committed r64085: <http://trac.webkit.org/changeset/64085>

[Ademar Reis]

Revision r64085 cherry-picked into qtwebkit-2.2 with commit 6cc7b07 <http://gitorious.org/webkit/qtwebkit/commit/6cc7b07>