Bug 131294 - [AX][GTK] No new lines in some AX tests output
Summary: [AX][GTK] No new lines in some AX tests output
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-07 00:49 PDT by Sergio Villar Senin
Modified: 2014-04-09 08:06 PDT (History)
12 users (show)

See Also:


Attachments
Patch proposal (6.68 KB, patch)
2014-04-09 03:49 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (6.68 KB, patch)
2014-04-09 05:18 PDT, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2014-04-07 00:49:05 PDT
Several tests fail:
accessibility/adjacent-continuations-cause-assertion-failure.html
accessibility/deleting-iframe-destroys-axcache.html
accessibility/div-within-anchors-causes-crash.html
accessibility/lists.html

This is the kind of errors bots are observing:
-AXRole: AXWebArea
- AXRole: AXSection AXValue: y
- AXRole: AXSection AXValue: End of test
+AXRole: AXWebArea AXValue: x<\n>y<\n>z<\n>End of test<\n>Make sure that a debug assert is not triggered when constructing the accessibility tree for this page.<\n><\n>On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".<\n><\n><\n>
Comment 1 Radar WebKit Bug Importer 2014-04-07 00:49:22 PDT
<rdar://problem/16537388>
Comment 2 Sergio Villar Senin 2014-04-07 00:58:43 PDT
Actually the failures is not only about the "</n>" thing, but it seems that some roles for children are not printed.
Comment 3 Sergio Villar Senin 2014-04-07 01:20:19 PDT
Another 3 more tests

platform/gtk/accessibility/entry-and-password.html
platform/gtk/accessibility/media-emits-object-replacement.html
platform/gtk/accessibility/replaced-objects-in-anonymous-blocks.html
Comment 4 Mario Sanchez Prada 2014-04-08 08:48:31 PDT
Curiously enough, I can only reproduce this with WebKit2 and WKTR. With WebKit1 and DRT everything is fine.

