Bug 123479 - AX: Audio and Video attachments are not output to VoiceOver
Summary: AX: Audio and Video attachments are not output to VoiceOver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-10-29 16:28 PDT by chris fleizach
Modified: 2013-11-06 06:15 PST (History)
5 users (show)

See Also:

Attachments
patch (13.68 KB, patch)
2013-10-29 17:38 PDT, chris fleizach 		no flags Details | Formatted Diff | Diff
patch (13.54 KB, patch)
2013-10-29 17:41 PDT, chris fleizach 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (360.53 KB, application/zip)
2013-10-29 18:08 PDT, Build Bot 		no flags Details
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (347.71 KB, application/zip)
2013-10-29 20:29 PDT, Build Bot 		no flags Details
patch (13.11 KB, patch)
2013-10-30 11:09 PDT, chris fleizach 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (400.96 KB, application/zip)
2013-10-30 11:51 PDT, Build Bot 		no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (364.19 KB, application/zip)
2013-10-30 12:46 PDT, Build Bot 		no flags Details
patch (44.28 KB, patch)
2013-10-30 16:56 PDT, chris fleizach 		buildbot: commit-queue-
Details | Formatted Diff | Diff
patch (44.09 KB, patch)
2013-10-30 17:39 PDT, chris fleizach 		mario: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (465.58 KB, application/zip)
2013-10-30 22:25 PDT, Build Bot 		no flags Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (407.16 KB, application/zip)
2013-10-31 00:36 PDT, Build Bot 		no flags Details
patch (223.19 KB, patch)
2013-11-05 12:16 PST, chris fleizach 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (448.02 KB, application/zip)
2013-11-05 14:02 PST, Build Bot 		no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (501.46 KB, application/zip)
2013-11-05 14:58 PST, Build Bot 		no flags Details
patch (223.65 KB, patch)
2013-11-05 15:00 PST, chris fleizach 		no flags Details | Formatted Diff | Diff
Show Obsolete (14) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2013-10-29 16:28:08 PDT 
Audio attachments in Mail are not navigable by VoiceOver or other apps that use accessibility.

<rdar://problem/15239047>
Comment 1 chris fleizach 2013-10-29 17:38:15 PDT 
Created attachment 215454 [details]
patch
Comment 2 chris fleizach 2013-10-29 17:41:20 PDT 
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
Comment 3 chris fleizach 2013-10-29 17:41:56 PDT 
Created attachment 215456 [details]
patch

patch to clean up some erroneous comments left in layout test
Comment 4 Build Bot 2013-10-29 18:08:44 PDT 
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
Comment 5 Build Bot 2013-10-29 18:08:47 PDT 
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
Comment 6 Build Bot 2013-10-29 20:29:04 PDT 
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
Comment 7 Build Bot 2013-10-29 20:29:06 PDT 
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
Comment 8 Mario Sanchez Prada 2013-10-30 04:38:02 PDT 
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 :)
Comment 9 chris fleizach 2013-10-30 11:06:05 PDT 
(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 :)
Comment 10 chris fleizach 2013-10-30 11:09:40 PDT 
Created attachment 215530 [details]
patch
Comment 11 chris fleizach 2013-10-30 11:10:29 PDT 
Updated patch based on review. 

Removed the changes to handleReplacedElement() because as you noted correctly, they're not necessary for the mac
Comment 12 Build Bot 2013-10-30 11:51:26 PDT 
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
Comment 13 Build Bot 2013-10-30 11:51:28 PDT 
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
Comment 14 Build Bot 2013-10-30 12:46:02 PDT 
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
Comment 15 Build Bot 2013-10-30 12:46:04 PDT 
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
Comment 16 chris fleizach 2013-10-30 16:56:49 PDT 
Created attachment 215575 [details]
patch
Comment 17 Build Bot 2013-10-30 17:29:07 PDT 
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
Comment 18 chris fleizach 2013-10-30 17:39:42 PDT 
Created attachment 215584 [details]
patch
Comment 19 Build Bot 2013-10-30 22:25:36 PDT 
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
Comment 20 Build Bot 2013-10-30 22:25:38 PDT 
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
Comment 21 Build Bot 2013-10-31 00:36:26 PDT 
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
Comment 22 Build Bot 2013-10-31 00:36:28 PDT 
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
Comment 23 Mario Sanchez Prada 2013-10-31 04:31:09 PDT 
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)
Comment 24 chris fleizach 2013-11-04 17:00:23 PST 
(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)
Comment 25 Mario Sanchez Prada 2013-11-05 01:42:44 PST 
(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?
Comment 26 chris fleizach 2013-11-05 10:57:52 PST 
(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
Comment 27 chris fleizach 2013-11-05 12:16:36 PST 
Created attachment 216057 [details]
patch
Comment 28 Build Bot 2013-11-05 14:02:26 PST 
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
Comment 29 Build Bot 2013-11-05 14:02:29 PST 
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
Comment 30 Build Bot 2013-11-05 14:58:19 PST 
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
Comment 31 Build Bot 2013-11-05 14:58:22 PST 
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
Comment 32 chris fleizach 2013-11-05 15:00:02 PST 
Created attachment 216087 [details]
patch
Comment 33 Mario Sanchez Prada 2013-11-06 03:22:40 PST 
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
Comment 34 WebKit Commit Bot 2013-11-06 06:14:59 PST 
Comment on attachment 216087 [details]
patch

Clearing flags on attachment: 216087

Committed r158743: <http://trac.webkit.org/changeset/158743>
Comment 35 WebKit Commit Bot 2013-11-06 06:15:03 PST 
All reviewed patches have been landed.  Closing bug.