Bug 132280

Summary: [ATK] Deprecate usage of logAccessibilityEvents() in layout tests
Product: WebKit Reporter: Xabier Rodríguez Calvar <calvaris>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apinheiro, jdiggs, mario, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Linux   
Attachments:
Description Flags
Patch proposal cfleizach: review+

Description Xabier Rodríguez Calvar 2014-04-28 08:33:52 PDT
platform/gtk/accessibility/aria-table-hierarchy.html [ Failure Pass ]
  platform/gtk/accessibility/aria-toggle-button-role.html [ Failure Pass ]
  platform/gtk/accessibility/button-accessible-name.html [ Failure Pass ]
  platform/gtk/accessibility/caret-browsing-select-focus.html [ Failure Pass ]
  platform/gtk/accessibility/caret-browsing-text-focus.html [ Failure Pass ]
  platform/gtk/accessibility/entry-and-password.html [ Failure Pass ]
  platform/gtk/accessibility/input-slider.html [ Failure Pass ]
  platform/gtk/accessibility/list-items-always-exposed.html [ Failure Pass ]
  platform/gtk/accessibility/media-controls-panel-title.html [ Failure Pass ]
  platform/gtk/accessibility/media-emits-object-replacement.html [ Failure Pass ]
  platform/gtk/accessibility/name-from-label.html [ Failure Pass ]
  platform/gtk/accessibility/object-with-title.html [ Failure Pass ]
  platform/gtk/accessibility/remapped-aria-crash.html [ Failure Pass ]
  platform/gtk/accessibility/replaced-objects-in-anonymous-blocks.html [ Failure Pass ]
  platform/gtk/accessibility/spans-paragraphs-and-divs.html [ Failure Pass ]
  platform/gtk/accessibility/spans.html [ Failure Pass ]
  platform/gtk/accessibility/table-hierarchy.html [ Failure Pass ]
  platform/gtk/accessibility/text-at-offset-embedded-objects.html [ Failure Pass ]
  platform/gtk/accessibility/text-at-offset-newlines.html [ Failure Pass ]
  platform/gtk/accessibility/text-at-offset-preformatted.html [ Failure Pass ]
  platform/gtk/accessibility/text-at-offset-simple.html [ Failure Pass ]
  platform/gtk/accessibility/text-at-offset-special-chars.html [ Failure Pass ]
  platform/gtk/accessibility/text-at-offset-textarea.html [ Failure Pass ]
  platform/gtk/accessibility/text-at-offset-textinput.html [ Failure Pass ]
  platform/gtk/accessibility/text-at-offset-wrapped-lines.html [ Failure Pass ]
  platform/gtk/accessibility/text-for-range-combo-box.html [ Failure Pass ]
  platform/gtk/accessibility/text-for-range-embedded-objects.html [ Failure Pass ]
  platform/gtk/accessibility/text-for-range-entry-and-password.html [ Failure Pass ]
  platform/gtk/accessibility/text-for-range-extraneous-whitespace.html [ Failure Pass ]
  platform/gtk/accessibility/text-for-range-formatted.html [ Failure Pass ]
  platform/gtk/accessibility/text-for-range-heading.html [ Failure Pass ]
  platform/gtk/accessibility/text-for-range-list-items.html [ Failure Pass ]
  platform/gtk/accessibility/text-for-range-simple.html [ Failure Pass ]
  platform/gtk/accessibility/text-for-range-table-cells.html [ Failure Pass ]
  platform/gtk/accessibility/text-for-range-with-link.html [ Failure Pass ]
  platform/gtk/accessibility/text-for-table.html [ Failure Pass ]
Comment 1 Radar WebKit Bug Importer 2014-04-28 08:34:56 PDT
<rdar://problem/16742674>
Comment 2 Mario Sanchez Prada 2014-05-29 01:20:40 PDT
I've tried hardly to get these tests failing in my local build (playing with different values of --repeat-each and --iterations) and could not get a single failure yet.

Also, I took a look to the archived test results in build.webkit.org and I can't see any of these tests failing in the latest builds either so I'm thinking that whatever the issue might be when you file this bug and added all these tests to TestExpectations might be gone now.

Would you be ok with me trying to remove then from here and see what the bots have to say about them now?
Comment 3 Mario Sanchez Prada 2014-05-29 03:00:12 PDT
I've checked the results over time using Carlos' WebKit test hunter[1], and I can't see any of these tests failing since 3 weeks ago (last failure was with [2]), which I'm not sure can be considered as "flaky enough" to put them in the TestExpectations file.

Calvaris, what do you think?

