Summary: | AX: Nonfunctional controls appear before every HTML5 video when using VoiceOver | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Craig <jcraig> | ||||||||||
Component: | Accessibility | Assignee: | Aaron Chu <aaron_chu> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | Safari 9 | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
URL: | http://ghbweb.com/ios_video_issue/ | ||||||||||||
Bug Depends on: | 145684 | ||||||||||||
Bug Blocks: | 157692 | ||||||||||||
Attachments: |
|
Description
James Craig
2016-01-13 18:13:50 PST
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. |