Bug 105538 - [GTK] Test /webkit/atk/getTextInParagraphAndBodyModerate fails
Summary: [GTK] Test /webkit/atk/getTextInParagraphAndBodyModerate fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mario Sanchez Prada
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2012-12-20 08:06 PST by Carlos Garcia Campos
Modified: 2013-09-10 01:51 PDT (History)
18 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 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
Comment 1 Carlos Garcia Campos 2012-12-20 08:22:20 PST
Skipped in r138256
Comment 2 Joanmarie Diggs 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 <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?
Comment 3 Mario Sanchez Prada 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.
Comment 4 EFL EWS Bot 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
Comment 5 Build Bot 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
Comment 6 kov's GTK+ EWS bot 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
Comment 7 Early Warning System Bot 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
Comment 8 Early Warning System Bot 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
Comment 9 Build Bot 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
Comment 10 EFL EWS Bot 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
Comment 11 Mario Sanchez Prada 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
Comment 12 Mario Sanchez Prada 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?
Comment 13 Mario Sanchez Prada 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).
Comment 14 Mario Sanchez Prada 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!
Comment 15 chris fleizach 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
Comment 16 Mario Sanchez Prada 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.
Comment 17 Mario Sanchez Prada 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.
Comment 18 chris fleizach 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())
Comment 19 Mario Sanchez Prada 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!
Comment 20 Mario Sanchez Prada 2013-09-10 01:51:13 PDT
Committed r155428: <http://trac.webkit.org/changeset/155428>