Bug 145684

Summary: REGRESSION(r184722) AX: WebKit video playback toolbar removed from DOM; no longer accessible to VoiceOver
Product: WebKit Reporter: James Craig <jcraig>
Component: AccessibilityAssignee: 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 Flags
untested patch, not for submission
none
patch
buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
Archive of layout-test-results from ews101 for mac-mavericks
none
patch w/ layout test
none
patch w/changelog
dino: review+
patch including CSS for iOS iPad in-page viewer
none
patch with iOS styles and much faster layout test
buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-mavericks
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
patch to account for new layout test failure
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-mavericks
none
Archive of layout-test-results from ews107 for mac-mavericks-wk2
none
patch to account for new layout test failure
none
patch none

Description James Craig 2015-06-04 21:30:05 PDT
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.
Comment 1 Radar WebKit Bug Importer 2015-06-04 21:30:19 PDT
<rdar://problem/21254835>
Comment 2 James Craig 2015-07-23 02:30:40 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.
Comment 3 James Craig 2015-07-23 02:31:21 PDT
WebCore/Modules/mediacontrols/mediaControlsApple.js
Comment 4 James Craig 2015-07-23 02:45:36 PDT
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?
Comment 5 James Craig 2015-07-23 03:08:48 PDT
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.
Comment 6 James Craig 2015-07-27 02:26:53 PDT
Created attachment 257554 [details]
untested patch, not for submission

Patch currently untested due to unrelated build failures in TOT.
Comment 7 James Craig 2015-08-03 23:11:08 PDT
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.
Comment 8 James Craig 2015-08-04 01:37:19 PDT
Created attachment 258164 [details]
patch

functional patch without changelog or tests
Comment 9 Build Bot 2015-08-04 02:19:08 PDT
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
Comment 10 Build Bot 2015-08-04 02:19:12 PDT
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 11 Build Bot 2015-08-04 02:31:39 PDT
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
Comment 12 Build Bot 2015-08-04 02:31:42 PDT
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
Comment 13 James Craig 2015-08-06 04:41:49 PDT
Created attachment 258361 [details]
patch w/ layout test
Comment 14 James Craig 2015-08-06 14:44:36 PDT
Created attachment 258400 [details]
patch w/changelog
Comment 15 Dean Jackson 2015-08-06 16:42:27 PDT
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 16 James Craig 2015-08-06 18:38:26 PDT
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.
Comment 17 James Craig 2015-08-07 01:30:43 PDT
Created attachment 258467 [details]
patch including CSS for iOS iPad in-page viewer
Comment 18 James Craig 2015-08-07 01:40:10 PDT
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.
Comment 19 James Craig 2015-08-07 02:14:14 PDT
Created attachment 258470 [details]
patch with iOS styles and much faster layout test
Comment 20 Build Bot 2015-08-07 02:48:20 PDT
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
Comment 21 Build Bot 2015-08-07 02:48:24 PDT
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 22 Build Bot 2015-08-07 02:51:43 PDT
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
Comment 23 Build Bot 2015-08-07 02:51:48 PDT
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
Comment 24 James Craig 2015-08-07 10:19:10 PDT
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 25 Build Bot 2015-08-07 10:52:56 PDT
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
Comment 26 Build Bot 2015-08-07 10:52:58 PDT
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 27 Build Bot 2015-08-07 11:11:32 PDT
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
Comment 28 Build Bot 2015-08-07 11:11:37 PDT
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
Comment 29 James Craig 2015-08-07 16:54:13 PDT
Created attachment 258545 [details]
patch to account for new layout test failure
Comment 30 James Craig 2015-08-07 17:30:35 PDT
Created attachment 258547 [details]
patch

Rebasing from TOT to fix the efl-wk2 patch application failure.
Comment 31 James Craig 2015-08-07 18:14:06 PDT
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 32 Dean Jackson 2015-08-07 18:23:28 PDT
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 33 Dean Jackson 2015-08-07 18:24:19 PDT
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 34 WebKit Commit Bot 2015-08-07 19:14:59 PDT
Comment on attachment 258547 [details]
patch

Clearing flags on attachment: 258547

Committed r188182: <http://trac.webkit.org/changeset/188182>
Comment 35 WebKit Commit Bot 2015-08-07 19:15:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 James Craig 2015-08-09 14:27:23 PDT
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.
Comment 37 James Craig 2015-08-09 14:32:15 PDT
(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)"
Comment 38 James Craig 2015-08-09 14:35:05 PDT
Filed bug 147817 to address Dean's comments.