Bug 145684 – REGRESSION(r184722) AX: WebKit video playback toolbar removed from DOM; no longer accessible to VoiceOver

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[James Craig]

AX: Regression: WebKit Video playback toolbar hides after playing and is no longer accessible to VoiceOver

This was working last year when I fixed the accessibility of the <video> shadow DOM controls. It's regressed since then.

[Radar WebKit Bug Importer]

<rdar://problem/21254835>

[James Craig]

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.

[James Craig]

WebCore/Modules/mediacontrols/mediaControlsApple.js

[James Craig]

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?

[James Craig]

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.

[James Craig]

Created attachment 257554 [details]
untested patch, not for submission

Patch currently untested due to unrelated build failures in TOT.

[James Craig]

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.

[James Craig]

Created attachment 258164 [details]
patch

functional patch without changelog or tests

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[James Craig]

Created attachment 258361 [details]
patch w/ layout test

[James Craig]

Created attachment 258400 [details]
patch w/changelog

[Dean Jackson]

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.

[James Craig]

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.

[James Craig]

Created attachment 258467 [details]
patch including CSS for iOS iPad in-page viewer

[James Craig]

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.

[James Craig]

Created attachment 258470 [details]
patch with iOS styles and much faster layout test

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[James Craig]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[James Craig]

Created attachment 258545 [details]
patch to account for new layout test failure

[James Craig]

Created attachment 258547 [details]
patch

Rebasing from TOT to fix the efl-wk2 patch application failure.

[James Craig]

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?

[Dean Jackson]

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!

[Dean Jackson]

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.

[WebKit Commit Bot]

Comment on attachment 258547 [details]
patch

Clearing flags on attachment: 258547

Committed r188182: <http://trac.webkit.org/changeset/188182>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[James Craig]

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.

[James Craig]

(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)"

[James Craig]

Filed bug 147817 to address Dean's comments.