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
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
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 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 :)
(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 :)
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
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
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
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 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)
(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)
(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?
(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
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
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 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
2013-10-29 17:38 PDT, chris fleizach
2013-10-29 17:41 PDT, chris fleizach
2013-10-29 18:08 PDT, Build Bot
2013-10-29 20:29 PDT, Build Bot
2013-10-30 11:09 PDT, chris fleizach
2013-10-30 11:51 PDT, Build Bot
2013-10-30 12:46 PDT, Build Bot
2013-10-30 16:56 PDT, chris fleizach
2013-10-30 17:39 PDT, chris fleizach
buildbot: commit-queue-
2013-10-30 22:25 PDT, Build Bot
2013-10-31 00:36 PDT, Build Bot
2013-11-05 12:16 PST, chris fleizach
2013-11-05 14:02 PST, Build Bot
2013-11-05 14:58 PST, Build Bot
2013-11-05 15:00 PST, chris fleizach