1/4/16, 5:37 PM Gordon Byrnes: Summary: When accessing HTML5 video on web pages using VoiceOver and Safari, two elements are added before every video. One is called "Show controls", and the other "start playback." The "show controls" element seems to be nonfunctional in all cases - clicking it has no effect. The "start playback" button generally works on simple HTML5 <video> tags, but not more complicated player interfaces. For instance, on the ubiquitous YouTube iframe embed, the "start playback" button does nothing at all unless the video has already been played by clicking the iframe, which can only be accessed by first navigating PAST both "show controls" and "start playback." This behavior is highly confusing. Steps to Reproduce: 1. Turn on VoiceOver. 2. Visit a page containing simple HTML5 video elements and YouTube embedded iframes. See attached example or live version at http://ghbweb.com/ios_video_issue/. 3. Using VO + left/right arrow, navigate to the first video element. Expected Results: On navigating to element, the user should be able to interact with the appropriate player controls. Actual Results: Simple HTML <video> embeds: - "start playback" button works largely as expected, but "show controls" button is nonfunctional. YouTube iframes: - Both "start playback" and "show controls" are entirely nonfunctional. Version: iOS 9.2 (13C75) Notes: According to comments within the script that actually generates the buttons, these two special controls were originally added as a workaround for this webkit bug 145684 Configuration: - iPhone 6s / iPhone 5s, Safari, VoiceOver
<rdar://problem/24050668>
This is a regression since the fix in bug 145684 because the bogus "show controls" element is showing in the original HTML 5 video example, too: http://www.w3.org/2010/05/video/mediaevents.html
Show controls button should only be appearing in the shadow DOM when the controls toolbar is hidden (during playback) but it's accessible at all times. <button pseudo="-webkit-media-show-controls" aria-label="Show Controls"></button> This appears to partially work on OS X (after initial play "show controls" is only shown when controls are not), but it is not hiding on iPad, and should never be displayed on iPhone (iPhone always uses native playback controls, not shadow DOM controls).
Possible regression candidate is bug 148755 due to the change re: showControlsButton hidden property. The shadow DOM element no longer has the 'hidden' DOM property (and subsequent content attr) set correctly.
Scratch that last comment. It's unlikely that r189358 from bug 148755 made it into iOS 9.2 (13C75).
Created attachment 277080 [details] Patch
Comment on attachment 277080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277080&action=review > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:16 > + this.hasStartedPlaying = false; See note below about isPlaying vs hasStartedPlaying. > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:-672 > - Avoid unnecessary whitespace-only changes. > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:998 > + } else { > this.video.pause(); > + } webkit-style: You need the closing brace from the if block, but the else block is a single line without braces. (not my pref, but webkit style guidelines) > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1555 > + this.showShowControlsButton(this.isPlayable() && this.isPlaying && this.hasStartedPlaying); Not clear what the difference between isPlaying and hasStartedPlaying is... Do you need both? And if so, pick a better name that more clearly indicates the difference. > Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js:156 > + if (this.controls.startPlaybackButton.parentNode) { > this.controls.startPlaybackButton.parentNode.removeChild(this.controls.startPlaybackButton); > + } webkit-style: excess braces for single line. > LayoutTests/media/video-controls-show-on-kb-or-ax-event.html:17 > + > + > + Minor nit: remove the extra whitespace-only diffs. > LayoutTests/media/video-controls-show-on-kb-or-ax-event.html:34 > + > + Minor nit: remove the extra whitespace-only diffs. > LayoutTests/media/video-controls-show-on-kb-or-ax-event.html:37 > + //making sure we are getting the updated root. webkit-style: Comments need to start with a capital (and end with a period like you have.) Minor nit: "making" => "Make" // Make sure we are getting the updated root.
Created attachment 277299 [details] Patch
Comment on attachment 277299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277299&action=review I’m not giving a review+ or a review-, but I do have some comments: I don’t think the tests cover enough cases. There are a couple interesting rules for when you are allowed to show controls and when you are not and I don’t see any test coverage for those rules. > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:996 > + } else Please don’t add trailing whitespace here. > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1526 > + if (shouldShow) > + this.showControlsButton.focus(); I’d think we’d only want to call focus() when we transition from hidden to not hidden, not when we call show but the controls happen to already be visible. Or maybe this function is only called when it’s changing the state? > Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js:154 > - if (this.controls.startPlaybackButton.parentNode) > + if (this.controls.startPlaybackButton.parentNode) Please don’t check in this addition of trailing whitespace.
Comment on attachment 277299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277299&action=review I will get the rest of the changes in. >> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1526 >> + this.showControlsButton.focus(); > > I’d think we’d only want to call focus() when we transition from hidden to not hidden, not when we call show but the controls happen to already be visible. Or maybe this function is only called when it’s changing the state? This only changes when the state is changed.
Created attachment 277599 [details] Patch
Comment on attachment 277599 [details] Patch I extended the test to not only test on "mouseout" while the video is playing. I also added a test for the pause state and the initial state. I could also test the natural progression of the UI behavior, but I decided against it because, by default, the toolbar disappears completely at the 5th second after the video is played, which means that testing that natural behavior would require this test to last for >5 seconds. At the moment, this test should only take ~ 1 second.
Comment on attachment 277599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277599&action=review Patch looks okay to me. Request final review from Dean Jackson or one of the other domain owners. > LayoutTests/media/video-controls-show-on-kb-or-ax-event.html:64 > + result.innerHTML += 'FAIL: "Show Controls" button are hidden or unavailable.<br>'; Minor typo: "are" -> "is" (Just in case there is more feedback. No need to re-spin for this alone… Unless it would eat at you later. ;-)
Comment on attachment 277599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277599&action=review > Source/WebCore/ChangeLog:3 > + Fix to make sure the showControls button in a media player behaves correctly. this should be the title of the bug you can use ./Tools/Scripts/prepare-Changelog --bug=277599 to help with this > LayoutTests/ChangeLog:3 > + A Layout Test to make sure showControls Button in media player is hidden by default. ditto > LayoutTests/media/video-controls-show-on-kb-or-ax-event.html:21 > + result.innerHTML = ""; incorrect indentation
Comment on attachment 277599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277599&action=review Looks good with the notes Chris already made. > LayoutTests/media/video-controls-show-on-kb-or-ax-event.html:79 > + }, 300); > + }, 400); It's a shame this test now takes more than a second to run :(
Created attachment 278138 [details] Patch
Comment on attachment 278138 [details] Patch Clearing flags on attachment: 278138 Committed r200441: <http://trac.webkit.org/changeset/200441>
All reviewed patches have been landed. Closing bug.