[1] https://github.com/clopez/webkit-testhunter
[2] http://trac.webkit.org/changeset/168469
Comment 4 Xabier Rodríguez Calvar 2014-05-29 03:05:00 PDT
(In reply to comment #3)
> I've checked the results over time using Carlos' WebKit test hunter[1], and I can't see any of these tests failing since 3 weeks ago (last failure was with [2]), which I'm not sure can be considered as "flaky enough" to put them in the TestExpectations file.
> 
> Calvaris, what do you think?
> 
> [1] https://github.com/clopez/webkit-testhunter
> [2] http://trac.webkit.org/changeset/168469

By using the script I still see some text failures not so long ago so they still fail, though you consider those do not happen ofter enough.
Comment 5 Mario Sanchez Prada 2014-05-29 03:21:37 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > I've checked the results over time using Carlos' WebKit test hunter[1], and I can't see any of these tests failing since 3 weeks ago (last failure was with [2]), which I'm not sure can be considered as "flaky enough" to put them in the TestExpectations file.
> > 
> > Calvaris, what do you think?
> > 
> > [1] https://github.com/clopez/webkit-testhunter
> > [2] http://trac.webkit.org/changeset/168469
> 
> By using the script I still see some text failures not so long ago so they still fail, though you consider those do not happen ofter enough.

Good point. I've just found the text diff for one of those tests (platform/gtk/accessibility/text-at-offset-newlines.html) in r168469 and this is what I saw: 

--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/platform/gtk/accessibility/text-at-offset-newlines-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/platform/gtk/accessibility/text-at-offset-newlines-actual.txt
@@ -1,3 +1,24 @@
+Accessibility object emitted "state-change:busy = 1" / Name: "(No name)" / Role: 93
+Accessibility object emitted "children-changed:add = -1" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 48
+Accessibility object emitted "children-changed:add = -1" / Name: "(No name)" / Role: 93
+Accessibility object emitted "children-changed:add = -1" / Name: "(No name)" / Role: 93
+Accessibility object emitted "children-changed:add = -1" / Name: "(No name)" / Role: 93
+Accessibility object emitted "children-changed:add = -1" / Name: "(No name)" / Role: 93
+Accessibility object emitted "children-changed:add = -1" / Name: "(No name)" / Role: 93
+Accessibility object emitted "property-change:accessible-parent" / Name: "(No name)" / Role: 71
 This is a test. This is the second sentence. And this the third.
 
 This tests the ability to get element text for different granularities and offsets.


This is very weird. For some reason, the test is enabling "logging accessibility events" (as if accessibilityController.logAccessibilityEvents()) was called for those tests, which is not), and that is causing that all that text is being dumped "accidentally", therefore causing the text failure. This is *clearly* the cause of that problem.

Now, to be honest, I'm not sure why that thing happens, although it sounds like related to the setup/teardown processes that happens for every single test, which seems indeed to be flaky, at least in what it relates to logAccessibilityEvents() which now I reckon that has not been a very good idea right from the start :/, as it has presented many problems and, on top of that, it should not be needed if we had addNotificationListener() implemented, which we do have now.

Also, the only tests actually using logAccessibilityEvents() these days in GTK/EFL are this ones: 

  platform/gtk/accessibility/aria-slider-required-attributes.html
  platform/gtk/accessibility/combo-box-collapsed-selection-changed.html

So, all in all, perhaps it's better to adapt those tests not to use logAccessibilityEvents() at all, and replace the implementation in WKTR with a dummy one, so it does not cause more harm anymore (we can't simply remove it, since logAccessibilityEvents() is implemented in the win port).

I'll work on this change soon, probably tomorrow (as today I need to leave early due to personal matters)
Comment 6 Mario Sanchez Prada 2014-05-29 09:04:11 PDT
Indeed, after doing some local tests I'm pretty sure we are better off deprecating the logAccessibilityEvents() method and using addNotificationListener instead, so I'm now renaming this bug and soon proposing a patch for review.
Comment 7 Mario Sanchez Prada 2014-05-30 06:55:13 PDT
Created attachment 232293 [details]
Patch proposal

Attaching a new patch deprecating logAccessibilityEvents() from ATK's implementation of the WKTR.

With regard to the following tests listed as "Failure" in TestExpectations:

  1. platform/gtk/accessibility/aria-slider-required-attributes.html
  2. platform/gtk/accessibility/combo-box-collapsed-selection-changed.html

...I'll tackle those in separate bugs, since there are different things to do in each case to migrate out of logAccessibilityEvents().

Please review. Thanks!
Comment 8 Mario Sanchez Prada 2014-05-30 08:21:02 PDT
Committed r169485: <http://trac.webkit.org/changeset/169485>
Comment 9 Mario Sanchez Prada 2014-05-30 08:22:12 PDT
(In reply to comment #8)
> Committed r169485: <http://trac.webkit.org/changeset/169485>

Sorry, this was a mistake. This bug is still open
Comment 10 Mario Sanchez Prada 2014-05-30 09:47:41 PDT
Committed r169487: <http://trac.webkit.org/changeset/169487>