Bug 123479 – AX: Audio and Video attachments are not output to VoiceOver

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

[chris fleizach]

Audio attachments in Mail are not navigable by VoiceOver or other apps that use accessibility.

<rdar://problem/15239047>

[chris fleizach]

Created attachment 215454 [details]
patch

[chris fleizach]

Comment on attachment 215454 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215454&action=review

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1104
> +    if (isControl())

this line was oddly off by one character

> Source/WebCore/editing/TextIterator.cpp:724
> +    if (m_emitsObjectReplacementCharacters && isRendererReplacedElement(renderer)) {

isRendererReplaced is a better check because it counts things like widgets, media elements and when role="img" is used

[chris fleizach]

Created attachment 215456 [details]
patch

patch to clean up some erroneous comments left in layout test

[Build Bot]

Comment on attachment 215456 [details]
patch

Attachment 215456 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/17548026

New failing tests:
fast/forms/access-key-for-all-elements.html
http/tests/media/reload-after-dialog.html
fast/css/relative-position-replaced-in-table-display-crash.html
fullscreen/video-controls-drag.html
fast/spatial-navigation/snav-media-elements.html
http/tests/media/video-play-progress.html
http/tests/appcache/video.html
http/tests/media/video-play-stall-before-meta-data.html
fullscreen/full-screen-no-style-sharing.html
fast/forms/form-associated-element-crash3.html
http/tests/media/pdf-served-as-pdf.html
fast/regions/full-screen-video-from-region.html
fast/multicol/renderer-positioned-assert-crash.html
http/tests/media/video-buffered-range-contains-currentTime.html
http/tests/media/video-served-as-text.html
http/tests/media/video-load-suspend.html
fullscreen/full-screen-crash-offsetLeft.html
http/tests/media/video-error-abort.html
http/tests/media/text-served-as-text.html
fullscreen/full-screen-stacking-context.html
compositing/video/video-poster.html
fullscreen/video-controls-override.html
http/tests/media/video-preload.html
compositing/visibility/visibility-simple-video-layer.html
http/tests/media/video-accept-encoding.html
http/tests/media/video-throttled-load-metadata.html
fast/css/first-letter-block-form-controls-crash.html
fast/runin/nonblock-runin.html
http/tests/misc/empty-urls.html
http/tests/media/video-redirect.html

[Build Bot]

Created attachment 215459 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 215456 [details]
patch

Attachment 215456 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/17558061

New failing tests:
fast/forms/access-key-for-all-elements.html
http/tests/media/reload-after-dialog.html
fast/css/relative-position-replaced-in-table-display-crash.html
http/tests/security/local-video-source-from-remote.html
fullscreen/video-controls-drag.html
http/tests/security/local-video-src-from-remote.html
http/tests/media/video-play-progress.html
http/tests/media/video-play-stall-before-meta-data.html
fullscreen/full-screen-no-style-sharing.html
fast/forms/form-associated-element-crash3.html
http/tests/media/pdf-served-as-pdf.html
fast/regions/full-screen-video-from-region.html
fast/multicol/renderer-positioned-assert-crash.html
http/tests/media/video-buffered-range-contains-currentTime.html
http/tests/media/video-served-as-text.html
http/tests/media/video-load-suspend.html
fullscreen/full-screen-crash-offsetLeft.html
http/tests/media/video-error-abort.html
http/tests/media/text-served-as-text.html
fullscreen/full-screen-stacking-context.html
compositing/video/video-poster.html
http/tests/security/local-video-poster-from-remote.html
http/tests/media/video-preload.html
compositing/visibility/visibility-simple-video-layer.html
http/tests/media/video-accept-encoding.html
http/tests/media/video-throttled-load-metadata.html
fast/css/first-letter-block-form-controls-crash.html
fast/runin/nonblock-runin.html
http/tests/misc/empty-urls.html
http/tests/media/video-redirect.html

[Build Bot]

Created attachment 215466 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Mario Sanchez Prada]

