Summary: | REGRESSION(r184722) AX: WebKit video playback toolbar removed from DOM; no longer accessible to VoiceOver | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Craig <jcraig> | ||||||||||||||||||||||||||||||||
Component: | Accessibility | Assignee: | James Craig <jcraig> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, dino, jcraig, jer.noble, lembutful, rniwa, roger_fong, simon.fraser, webkit-bug-importer | ||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||
Bug Blocks: | 147817, 153089 | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
James Craig
2015-06-04 21:30:05 PDT
Regression in r184722 The controls are being entirely removed from the DOM now on mouseout. > 184722 roger_fong this.base.removeChild(this.controls.panel); They are added back in only on <video> element mouseover. WebCore/Modules/mediacontrols/mediaControlsApple.js Regression caused by bug 144770. Roger, Simon, and Jer, would you prefer to roll out r184722 or lay a fix on top of it? The controls need to be left in the DOM, so we should discuss other solutions. One possibility is to move the controls offscreen when they are not hovered or focused. This would allow them to forego the opacity pinch, though I'm not sure that will solve your repaint problem. Any other ideas? Justification from other bug:
> This is the strategy that Simon, Jer and [Roger] finally settled on for avoiding unnecessary paints and style recalcs, simply to remove the controls entirely when they are hidden and re-add them back in when they are shown. This way we can call updateTime and progress as much as we want while the controls are hidden with none of those performance penalties.
Created attachment 257554 [details]
untested patch, not for submission
Patch currently untested due to unrelated build failures in TOT.
The workaround turns out to be a little more challenging than anticipated. Above patch was not working due to VoiceOver ignoring the focusable group *because* it's empty. The DOMFocusIn event works, but only if you Tab or Shift+Tab in with full keyboard access. That's not a good enough solution. Now attempting to insert a new "show controls" button instead of trying focus tricks. Created attachment 258164 [details]
patch
functional patch without changelog or tests
Comment on attachment 258164 [details] patch Attachment 258164 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/14494 New failing tests: media/track/track-cue-rendering-horizontal.html http/tests/contentextensions/text-track-blocked.html Created attachment 258165 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 258164 [details] patch Attachment 258164 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/14523 New failing tests: media/track/track-cue-rendering-horizontal.html Created attachment 258166 [details]
Archive of layout-test-results from ews101 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 258361 [details]
patch w/ layout test
Created attachment 258400 [details]
patch w/changelog
Comment on attachment 258400 [details] patch w/changelog View in context: https://bugs.webkit.org/attachment.cgi?id=258400&action=review Looks good. Have you checked this on iOS? (You don't need to build all of WebKit to check. Just copy the .js/.css files into <Simulator>/S/L/PF/WebCore.framework and killall MobileSafari) > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1437 > + this.showControlsButton.hidden = true; Interesting. I didn't know that setting the hidden attribute removes the button from VO, but I guess that makes complete sense! > LayoutTests/media/video-controls-show-on-kb-or-ax-event.html:32 > + }, 4500) // Wait 4+ seconds for the controls to disappear before testing. People are not going to be happy with such a timeout, but I'm not sure what to do about it. Comment on attachment 258400 [details] patch w/changelog View in context: https://bugs.webkit.org/attachment.cgi?id=258400&action=review >> LayoutTests/media/video-controls-show-on-kb-or-ax-event.html:32 >> + }, 4500) // Wait 4+ seconds for the controls to disappear before testing. > > People are not going to be happy with such a timeout, but I'm not sure what to do about it. I could try reaching into the script to call .hideControls() immediately, but that's a bad testing methodology. I'll file a follow up bug to address it. Created attachment 258467 [details]
patch including CSS for iOS iPad in-page viewer
Comment on attachment 258467 [details]
patch including CSS for iOS iPad in-page viewer
Just had an idea to speed up the test that I'll try out tomorrow. 4 secs is the default, but if I force a mouseover/mouseout, it hides in ~250ms.
Created attachment 258470 [details]
patch with iOS styles and much faster layout test
Comment on attachment 258470 [details] patch with iOS styles and much faster layout test Attachment 258470 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/26632 New failing tests: media/track/track-cue-rendering-horizontal.html Created attachment 258474 [details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 258470 [details] patch with iOS styles and much faster layout test Attachment 258470 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/26634 New failing tests: media/track/track-cue-rendering-horizontal.html http/tests/contentextensions/text-track-blocked.html Created attachment 258475 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 258509 [details]
patch to account for new layout test failure
Previous patch was flaky on some systems b/c it didn't wait quite long enough for the video controls to be hidden. Adjusting the timeout speed. The other text-track-blocked expectation might still get a platform-specific failure.
Comment on attachment 258509 [details] patch to account for new layout test failure Attachment 258509 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/27832 New failing tests: media/track/track-cue-rendering-horizontal.html Created attachment 258510 [details]
Archive of layout-test-results from ews103 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 258509 [details] patch to account for new layout test failure Attachment 258509 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/27872 New failing tests: media/track/track-cue-rendering-horizontal.html http/tests/contentextensions/text-track-blocked.html Created attachment 258513 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 258545 [details]
patch to account for new layout test failure
Created attachment 258547 [details]
patch
Rebasing from TOT to fix the efl-wk2 patch application failure.
Comment on attachment 258547 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=258547&action=review > LayoutTests/media/video-controls-show-on-kb-or-ax-event.html:39 > + }, 300) // Wait for video toolbar to hide. Much better than the previous 4 seconds, eh? Comment on attachment 258547 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=258547&action=review r=me in order to get the commit in. I'm slightly concerned about making changes so late, because this entire section of the codebase is very unstable :( Another question: Will this have any impact on people using keyboard navigation (without VO). I haven't even tried this myself. > Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:146 > + width: 100%; > + height: 10px; Is there a reason the controls have to have dimensions in the video case? > LayoutTests/http/tests/contentextensions/text-track-blocked-expected.txt:18 > + RenderButton {BUTTON} at (0,230) size 320x10 [bgcolor=#C0C0C0] [border: (2px outset #C0C0C0)] It's weird that we still get a border even with appearance none. I wonder if we should be explicit about border: none !important? >> LayoutTests/media/video-controls-show-on-kb-or-ax-event.html:39 >> + }, 300) // Wait for video toolbar to hide. > > Much better than the previous 4 seconds, eh? Yes! Thanks for this! Comment on attachment 258547 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=258547&action=review > LayoutTests/media/video-controls-show-on-kb-or-ax-event.html:32 > + } else { > + result.innerHTML += 'FAIL: "Show Controls" button is not available.<br>'; > + } No braces required here. Comment on attachment 258547 [details] patch Clearing flags on attachment: 258547 Committed r188182: <http://trac.webkit.org/changeset/188182> All reviewed patches have been landed. Closing bug. Comment on attachment 258547 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=258547&action=review >> Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:146 >> + height: 10px; > > Is there a reason the controls have to have dimensions in the video case? Yes. Even though they are invisible, the dimensions give VoiceOver dimensions for a touch position. It's full width so you can drag your finger down the screen and hit it. If it was small or offscreen, VoiceOver users would only be able to get this through linear swipe navigation only. >> LayoutTests/http/tests/contentextensions/text-track-blocked-expected.txt:18 >> + RenderButton {BUTTON} at (0,230) size 320x10 [bgcolor=#C0C0C0] [border: (2px outset #C0C0C0)] > > It's weird that we still get a border even with appearance none. I wonder if we should be explicit about border: none !important? It doesn't really matter since the opacity is 0. I can address it in the follow up bug if you think it is better. >> LayoutTests/media/video-controls-show-on-kb-or-ax-event.html:32 >> + } > > No braces required here. I will update in the follow-up bug. (In reply to comment #32) > Another question: Will this have any impact on people using keyboard > navigation (without VO). I haven't even tried this myself. Not yet. Keyboard navigation of all Shadow DOM controls is blocked by bug 117857 "FKA: No way to get focus from DOM to shadow DOM components (Was: HTML5 media controls not keyboard accessible)" Filed bug 147817 to address Dean's comments. |