I'm taking a look to this right now
Comment 5 Martin Robinson 2014-04-08 09:05:57 PDT
(In reply to comment #4)
> Curiously enough, I can only reproduce this with WebKit2 and WKTR. With WebKit1 and DRT everything is fine.
> 
> I'm taking a look to this right now

Great. And FWIW, WebKit1 is toast soon.
Comment 6 Mario Sanchez Prada 2014-04-08 09:19:37 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Curiously enough, I can only reproduce this with WebKit2 and WKTR. With WebKit1 and DRT everything is fine.
> > 
> > I'm taking a look to this right now
> 
> Great. And FWIW, WebKit1 is toast soon.

I know. Just mentioned it because I found the difference in behaviour quite interesting, as it could mean the problem is not in WebKit itself, but in the testing tools
Comment 7 Carlos Alberto Lopez Perez 2014-04-08 10:18:31 PDT
Two more tests are also failing:

* accessibility/children-changed-sends-notification.html
* accessibility/notification-listeners.html


The diffs are the following:

--- /stuff/webkit/webkit/layout-test-results/accessibility/children-changed-sends-notification-expected.txt
+++ /stuff/webkit/webkit/layout-test-results/accessibility/children-changed-sends-notification-actual.txt
@@ -6,12 +6,8 @@
 Plain text paragraph 
 
 End of test
-PARAGRAPH notification: AXChildrenRemoved
-GLOBAL notification: AXChildrenRemoved on element with role AXRole: AXParagraph
-PARAGRAPH notification: AXChildrenAdded
-GLOBAL notification: AXChildrenAdded on element with role AXRole: AXParagraph
 PASS paragraphNotificationCount is globalNotificationCount
-PASS globalNotificationCount is 2
+FAIL globalNotificationCount should be 2. Was 0.
 PASS successfullyParsed is true
 
 TEST COMPLETE

--- /stuff/webkit/webkit/layout-test-results/accessibility/notification-listeners-expected.txt
+++ /stuff/webkit/webkit/layout-test-results/accessibility/notification-listeners-actual.txt
@@ -7,11 +7,13 @@
 Slider
 SELECT AXInvalidStatusChanged
 GLOBAL AXInvalidStatusChanged on element with role AXRole: AXComboBox
+SLIDER AXChildrenAdded
+GLOBAL AXChildrenAdded on element with role AXRole: AXSlider
 SLIDER AXValueChanged
 GLOBAL AXValueChanged on element with role AXRole: AXSlider
 PASS selectNotificationCount is 1
-PASS sliderNotificationCount is 1
-PASS globalNotificationCount is 2
+FAIL sliderNotificationCount should be 1. Was 2.
+FAIL globalNotificationCount should be 2. Was 3.
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 8 Sergio Villar Senin 2014-04-08 11:21:17 PDT
(In reply to comment #7)
> Two more tests are also failing:
> 
> * accessibility/children-changed-sends-notification.html
> * accessibility/notification-listeners.html

I didn't include them because there seem to be different types of failures. The differences in the output with the expected results are not the same.
Comment 9 Carlos Alberto Lopez Perez 2014-04-08 12:19:11 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Two more tests are also failing:
> > 
> > * accessibility/children-changed-sends-notification.html
> > * accessibility/notification-listeners.html
> 
> I didn't include them because there seem to be different types of failures. The differences in the output with the expected results are not the same.

Ok. Reported it on bug #131380
Comment 10 Mario Sanchez Prada 2014-04-09 02:25:29 PDT
(In reply to comment #0)
> accessibility/lists.html

Quick thing: this test is not failing due to the same issue, but due to an <hr> being removed in http://trac.webkit.org/changeset/166175, which just needs rebaselining.

I'll land a gardening patch related to that independently
Comment 11 Mario Sanchez Prada 2014-04-09 03:41:23 PDT
(In reply to comment #10)
> (In reply to comment #0)
> > accessibility/lists.html
> 
> Quick thing: this test is not failing due to the same issue, but due to an <hr> being removed in http://trac.webkit.org/changeset/166175, which just needs rebaselining.
> 
> I'll land a gardening patch related to that independently

http://trac.webkit.org/changeset/167009
Comment 12 Mario Sanchez Prada 2014-04-09 03:47:24 PDT
The problem is caused by http://trac.webkit.org/changeset/166194 because, as per my own recommendation, the call to updateRoleAfterChildrenCreation() is happening now in AccessibilityObject::updateBackingStore(), which unveiled a problem in the way we determined which ATK interfaces to implement in the ATK wrapper.

Because of this issue, all these tests were printing a lot of text when called the stringValue() function of AccessibilityUIElement through WKTR, since the ATK object for the WebArea would be recognized as implementing AtkText. Therefore, calling atk_text_get_text() over it would return a textual representation of the whole web page (internally using WebCore's TextIterator), which resulted in all that text being shown in the output.

And of course, the javascript code in buildAccessibilityTree() would not keep digging into the subtree for the WebArea because, due to the WebArea already returning a lot of text through the AtkText interface, and due to that text already including the 'End of test' string (used as condition to stop digging more in the tree).

So, we need to make sure we never, ever, implement AtkText for objects with the WebArea role. Patch coming soon
Comment 13 Mario Sanchez Prada 2014-04-09 03:49:47 PDT
Created attachment 228953 [details]
Patch proposal

See the patch proposal attached. Notice that this patch not only prevents the WebArea object from implementing AtkText but also AtkHypertext, which I believe is correct because that other interface will be implemented anyway by the real containers of the different blocks of content in the web page.
Comment 14 Mario Sanchez Prada 2014-04-09 05:18:30 PDT
Created attachment 228956 [details]
Patch proposal

Forgot the parenthesis in the previous patch. Adding them now to make EFL happy (well, and because I think it's clearer this way too)
Comment 15 Martin Robinson 2014-04-09 07:36:59 PDT
Comment on attachment 228956 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=228956&action=review

> Source/WebCore/ChangeLog:8
> +        Do not implement Hypetext or AtkText for the WebArea. Those

Small typo: Hypetext -> Hypertext
Comment 16 Mario Sanchez Prada 2014-04-09 08:02:33 PDT
(In reply to comment #15)
> (From update of attachment 228956 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228956&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Do not implement Hypetext or AtkText for the WebArea. Those
> 
> Small typo: Hypetext -> Hypertext

Oops! Thanks
Comment 17 Mario Sanchez Prada 2014-04-09 08:06:31 PDT
Committed r167011: <http://trac.webkit.org/changeset/167011>