Bug 69097 - REGRESSION (r95381): Standalone video can be focused and draws a focus ring
Summary: REGRESSION (r95381): Standalone video can be focused and draws a focus ring
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Nobody
URL: http://movies.apple.com/movies/us/app...
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2011-09-29 11:35 PDT by mitz
Modified: 2011-10-13 09:07 PDT (History)
9 users (show)

See Also:


Attachments
patch (1.46 KB, patch)
2011-10-05 04:56 PDT, Deepak Sherveghar
tonikitoo: review-
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
Updated Patch with layout test. (5.15 KB, patch)
2011-10-07 03:02 PDT, Deepak Sherveghar
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (5.38 KB, patch)
2011-10-07 05:16 PDT, Deepak Sherveghar
no flags Details | Formatted Diff | Diff
Updated Patch with review comments. (5.58 KB, patch)
2011-10-12 05:40 PDT, Deepak Sherveghar
eric.carlson: review-
Details | Formatted Diff | Diff
Updated Patch (5.60 KB, patch)
2011-10-12 22:21 PDT, Deepak Sherveghar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2011-09-29 11:35:45 PDT
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.
Comment 1 Radar WebKit Bug Importer 2011-09-29 11:36:40 PDT
<rdar://problem/10209143>
Comment 2 mitz 2011-09-29 11:43:09 PDT
Caused by <http://trac.webkit.org/r95381>.
Comment 3 Antonio Gomes 2011-09-29 12:33:06 PDT
(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?
Comment 4 mitz 2011-09-29 12:37:34 PDT
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.
Comment 5 Alexey Proskuryakov 2011-09-30 09:02:38 PDT
Perhaps we should consider whether we want to focus a media element when clicking on its controls. Bug 67190 asked for Tab only.
Comment 6 Antonio Gomes 2011-09-30 09:34:29 PDT
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?
Comment 7 Deepak Sherveghar 2011-10-02 00:46:43 PDT
(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 ???
Comment 8 Deepak Sherveghar 2011-10-05 04:56:44 PDT
Created attachment 109773 [details]
patch
Comment 9 Antonio Gomes 2011-10-05 05:00:50 PDT
Comment on attachment 109773 [details]
patch

Patch is nice, but it needs a test.
Comment 10 Deepak Sherveghar 2011-10-05 05:31:26 PDT
(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.
Comment 11 Alexey Proskuryakov 2011-10-05 08:30:57 PDT
run-webkit-tests doesn't treat media files as top level tests, but you could put one into an iframe.
Comment 12 Deepak Sherveghar 2011-10-07 03:02:03 PDT
Created attachment 110116 [details]
Updated Patch with layout test.

Thanks Alexey and Antonio, Created a test for this using IFRAME.
Comment 13 WebKit Review Bot 2011-10-07 03:52:23 PDT
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
Comment 14 Deepak Sherveghar 2011-10-07 05:16:48 PDT
Created attachment 110128 [details]
Updated Patch

Made layout test to select appropriate media file on respective platforms.
Comment 15 Antonio Gomes 2011-10-07 07:07:56 PDT
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.
Comment 16 Eric Carlson 2011-10-10 15:54:19 PDT
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.
Comment 17 Deepak Sherveghar 2011-10-12 05:40:47 PDT
Created attachment 110673 [details]
Updated Patch with review comments.
Comment 18 Eric Carlson 2011-10-12 06:54:22 PDT
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.
Comment 19 Deepak Sherveghar 2011-10-12 22:21:13 PDT
Created attachment 110800 [details]
Updated Patch
Comment 20 Eric Carlson 2011-10-13 07:48:40 PDT
Comment on attachment 110800 [details]
Updated Patch

Thanks!
Comment 21 WebKit Review Bot 2011-10-13 08:56:16 PDT
Comment on attachment 110800 [details]
Updated Patch

Clearing flags on attachment: 110800

Committed r97367: <http://trac.webkit.org/changeset/97367>
Comment 22 WebKit Review Bot 2011-10-13 08:56:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Deepak Sherveghar 2011-10-13 09:07:08 PDT
Thanks Antonio, Eric, Alexey for review and guidance !!