Bug 132280 – [ATK] Deprecate usage of logAccessibilityEvents() in layout tests

• Component: Accessibility
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

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 ]

[Radar WebKit Bug Importer]

<rdar://problem/16742674>

[Mario Sanchez Prada]

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?

[Mario Sanchez Prada]

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

[Xabier Rodríguez Calvar]

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

[Mario Sanchez Prada]

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

[Mario Sanchez Prada]

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.

[Mario Sanchez Prada]

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!

[Mario Sanchez Prada]

Committed r169485: <http://trac.webkit.org/changeset/169485>

[Mario Sanchez Prada]

(In reply to comment #8)
> Committed r169485: <http://trac.webkit.org/changeset/169485>

Sorry, this was a mistake. This bug is still open

[Mario Sanchez Prada]

Committed r169487: <http://trac.webkit.org/changeset/169487>