REGRESSION (r95381): Standalone video can be focused and draws a focus ring

• Component: Media
• Status: RESOLVED FIXED

To reproduce, navigate to <http://movies.apple.com/movies/us/apple/ipoditunes/2007/touch/ads/apple_ipodtouch_touch_r640-9cie.mov> and click inside the video. It gets a focus ring, but it should not.

[Radar WebKit Bug Importer]

<rdar://problem/10209143>

Caused by <http://trac.webkit.org/r95381>.

[Antonio Gomes]

(In reply to comment #2)
> Caused by <http://trac.webkit.org/r95381>.

mitz: so you are against this change, we you are up to special case this fullscreen case?

I am not sure what you’re asking. This bug is not about full screen video. In standalone media documents, the element should behave the same (responding to key presses) regardless of whether the user has clicked inside it once, so there should never be a focus indicator.

[Alexey Proskuryakov]

Perhaps we should consider whether we want to focus a media element when clicking on its controls. Bug 67190 asked for Tab only.

[Antonio Gomes]

It should be fine with revert the original patch (fixing this), and re-landing it in a way that it does not break this case. Is it preferrable?

[Deepak Sherveghar]

(In reply to comment #6)
> It should be fine with revert the original patch (fixing this), and re-landing it in a way that it does not break this case. Is it preferrable?

probably we can add a check if the media element is in a standalone media document inside supportFocus(). if it is, then simply return false from supportsFocus().

ie: 
  if (ownerDocument()->isMediaDocument())
      return false

   // rest of the code ...

Does this seem reasonable ???

[Deepak Sherveghar]

Created attachment 109773 [details]
patch

[Antonio Gomes]

Comment on attachment 109773 [details]
patch

Patch is nice, but it needs a test.

[Deepak Sherveghar]

(In reply to comment #9)
> (From update of attachment 109773 [details])
> Patch is nice, but it needs a test.

I agree, but adding a test case for standalone media document seems little difficult. Any pointers on this would be of great help.

[Alexey Proskuryakov]

run-webkit-tests doesn't treat media files as top level tests, but you could put one into an iframe.

[Deepak Sherveghar]

Created attachment 110116 [details]
Updated Patch with layout test.

Thanks Alexey and Antonio, Created a test for this using IFRAME.

[WebKit Review Bot]

Comment on attachment 110116 [details]
Updated Patch with layout test.

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

New failing tests:
fast/events/media-focus-in-standalone-media-document.html

[Deepak Sherveghar]

Created attachment 110128 [details]
Updated Patch

Made layout test to select appropriate media file on respective platforms.

[Antonio Gomes]

Comment on attachment 110116 [details]
Updated Patch with layout test.

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

Looks good to me, but I will defer to our Apple friends a final word.

> Source/WebCore/html/HTMLMediaElement.cpp:251
> +    // We don't want to focus a media element in a standalone document.

Not sure if the comment is needed, given that it is very obvious from the actual code below it.

[Eric Carlson]

Comment on attachment 110128 [details]
Updated Patch

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

> LayoutTests/fast/events/media-focus-in-standalone-media-document.html:17
> +    var videoElement;
> +    var skipOnFirstEmptyLoad = 0;
> +    if (window.layoutTestController)
> +    {
> +         layoutTestController.dumpAsText();
> +         layoutTestController.waitUntilDone();
> +    }
> +
> +    function log(message)
> +    {
> +        document.getElementById("console").innerHTML += message + "<br>";
> +    }

If you include video-test.js much of this is unnecessary, plus the test output will look like the rest of the media layout tests.

> LayoutTests/fast/events/media-focus-in-standalone-media-document.html:48
> +        //Simulate click event to try focus video element.

Nit: missing a space after the "//"

> LayoutTests/fast/events/media-focus-in-standalone-media-document.html:67
> +</script>
> +</head>
> +<body>
> +<p>
> +This tests that  media element in a standalone media document cannot be focused directly using focus() method or by mouse click.
> +</p>
> +<p id="console"></p>
> +<iframe id="videoframe" width=380 height=330 onload="frameLoaded()"></iframe>
> +<script>
> +document.getElementById("videoframe").src = "../../media/" + findMediaFile("video", "content/test");
> +</script>
> +</body>
> +</html>

Indentation would make this much easier to read, IMO.

[Deepak Sherveghar]

Created attachment 110673 [details]
Updated Patch with review comments.

[Eric Carlson]

Comment on attachment 110673 [details]
Updated Patch with review comments.

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

This is really close, but I think it makes sense to correct the one mistake before committing.

> LayoutTests/fast/events/media-focus-in-standalone-media-document-expected.txt:11
> +*** Video element clicked.
> +
> +*** Should not focus video element by mouse click.
> +EXPECTED (standaloneMediaDocument.activeElement != '[object HTMLVideoElement]') OK

The results are confusing, it looks like the click event happens before the test...

> LayoutTests/fast/events/media-focus-in-standalone-media-document.html:43
> +                videoElement.dispatchEvent(click);
> +                consoleWrite("<br>*** Should not focus video element by mouse click.");
> +                testExpected("standaloneMediaDocument.activeElement", videoElement, "!=");

because you dispatch the event before you write the comment about what is being tested.

[Deepak Sherveghar]

Created attachment 110800 [details]
Updated Patch

[Eric Carlson]

Comment on attachment 110800 [details]
Updated Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 110800 [details]
Updated Patch

Clearing flags on attachment: 110800

Committed r97367: <http://trac.webkit.org/changeset/97367>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Deepak Sherveghar]

Thanks Antonio, Eric, Alexey for review and guidance !!