Bug 105538

Summary:

[GTK] Test /webkit/atk/getTextInParagraphAndBodyModerate fails

Product:

WebKit

Reporter:

Carlos Garcia Campos <cgarcia>

Component:

Tools / Tests

Assignee:

Mario Sanchez Prada <mario>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboxhall, apinheiro, buildbot, cdumez, cfleizach, commit-queue, dmazzoni, eflews.bot, esprehn+autocc, gtk-ews, gyuyoung.kim, jdiggs, mario, rakuco, rego+ews, rniwa, webkit-ews, xan.lopez

Priority:

Keywords:

Gtk

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch proposal	none
Patch proposal	none
Patch proposal + Layout Test	none
Patch proposal plus new Layout test	cfleizach: review+

Carlos Garcia Campos

Reported 2012-12-20 08:06:13 PST

/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

Attachments
Patch proposal (7.71 KB, patch) 2013-06-28 10:29 PDT, Mario Sanchez Prada	no flags	Details Formatted Diff Diff
Patch proposal (6.21 KB, patch) 2013-06-28 15:13 PDT, Mario Sanchez Prada	no flags	Details Formatted Diff Diff
Patch proposal + Layout Test (11.69 KB, patch) 2013-07-17 02:55 PDT, Mario Sanchez Prada	no flags	Details Formatted Diff Diff
Patch proposal plus new Layout test (15.50 KB, patch) 2013-09-09 10:10 PDT, Mario Sanchez Prada	cfleizach: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Carlos Garcia Campos

Comment 1 2012-12-20 08:22:20 PST

Skipped in r138256

Joanmarie Diggs

Comment 2 2012-12-21 01:51:24 PST

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 tags in this test: "<html><body>This is a test.Hello world. This sentence is green. 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?

Mario Sanchez Prada

Comment 3 2013-06-28 10:29:55 PDT

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.

EFL EWS Bot

Comment 4 2013-06-28 10:33:31 PDT

Comment on attachment 205724 [details] Patch proposal Attachment 205724 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/934945

Build Bot

Comment 5 2013-06-28 10:34:07 PDT

Comment on attachment 205724 [details] Patch proposal Attachment 205724 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/981768

kov's GTK+ EWS bot

Comment 6 2013-06-28 10:35:41 PDT

Comment on attachment 205724 [details] Patch proposal Attachment 205724 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/981770

Early Warning System Bot

Comment 7 2013-06-28 10:42:52 PDT

Comment on attachment 205724 [details] Patch proposal Attachment 205724 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/934947

Early Warning System Bot

Comment 8 2013-06-28 10:44:38 PDT

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

Build Bot

Comment 9 2013-06-28 10:50:06 PDT

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

EFL EWS Bot

Comment 10 2013-06-28 10:56:39 PDT

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

Mario Sanchez Prada

Comment 11 2013-06-28 15:13:14 PDT

Created attachment 205749 [details] Patch proposal It seems I uploaded the wrong patch before. Now uploading the right one

Mario Sanchez Prada

Comment 12 2013-07-10 09:50:24 PDT

(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?

Mario Sanchez Prada

Comment 13 2013-07-17 02:55:12 PDT

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).

Mario Sanchez Prada

Comment 14 2013-07-29 10:33:02 PDT

(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!

chris fleizach

Comment 15 2013-09-06 08:55:53 PDT

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

Mario Sanchez Prada

Comment 16 2013-09-09 09:02:20 PDT

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.

Mario Sanchez Prada

Comment 17 2013-09-09 10:10:36 PDT

Created attachment 211053 [details] Patch proposal plus new Layout test Attaching a new patch trying to address the concerns from previous review.

chris fleizach

Comment 18 2013-09-09 10:13:41 PDT

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())

Mario Sanchez Prada

Comment 19 2013-09-10 01:41:41 PDT

(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!

Mario Sanchez Prada

Comment 20 2013-09-10 01:51:13 PDT

Committed r155428: <http://trac.webkit.org/changeset/155428>

Note You need to log in before you can comment on or make changes to this bug.