Bug 47907 – Setting media element 'src' attribute to "" should trigger load

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

The spec currently says:

If a src attribute of a media element is set or changed, the user agent must invoke the media element's media element load algorithm. (Removing the src attribute does not do this, even if there are source elements present.)

but HTMLMediaElement::attributeChanged only triggers a load if the src attribute is non-empty:

    if (attrName == srcAttr) {
        // Trigger a reload, as long as the 'src' attribute is present.
        if (!getAttribute(srcAttr).isEmpty())
            scheduleLoad();
    }

The comment  is correct but the code is wrong.

[Andrew Scherkus]

Other tips when discussing this with hixie:
"""
Regarding setting .src to "": that is indeed non-conforming (it technically means "load the current HTML page as a video" — the empty string is a relative URL to the current page), but you can just remove the "src" attribute altogether and call load(), which is legal.
"""

Regardless we still need to kick off a load, even if it attempts to load the current HTML page :)

[Alexey Proskuryakov]

See also: bug 35088.

[Alexey Proskuryakov]

*** Bug 75440 has been marked as a duplicate of this bug. ***

[Arun Patole]

Currently webkit allows media element's src attribute to be set to "" but doesn't invoke the media element's media element load algorithm.

But the spec says:
"The attribute, if present, must contain a valid non-empty URL potentially surrounded by spaces."
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#location-of-the-media-resource

so, should we really allow src to be set to null or invalid value??

One option is to just remove the src attribute if attempt is made to set it to null or invalid url. And as per the spec, removing it doesn't invoke load algo and current behaviour of playing older media resource looks fine. This option looks good as it is also closer to the spec ("The attribute, if present, must be nonempty/valid URL")

Other option is, like Firefox just let it be set to null or invalid and then load algo when invoked, aborts current play and throws error as src is invalid. But this is not what current spec says.

[Eric Carlson]

(In reply to comment #4)
> But the spec says:
> "The attribute, if present, must contain a valid non-empty URL potentially surrounded by spaces."
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#location-of-the-media-resource
> 
This is an authoring requirement.

> so, should we really allow src to be set to null or invalid value??
> 
Yes. The spec says [1]:

If a src attribute of a media element is set or changed, the user agent must invoke the media element's media element load algorithm. (Removing the src attribute does not do this, even if there are source elements present.)

and step 6.1 of the resource selection algorithm says:

Process candidate: If the src attribute's value is the empty string, then end the synchronous section, and jump down to the failed step below.

> Other option is, like Firefox just let it be set to null or invalid and then load algo when invoked, aborts current play and throws error as src is invalid. But this is not what current spec says.

Firefox is correct.

[1] http://dev.w3.org/html5/spec/media-elements.html#location-of-the-media-resource

[Arun Patole]

Created attachment 125797 [details]
proposed patch

[WebKit Review Bot]

Comment on attachment 125797 [details]
proposed patch

Attachment 125797 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11442257

New failing tests:
media/video-empty-source.html

[Arun Patole]

Comment on attachment 125797 [details]
proposed patch

looks like, we need to rebase video-empty-source.html for chromium ports.

[Eric Carlson]

Comment on attachment 125797 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125797&action=review

Marking r- because I think the new test should set src to "" after another video has loaded.

> LayoutTests/media/video-src-empty.html:13
> +            function start()
> +            {
> +                videos = document.querySelectorAll('video');
> +                run('videos[0].src = ""');
> +                testLoad();
> +            }

I think this would be much more useful test if it set src to "" after the <video> element has already loaded another file so we test to ensure that it unloads the other file and fires an error.

> LayoutTests/media/video-src-empty.html:17
> +                if(state == "load() with empty 'src'") {

In this test an 'error' event should only fire once the src has been set to "".

> LayoutTests/media/video-src-empty.html:30
> +                state = "load() with empty 'src'";

Setting "state" to a string sort of made sense in video-src-none.html where two <video> elements were being tested, but you might as well just use a bool here.

> LayoutTests/media/video-src-none.html:33
> +                state = "load() with missing 'src'";

As above, you may as well update this to use a bool instead of a string now that only one element is being tested.

[Arun Patole]

Created attachment 126013 [details]
updated patch

[WebKit Review Bot]

Comment on attachment 126013 [details]
updated patch

Attachment 126013 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11449299

New failing tests:
media/video-empty-source.html

[Eric Carlson]

Comment on attachment 126013 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126013&action=review

Marking r+, but I would prefer to have the minor issues noted cleaned up before the patch is landed.

> LayoutTests/media/video-src-empty.html:21
> +                run('video.load()');

Calling load() is unnecessary, setting src trigger a load implicitly.

> LayoutTests/media/video-src-empty.html:27
> +                testExpected(event.target, video);

Nit: "testExpected('event.target', video)" will generate "EXPECTED (event.target == '[object HTMLVideoElement]')", which IMO is slightly more readable.

> LayoutTests/media/video-src-empty.html:37
> +                run('video.load()');

load() is unnecessary.

> LayoutTests/media/video-src-none.html:19
> +                failTest("<br><i>Calling load() with no 'src' should NOT fire 'error' event<" + "/i>");              

This will cause the test to fail, but the message it logs is confusing because it is exactly the same as that already logged from the "test()" function. I would rather see another message here.

[Arun Patole]

Created attachment 126282 [details]
updated patch

Updated patch... also skipped failing test on chromium as it needs a rebaseline.

[Arun Patole]

Created attachment 126284 [details]
updated patch

[Eric Carlson]

Comment on attachment 126284 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126284&action=review

> LayoutTests/platform/chromium/test_expectations.txt:4122
> +BUGWK47907 : media/video-empty-source.html = IMAGE+TEXT

Please either get someone with a cr build to land new results or file a new bug asking for a rebaseline.

[Arun Patole]

> Please either get someone with a cr build to land new results or file a new bug asking for a rebaseline.
Done:https://bugs.webkit.org/show_bug.cgi?id=78233 , Thanks!

[WebKit Review Bot]

Comment on attachment 126284 [details]
updated patch

Clearing flags on attachment: 126284

Committed r107244: <http://trac.webkit.org/changeset/107244>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.