Comment on attachment 215456 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215456&action=review

> Source/WebCore/ChangeLog:10
> +        treate these characters like attachments, for one. On the Mac platform, we should

s/treate/treat

> Source/WebCore/ChangeLog:14
> +        Tests: platform/mac/accessibility/media-emits-object-replacement.html
> +               platform/mac/accessibility/media-role-descriptions.html

I feel like these tests might work in a very similar way in GTK/EFL, but probably need some adaptation so I think it's ok to add them as mac specific for now

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1104
>       // If it's a control, it's not generic.
> -     if (isControl())
> +    if (isControl())

Stupid nit: you can line up the comment too :)

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2536
> +#if ENABLE(VIDEO)

Shouldn't this affect only the snippet below that is about video?

Also, why not using isHTMLVideoElement() as you do for the audio?

> Source/WebCore/editing/TextIterator.cpp:724
> -    if (m_emitsObjectReplacementCharacters && renderer && renderer->isReplaced()) {
> +    if (m_emitsObjectReplacementCharacters && isRendererReplacedElement(renderer)) {

Does this change actually makes a difference for the Mac? I guess so, otherwise you wouldn't be putting here nor modifying isRendererReplacedElement() to consider media renderers, but I could not find any usage of TextIteratorEmitsObjectReplacementCharacters by grepping the code (other than in GTK/EFL ports).

Maybe you're setting that behaviour from code out of WebKit's trunk? If so, please let me know because that also might affect my comment in https://bugs.webkit.org/show_bug.cgi?id=123153#c5 :)

[chris fleizach]

(In reply to comment #8)
> (From update of attachment 215456 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215456&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        treate these characters like attachments, for one. On the Mac platform, we should
> 
> s/treate/treat
> 
> > Source/WebCore/ChangeLog:14
> > +        Tests: platform/mac/accessibility/media-emits-object-replacement.html
> > +               platform/mac/accessibility/media-role-descriptions.html
> 
> I feel like these tests might work in a very similar way in GTK/EFL, but probably need some adaptation so I think it's ok to add them as mac specific for now
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1104
> >       // If it's a control, it's not generic.
> > -     if (isControl())
> > +    if (isControl())
> 
> Stupid nit: you can line up the comment too :)
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2536
> > +#if ENABLE(VIDEO)
> 
> Shouldn't this affect only the snippet below that is about video?

It looks like Audio is also controlled by the <video> feature flag since they end up being essentially the same render object

> 
> Also, why not using isHTMLVideoElement() as you do for the audio?
> 
> > Source/WebCore/editing/TextIterator.cpp:724
> > -    if (m_emitsObjectReplacementCharacters && renderer && renderer->isReplaced()) {
> > +    if (m_emitsObjectReplacementCharacters && isRendererReplacedElement(renderer)) {
> 
> Does this change actually makes a difference for the Mac? I guess so, otherwise you wouldn't be putting here nor modifying isRendererReplacedElement() to consider media renderers, but I could not find any usage of TextIteratorEmitsObjectReplacementCharacters by grepping the code (other than in GTK/EFL ports).
> 
> Maybe you're setting that behaviour from code out of WebKit's trunk? If so, please let me know because that also might affect my comment in https://bugs.webkit.org/show_bug.cgi?id=123153#c5 :)

[chris fleizach]

Created attachment 215530 [details]
patch

[chris fleizach]

Updated patch based on review. 

Removed the changes to handleReplacedElement() because as you noted correctly, they're not necessary for the mac

[Build Bot]

Comment on attachment 215530 [details]
patch

Attachment 215530 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/17078263

New failing tests:
fast/forms/access-key-for-all-elements.html
http/tests/media/reload-after-dialog.html
fast/css/relative-position-replaced-in-table-display-crash.html
http/tests/security/local-video-source-from-remote.html
fullscreen/video-controls-drag.html
http/tests/security/local-video-src-from-remote.html
http/tests/media/video-play-progress.html
http/tests/media/video-play-stall-before-meta-data.html
fullscreen/full-screen-no-style-sharing.html
fast/forms/form-associated-element-crash3.html
http/tests/media/pdf-served-as-pdf.html
fast/regions/full-screen-video-from-region.html
fast/multicol/renderer-positioned-assert-crash.html
http/tests/media/video-buffered-range-contains-currentTime.html
http/tests/media/video-served-as-text.html
http/tests/media/video-load-suspend.html
fullscreen/full-screen-crash-offsetLeft.html
http/tests/media/video-error-abort.html
http/tests/media/text-served-as-text.html
fullscreen/full-screen-stacking-context.html
compositing/video/video-poster.html
http/tests/security/local-video-poster-from-remote.html
http/tests/media/video-preload.html
compositing/visibility/visibility-simple-video-layer.html
http/tests/media/video-accept-encoding.html
http/tests/media/video-throttled-load-metadata.html
fast/css/first-letter-block-form-controls-crash.html
fast/runin/nonblock-runin.html
http/tests/misc/empty-urls.html
http/tests/media/video-redirect.html

[Build Bot]

Created attachment 215543 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 215530 [details]
patch

Attachment 215530 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/17098260

New failing tests:
fast/forms/access-key-for-all-elements.html
http/tests/media/reload-after-dialog.html
fast/css/relative-position-replaced-in-table-display-crash.html
fullscreen/video-controls-drag.html
fast/spatial-navigation/snav-media-elements.html
http/tests/media/video-play-progress.html
http/tests/appcache/video.html
http/tests/media/video-play-stall-before-meta-data.html
fullscreen/full-screen-no-style-sharing.html
fast/forms/form-associated-element-crash3.html
http/tests/media/pdf-served-as-pdf.html
fast/regions/full-screen-video-from-region.html
fast/multicol/renderer-positioned-assert-crash.html
http/tests/media/video-buffered-range-contains-currentTime.html
http/tests/media/video-served-as-text.html
http/tests/media/video-load-suspend.html
fullscreen/full-screen-crash-offsetLeft.html
http/tests/media/video-error-abort.html
http/tests/media/text-served-as-text.html
fullscreen/full-screen-stacking-context.html
compositing/video/video-poster.html
fullscreen/video-controls-override.html
http/tests/media/video-preload.html
compositing/visibility/visibility-simple-video-layer.html
http/tests/media/video-accept-encoding.html
http/tests/media/video-throttled-load-metadata.html
fast/css/first-letter-block-form-controls-crash.html
fast/runin/nonblock-runin.html
http/tests/misc/empty-urls.html
http/tests/media/video-redirect.html

[Build Bot]

Created attachment 215553 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[chris fleizach]

Created attachment 215575 [details]
patch

[Build Bot]

Comment on attachment 215575 [details]
patch

Attachment 215575 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/17078356

[chris fleizach]

Created attachment 215584 [details]
patch

[Build Bot]

Comment on attachment 215584 [details]
patch

Attachment 215584 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/16058433

New failing tests:
fast/forms/access-key-for-all-elements.html
media/before-load-member-access.html
media/W3C/audio/events/event_canplaythrough.html
media/media-can-play-type.html
media/controls-css-overload.html
media/W3C/audio/events/event_loadeddata.html
media/media-can-play-mpeg-audio.html
media/audio-mpeg4-supported.html
media/csp-blocks-video.html
media/W3C/audio/events/event_canplay_manual.html
media/media-blocked-by-willsendrequest.html
media/audio-mpeg-supported.html
media/event-attributes.html
media/W3C/audio/events/event_loadeddata_manual.html
media/media-can-play-mpeg4-video.html
fast/multicol/renderer-positioned-assert-crash.html
media/W3C/audio/events/event_canplay.html
media/media-can-play-octet-stream.html
media/constructors.html
media/audio-only-video-intrinsic-size.html
media/W3C/audio/events/event_loadedmetadata.html
media/controls-right-click-on-timebar.html
http/tests/misc/empty-urls.html
fast/runin/nonblock-runin.html
media/broken-video.html
media/controls-drag-timebar.html
media/controls-layout-direction.html
media/W3C/audio/events/event_canplaythrough_manual.html
fast/css/first-letter-block-form-controls-crash.html
fast/forms/form-associated-element-crash3.html

[Build Bot]

Created attachment 215614 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 215584 [details]
patch

Attachment 215584 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/17128444

New failing tests:
fast/forms/access-key-for-all-elements.html
media/before-load-member-access.html
media/W3C/audio/events/event_play.html
media/W3C/audio/events/event_canplaythrough.html
media/controls-css-overload.html
media/W3C/audio/events/event_loadeddata.html
media/audio-mpeg4-supported.html
media/W3C/audio/events/event_canplaythrough_manual.html
media/W3C/audio/events/event_canplay_manual.html
media/W3C/audio/events/event_order_canplay_playing.html
media/audio-mpeg-supported.html
media/audio-only-video-intrinsic-size.html
media/W3C/audio/events/event_loadeddata_manual.html
fast/multicol/renderer-positioned-assert-crash.html
media/W3C/audio/events/event_order_loadstart_progress.html
media/W3C/audio/events/event_canplay.html
media/constructors.html
media/W3C/audio/events/event_pause_manual.html
media/W3C/audio/events/event_loadedmetadata.html
media/W3C/audio/events/event_loadstart.html
http/tests/misc/empty-urls.html
fast/runin/nonblock-runin.html
media/W3C/audio/events/event_loadedmetadata_manual.html
media/W3C/audio/events/event_order_canplay_canplaythrough.html
media/W3C/audio/events/event_order_loadedmetadata_loadeddata.html
media/controls-drag-timebar.html
media/controls-layout-direction.html
media/W3C/audio/events/event_loadstart_manual.html
fast/css/first-letter-block-form-controls-crash.html
fast/forms/form-associated-element-crash3.html

[Build Bot]

Created attachment 215626 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Mario Sanchez Prada]

Comment on attachment 215584 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215584&action=review

> LayoutTests/ChangeLog:9
> +        Making media elements be replaced in TextIterating has the unfortunate effect
> +        of causing a new line to be inserted into any test using a <video> tag.

That's weird, specially because it seems the new line is neither inserted before nor after the video, but at some "random" place at the beginning of the output...

Maybe it's worth checking if that could be fixed instead of doing this massive rebaselining? In any case, if rebaselining turns out to be a better option I think we first need to figure out why all those tests keep failing in the EWS in the mac.

Setting the r- because of the EWS output (which seems related)

[chris fleizach]

(In reply to comment #23)
> (From update of attachment 215584 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215584&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        Making media elements be replaced in TextIterating has the unfortunate effect
> > +        of causing a new line to be inserted into any test using a <video> tag.
> 
> That's weird, specially because it seems the new line is neither inserted before nor after the video, but at some "random" place at the beginning of the output...
> 
> Maybe it's worth checking if that could be fixed instead of doing this massive rebaselining? In any case, if rebaselining turns out to be a better option I think we first need to figure out why all those tests keep failing in the EWS in the mac.

Spent a long time looking into this. I think the problem is essentially in TextIterator::handleReplacedElement

m_hasEmitted is set to true, even when something doesn't actually get emitted.

So when you have a replaced element (like videos will now be), you're going to get that new line. 

I tested a "fix" for that and it resolved the videos cases, but it removed a line in thousands of other cases

> 
> Setting the r- because of the EWS output (which seems related)

[Mario Sanchez Prada]

(In reply to comment #24)
> [...]
> > Maybe it's worth checking if that could be fixed instead of doing this
> > massive rebaselining? In any case, if rebaselining turns out to be a better 
> > option I think we first need to figure out why all those tests keep failing 
> > in the EWS in the mac.
> 
> Spent a long time looking into this. I think the problem is essentially in 
> TextIterator::handleReplacedElement
> 
> m_hasEmitted is set to true, even when something doesn't actually get emitted.
> 
> So when you have a replaced element (like videos will now be), you're going 
> to get that new line. 
> 
> I tested a "fix" for that and it resolved the videos cases, but it removed a 
> line in thousands of other cases

I see, and if that's the case I think I agree with you that it's better to rebaseline a bunch of tests than thousands. That's quite clear.

However, I still wonder if we could fix it for the video objects without breaking all the other stuff... I don't know what the "fix" that you have is about, but I've checked the code and I saw that the new line is being probably added in TextIterator::exitNode(), which calls to shouldEmitNewlineAfterNode() to know whether to add the new line or not, when m_hasEmitted is true.

So, what about combining your fix with some additional checks in shouldEmitNewLineAfterNode()? Would it be possible to make it work for videos that way without breaking the rest?

[chris fleizach]

(In reply to comment #25)
> (In reply to comment #24)
> > [...]
> > > Maybe it's worth checking if that could be fixed instead of doing this
> > > massive rebaselining? In any case, if rebaselining turns out to be a better 
> > > option I think we first need to figure out why all those tests keep failing 
> > > in the EWS in the mac.
> > 
> > Spent a long time looking into this. I think the problem is essentially in 
> > TextIterator::handleReplacedElement
> > 
> > m_hasEmitted is set to true, even when something doesn't actually get emitted.
> > 
> > So when you have a replaced element (like videos will now be), you're going 
> > to get that new line. 
> > 
> > I tested a "fix" for that and it resolved the videos cases, but it removed a 
> > line in thousands of other cases
> 
> I see, and if that's the case I think I agree with you that it's better to rebaseline a bunch of tests than thousands. That's quite clear.
> 
> However, I still wonder if we could fix it for the video objects without breaking all the other stuff... I don't know what the "fix" that you have is about, but I've checked the code and I saw that the new line is being probably added in TextIterator::exitNode(), which calls to shouldEmitNewlineAfterNode() to know whether to add the new line or not, when m_hasEmitted is true.
> 
> So, what about combining your fix with some additional checks in shouldEmitNewLineAfterNode()? Would it be possible to make it work for videos that way without breaking the rest?

Looked into this, the newline isn't being emitted by the video element itself (in exitNode or elsewhere). It's the fact that it's a replaced element now, which sets m_emitted = true, so that the next node, which might normally not emit a new line sees that something has been emitted (even though in this case something was not), which causes it to emit a newline

[chris fleizach]

Created attachment 216057 [details]
patch

[Build Bot]

Comment on attachment 216057 [details]
patch

Attachment 216057 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/21158090

New failing tests:
fast/forms/access-key-for-all-elements.html
media/track/track-load-error-readyState.html

[Build Bot]

Created attachment 216079 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 216057 [details]
patch

Attachment 216057 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/21268026

New failing tests:
fast/forms/access-key-for-all-elements.html
media/track/track-load-error-readyState.html

[Build Bot]

Created attachment 216086 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[chris fleizach]

Created attachment 216087 [details]
patch

[Mario Sanchez Prada]

Comment on attachment 216087 [details]
patch

It still looks a bit strange to me to have to do this rebaselining, but considering that you have already tried hard to find the right solution and that the massive rebaselining is not a critical logic change either, I'm ok with giving it a try now so VO gets sane in Mail

[WebKit Commit Bot]

Comment on attachment 216087 [details]
patch

Clearing flags on attachment: 216087

Committed r158743: <http://trac.webkit.org/changeset/158743>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.