/webkit/atk/getTextInParagraphAndBodyModerate: ** ERROR:../../Source/WebKit/gtk/tests/testatk.c:1002:testWebkitAtkGetTextInParagraphAndBodyModerate: assertion failed (text == "Hello world.\nThis sentence is green.\nThis one is not."): ("Hello world.This sentence is green.This one is not." == "Hello world.\nThis sentence is green.\nThis one is not.") FAIL It's failing in all the bots
Skipped in r138256
The regression is the result of this commit: http://trac.webkit.org/changeset/137946 And the test is failing because the newline chars associated with the <br /> tags in this test: "<html><body><p>This is a test.</p>Hello world.<br /><font color='#00cc00'>This sentence is green.</font><br />This one is not.</body></html>" Used to be represented in the text: "Hello world.\nThis sentence is green.\nThis one is not." Now they are being stripped out: "Hello world.This sentence is green.This one is not." Dominic and Chris: Is stripping them out the expected behavior for the Chromium and/or Mac ports? Or is this a cross-platform regression that was only caught on the Gtk/Atk port?
Created attachment 205724 [details] Patch proposal The following patch fixes the issue for GTK+ and hopefully won't break any test in other platforms, according to what I could see by checking the tests and expections modified in SVN revision r151131, although I'm not completely sure yet (let the EWS work). Have a nice weekend.
Comment on attachment 205724 [details] Patch proposal Attachment 205724 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/934945
Comment on attachment 205724 [details] Patch proposal Attachment 205724 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/981768
Comment on attachment 205724 [details] Patch proposal Attachment 205724 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/981770
Comment on attachment 205724 [details] Patch proposal Attachment 205724 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/934947
Comment on attachment 205724 [details] Patch proposal Attachment 205724 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/998290
Comment on attachment 205724 [details] Patch proposal Attachment 205724 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/998288
Comment on attachment 205724 [details] Patch proposal Attachment 205724 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/934946
Created attachment 205749 [details] Patch proposal It seems I uploaded the wrong patch before. Now uploading the right one
(In reply to comment #11) > Created an attachment (id=205749) [details] > Patch proposal > > It seems I uploaded the wrong patch before. Now uploading the right one Ping reviewers?
Created attachment 206873 [details] Patch proposal + Layout Test I just realized that a Layout test might serve well here, so I'm now attaching a new patch including one. Still, I've added lines to TestExpectations files for Mac and Win, since they will probably output slightly different results (e.g. they don't change the representation of the \n as we do in GTK/EFL's DRT).
(In reply to comment #13) > Created an attachment (id=206873) [details] > Patch proposal + Layout Test > > I just realized that a Layout test might serve well here, so I'm now attaching > a new patch including one. Still, I've added lines to TestExpectations files > for Mac and Win, since they will probably output slightly different results > (e.g. they don't change the representation of the \n as we do in GTK/EFL's > DRT). Chris, could you take a look to this one as well when you have some time? Btw, the reason why I'm skipping it for Mac and Win is because EFL/GTK have their own fancy way of printing line breaks through DRT/WKTR, hence the output won't be exactly the same. Thanks!
Comment on attachment 206873 [details] Patch proposal + Layout Test View in context: https://bugs.webkit.org/attachment.cgi?id=206873&action=review few questions interspersed. thanks > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1574 > + if (builder.length() && childText != "\n") do we need to worry about \r or do these methods only ever emit \n > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1580 > + return builder.toString().replace("\n ", "\n").stripWhiteSpace().simplifyWhiteSpace(isHTMLSpaceButNotLineBreak); why do we need to do this replacement? is this looking for \n in the middle of text, and by default the is emitting "\n "? aren't you not appending the extra space specifically in the above code change? > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:638 > + if (m_renderer->isBR()) can we instead call m_renderer->text() instead of hard coding a new line? will that give the same results? > LayoutTests/accessibility/paragraph-with-linebreaks-expected.txt:13 > +Value of the paragraph with break lines: AXValue: Lorem ipsum dolor sit amet,<\n>consectetur adipiscing elit.<\n>Aliquam faucibus diam sit amet nunc vestibulum auctor. is this what we're expecting to be outputted? <\n> i would have thought we'd see the manifestation of the new line
Comment on attachment 206873 [details] Patch proposal + Layout Test View in context: https://bugs.webkit.org/attachment.cgi?id=206873&action=review >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1574 >> + if (builder.length() && childText != "\n") > > do we need to worry about \r or do these methods only ever emit \n Good point. I think we might need to do it since I think that's something it might happen in Windows environments. Perhaps using isHTMLLineBreak() here could be the right thing to do then. >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1580 >> + return builder.toString().replace("\n ", "\n").stripWhiteSpace().simplifyWhiteSpace(isHTMLSpaceButNotLineBreak); > > why do we need to do this replacement? is this looking for \n in the middle of text, and by default the is emitting "\n "? aren't you not appending the extra space specifically in the above code change? The problem is that we the code above we only avoid adding a extra space before a line break (as we check that the new test to be appended is a "\n"), but we don't do the same when adding a extra space after the linebreak. And I guess we probably should do that check as well, and get rid of this ugly replacement. >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:638 >> + if (m_renderer->isBR()) > > can we instead call m_renderer->text() instead of hard coding a new line? will that give the same results? Yes, we can do that. I'll change it in a new patch >> LayoutTests/accessibility/paragraph-with-linebreaks-expected.txt:13 >> +Value of the paragraph with break lines: AXValue: Lorem ipsum dolor sit amet,<\n>consectetur adipiscing elit.<\n>Aliquam faucibus diam sit amet nunc vestibulum auctor. > > is this what we're expecting to be outputted? <\n> > i would have thought we'd see the manifestation of the new line This is something we have in our DRT to be able to check things like object replacement characters and line breaks while keeping the test results easy to read, so that's why I had to add that. I'm not entirely convinced about using that approach with line breaks to be honest, but it already affects some tests and did not want to change it right now. Maybe it would be useful to revisit that issue in a later bug, though. But perhaps for now I should provide GTK/EFL specific expectations instead of a cross platform one (and skipping the test in the Mac and Win platforms), so I don't "force" other ports to do the same thing.
Created attachment 211053 [details] Patch proposal plus new Layout test Attaching a new patch trying to address the concerns from previous review.
Comment on attachment 211053 [details] Patch proposal plus new Layout test View in context: https://bugs.webkit.org/attachment.cgi?id=211053&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1552 > + if (isHTMLLineBreak(childText[0]) || isHTMLLineBreak(builder[builder.length() - 1])) looks like you can just return !(isHTMLLineBreak() or isHTMLLineBreak())
(In reply to comment #18) > (From update of attachment 211053 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211053&action=review > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1552 > > + if (isHTMLLineBreak(childText[0]) || isHTMLLineBreak(builder[builder.length() - 1])) > > looks like you can just > > return !(isHTMLLineBreak() or isHTMLLineBreak()) Definitely. I'll change that before landing. Thanks!
Committed r155428: <http://trac.webkit.org/changeset/155428>