Need e10s tests for accessibility

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: billm, Assigned: yzen)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox42 affected)

Details

Attachments

(1 attachment, 9 obsolete attachments)

The crash rate lately from a11y has been very high and some of it has been caused by regressions. Having automated tests would really help. We should investigate running the current a11y tests with e10s enabled.
(In reply to Bill McCloskey (:billm) from comment #0)
> The crash rate lately from a11y has been very high and some of it has been
> caused by regressions. Having automated tests would really help. We should

yes please!!! have someone to work on it?

> investigate running the current a11y tests with e10s enabled.

unfortunately I suspect that won't work out well.  mochitest-a11y opens all the tests as  chrome://foo/bar.whatever (I guess we probably only care about the html ones here) and then runs a bunch of chrome js.  I'm not really sure if that'll end up running in the parent or the child, but I suspect it needs parent only things and of course to do anything useful the document needs to be in the child.  maybe shims will be enough to get it running? I'm not actually sure now that I think about it.

Even if you did get it running I think it would basically just be crash tests since the xpcom events and methods are only delt with in the process with the accessible and the content node for it.  Of course that would be much better than nothing.
(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> (In reply to Bill McCloskey (:billm) from comment #0)

> Even if you did get it running I think it would basically just be crash
> tests since the xpcom events and methods are only delt with in the process
> with the accessible and the content node for it.  Of course that would be
> much better than nothing.

I think we decided in IRC that this is a good first step. I'll try to find an owner.
Flags: needinfo?(dbolter)
Yura mentioned he would comment on where/how Marionnette tests can play a role.
Flags: needinfo?(yzenevich)
Assignee

Comment 4

4 years ago
So I guess the context of this bug has little to do with creating/running tests that replicate user actions (either within chrome or content). That is what Marionette is usually used for. Through, David and Trevor, you mentioned some failing Marionette tests, could you give some more context or point me to that?
Flags: needinfo?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #4)
> So I guess the context of this bug has little to do with creating/running
> tests that replicate user actions (either within chrome or content).

As I understand it we need a way to exercise and test the desktop layer of our e10s code. I was hoping Marionette might be a way? Mochitests only test our internal abstraction.
Flags: needinfo?(dbolter)
Assignee

Comment 6

4 years ago
(In reply to David Bolter [:davidb] from comment #5)
> (In reply to Yura Zenevich [:yzen] from comment #4)
> > So I guess the context of this bug has little to do with creating/running
> > tests that replicate user actions (either within chrome or content).
> 
> As I understand it we need a way to exercise and test the desktop layer of
> our e10s code. I was hoping Marionette might be a way? Mochitests only test
> our internal abstraction.

Yes so marionette tests is a descent way, and the only one at the moment that I can think of, that can test integration between e10s a11y desktop and an AT user. This is how we test if our Firefox OS a11y works for our own screen reader. Unlike other market a11y testing frameworks, we can reliably say if something is accessible to an AT user.
(In reply to Yura Zenevich [:yzen] from comment #4)
> So I guess the context of this bug has little to do with creating/running
> tests that replicate user actions (either within chrome or content). That is
> what Marionette is usually used for. Through, David and Trevor, you
> mentioned some failing Marionette tests, could you give some more context or
> point me to that?

not really I don't remember the details.  However I have seen a11y patches break marrionette tests on treeherder.  I have no idea what those tests do, but there's the intermitant orange bug involving a11y bug 1114895, and some other tests you can find with find -name *accessibility*.py


(In reply to Yura Zenevich [:yzen] from comment #6)
> (In reply to David Bolter [:davidb] from comment #5)
> > (In reply to Yura Zenevich [:yzen] from comment #4)
> > > So I guess the context of this bug has little to do with creating/running
> > > tests that replicate user actions (either within chrome or content).
> > 
> > As I understand it we need a way to exercise and test the desktop layer of
> > our e10s code. I was hoping Marionette might be a way? Mochitests only test
> > our internal abstraction.
> 
> Yes so marionette tests is a descent way, and the only one at the moment
> that I can think of, that can test integration between e10s a11y desktop and
> an AT user. This is how we test if our Firefox OS a11y works for our own
> screen reader. Unlike other market a11y testing frameworks, we can reliably
> say if something is accessible to an AT user.

I guess the tests are platform specific? does it even work for mac / windows?
Assignee

Comment 8

4 years ago
Yes I think the those are marionette python tests for Gaia (Gip). Afaik they are ran for b2g desktop builds.

AFAIK you can run them on all platforms:
https://developer.mozilla.org/en-US/docs/Marionette_Test_Runner
or you can run them with ./mach marionette-test ...
Assignee

Updated

4 years ago
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Assignee

Comment 9

4 years ago
Posted patch sample 1186029 test (obsolete) — Splinter Review
Attachment #8708496 - Flags: feedback?(tbsaunde+mozbugs)
Assignee

Comment 10

4 years ago
Posted patch 1186029.2.patch (obsolete) — Splinter Review
Quick sanity check that I should expect this to pass.
Attachment #8710482 - Flags: feedback?(tbsaunde+mozbugs)
Assignee

Comment 11

4 years ago
The part that is failing in that test is the last check for accessible being retrieved.
Assignee

Updated

4 years ago
Attachment #8708496 - Attachment is obsolete: true
Attachment #8708496 - Flags: feedback?(tbsaunde+mozbugs)
Comment on attachment 8710482 [details] [diff] [review]
1186029.2.patch

I think the previous patch was closer to on the right path.

I don't see any point in messing with browser-chrome tests, and I don't really understand that js, but it seems like   you are poking at accessibles in the same process as the content they are for which isn't really useful.
Attachment #8710482 - Flags: feedback?(tbsaunde+mozbugs)
Assignee

Comment 13

3 years ago
Posted patch 1186029 patch v3 (obsolete) — Splinter Review
Note most of the supporting common files are very close to (inspered by) to the ones in mochitest folder. See the actual tests to get an idea how events are queued.
Attachment #8710482 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8718551 - Flags: feedback?(tbsaunde+mozbugs)
Comment on attachment 8718551 [details] [diff] [review]
1186029 patch v3

I'm not familiar enough with generators and stuff to be able to understand this code.  It seems to me writing this in the style of the other tests would make it a lot more understandable and easier to see it is correct.

on the substance of the issue these tests don't seem to run a content process which is the reason they work, but means they aren't testing something useful.  Unfortunately because of the above I have no idea why that is.

I think I'll try and port a test or two next week to try and better understand why this is hard.
Attachment #8718551 - Flags: feedback?(tbsaunde+mozbugs) → feedback-
Assignee

Comment 15

3 years ago
Have you tried running then with --e10s flag?
(In reply to Yura Zenevich [:yzen] from comment #15)
> Have you tried running then with --e10s flag?

I just tried that with the same result.
Assignee

Comment 17

3 years ago
Mike, if you have a sec, perhaps you would know why the tests (any) don't run with content in content process even with the --e10s flag?
Flags: needinfo?(mconley)
Assignee

Comment 18

3 years ago
Not sure if I'm reading the logs correctly, but when entering a test:

2 INFO TEST-START | accessible/tests/browser/browser_events_aria_alert.js
3 INFO Entering test 
++DOMWINDOW == 17 (0x136036800) [pid = 65974] [serial = 17] [outer = 0x11bf32c00]

PID is 65974

But the tab seems to be open in process with id 65975

--DOMWINDOW == 3 (0x125f42400) [pid = 65975] [serial = 3] [outer = 0x0] [url = data:text/html,%20%20%20%20<html>%20%20%20%20%20%20<head>%20%20%20%20%20%20%20%20<meta%20charset="utf-8"/>%20%20%20%20%20%20%20%20<title>ARIA%20alert%20event%20testing</title>%20%20%20%20%20%20</head>%20%20%20%20%20%20<body></body>%20%20%20%20</html>]
(In reply to Yura Zenevich [:yzen] from comment #18)
> Not sure if I'm reading the logs correctly, but when entering a test:
> 
> 2 INFO TEST-START | accessible/tests/browser/browser_events_aria_alert.js
> 3 INFO Entering test 
> ++DOMWINDOW == 17 (0x136036800) [pid = 65974] [serial = 17] [outer =
> 0x11bf32c00]
> 
> PID is 65974
> 
> But the tab seems to be open in process with id 65975
> 
> --DOMWINDOW == 3 (0x125f42400) [pid = 65975] [serial = 3] [outer = 0x0] [url
> =
> data:text/html,
> %20%20%20%20<html>%20%20%20%20%20%20<head>%20%20%20%20%20%20%20%20<meta%20cha
> rset="utf-8"/>%20%20%20%20%20%20%20%20<title>ARIA%20alert%20event%20testing</
> title>%20%20%20%20%20%20</head>%20%20%20%20%20%20<body></body>%20%20%20%20</
> html>]

hmm yeah I was measuring --e10s incorrectly, I guess I'll need to investigate why this works next week.
yura and I looked at this IRL - he'll need to check Services.appinfo.browserTabsRemoteAutostart to make sure that remote tabs are enabled, and then browser.isRemoteBrowser to ensure that the browsers that are being passed to the generator function are actually remote.

If both of those things are true, and it still looks like the content is being loaded in the parent, then that's pretty interesting, and I'll happily help investigate.
Flags: needinfo?(mconley)
Assignee

Comment 21

3 years ago
Posted patch 1186029 patch v4 (obsolete) — Splinter Review
OK, thanks to Mike, I fixed the issue. chrome:// urls of the test docs were indeed loaded in parent, I fixed it so it loads it with example.com prefix and now state related tests fail as expected \o/.
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Yura Zenevich [:yzen] from comment #21)
> Created attachment 8720367 [details] [diff] [review]
> 1186029 patch v4
> 
> OK, thanks to Mike, I fixed the issue. chrome:// urls of the test docs were
> indeed loaded in parent, I fixed it so it loads it with example.com prefix
> and now state related tests fail as expected \o/.

ok, its good to have some idea what's going on there I guess.  However it seems to me like we'd be far better served by just writing tests in xul files and using <browser remote="true">.  I really should have known it was that simple to do what we need earlier.
Flags: needinfo?(tbsaunde+mozbugs)
Assignee

Comment 23

3 years ago
Posted patch 1186029 failing test (obsolete) — Splinter Review
failing test is:
accessible/tests/browser/browser_events_aria_menu.js
to see the failure run:
./mach mochitest accessible/tests/browser/browser_events_aria_menu.js --e10s
with no e10s flag the tests passes
Assignee

Comment 24

3 years ago
Attachment #8718551 - Attachment is obsolete: true
Attachment #8720367 - Attachment is obsolete: true
Attachment #8723012 - Attachment is obsolete: true
(In reply to Yura Zenevich [:yzen] from comment #25)
> Created attachment 8732025 [details] [diff] [review]
> 1186029 patch with e10s compatible treeupdate, name caching and events tests.

while I'm not JS guru, but I find these tests cool and quite readable, yields are fancy.
Assignee

Comment 27

3 years ago
Posted patch 1186029 patch (obsolete) — Splinter Review
Attachment #8732025 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8735888 - Flags: feedback?(mconley)
Comment on attachment 8735888 [details] [diff] [review]
1186029 patch

Review of attachment 8735888 [details] [diff] [review]:
-----------------------------------------------------------------

I've skimmed some of these tests, and it looks good to me. You're doing the right things - using ContentTask, yielding Promises, etc.

What I would recommend is that every test get a docstring describing what exactly is being tested. Even more importantly, every utility in head.js and in your other utility scripts (like events.js) should get a docstring describing what they do, what arguments they expect, what they return, etc.

::: accessible/tests/browser/browser.ini
@@ +8,5 @@
> +  doc_treeupdate_removal.xhtml
> +  doc_treeupdate_visibility.html
> +
> +[browser_events.js]
> +skip-if = e10s

So I guess the plan is to make these pass for non-e10s, and then work on enabling each one for e10s?

::: accessible/tests/mochitest/common.js
@@ +764,4 @@
>        msg += ", role: " + roleToString(acc.role);
>        if (acc.name)
>          msg += ", name: '" + shortenString(acc.name) + "'";
> +    } catch (e) {}

What could we possibly be catching here and ignoring? Is it possible that it's useful information that we want to put onto msg?
Attachment #8735888 - Flags: feedback?(mconley) → feedback+
Assignee

Comment 29

3 years ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #28)
> Comment on attachment 8735888 [details] [diff] [review]
> 1186029 patch
> 
> Review of attachment 8735888 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: accessible/tests/browser/browser.ini
> @@ +8,5 @@
> > +  doc_treeupdate_removal.xhtml
> > +  doc_treeupdate_visibility.html
> > +
> > +[browser_events.js]
> > +skip-if = e10s
> 
> So I guess the plan is to make these pass for non-e10s, and then work on
> enabling each one for e10s?

That's the plan yes.
Assignee

Comment 30

3 years ago
Comment on attachment 8735888 [details] [diff] [review]
1186029 patch

Review of attachment 8735888 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/common.js
@@ +764,4 @@
>        msg += ", role: " + roleToString(acc.role);
>        if (acc.name)
>          msg += ", name: '" + shortenString(acc.name) + "'";
> +    } catch (e) {}

Here the meaningful failure would've already happened in the catch above.
Assignee

Updated

3 years ago
Depends on: 1259023
Assignee

Comment 31

3 years ago
---
 accessible/moz.build                               |   2 +
 accessible/tests/browser/browser.ini               |  49 ++
 accessible/tests/browser/browser_caching_name.js   | 434 ++++++++++++
 .../tests/browser/browser_events_caretmove.js      |  22 +
 accessible/tests/browser/browser_events_hide.js    |  29 +
 accessible/tests/browser/browser_events_show.js    |  22 +
 .../tests/browser/browser_events_statechange.js    |  59 ++
 .../tests/browser/browser_events_textchange.js     |  64 ++
 .../tests/browser/browser_treeupdate_ariadialog.js |  41 ++
 .../tests/browser/browser_treeupdate_ariaowns.js   | 324 +++++++++
 .../tests/browser/browser_treeupdate_canvas.js     |  33 +
 .../browser/browser_treeupdate_cssoverflow.js      |  60 ++
 accessible/tests/browser/browser_treeupdate_doc.js | 273 ++++++++
 .../tests/browser/browser_treeupdate_gencontent.js |  68 ++
 .../tests/browser/browser_treeupdate_hidden.js     |  39 ++
 .../tests/browser/browser_treeupdate_imagemap.js   | 175 +++++
 .../tests/browser/browser_treeupdate_list.js       |  46 ++
 .../browser/browser_treeupdate_list_editabledoc.js |  37 +
 .../tests/browser/browser_treeupdate_listener.js   |  46 ++
 .../tests/browser/browser_treeupdate_optgroup.js   |  94 +++
 .../tests/browser/browser_treeupdate_removal.js    |  41 ++
 .../tests/browser/browser_treeupdate_table.js      |  43 ++
 .../tests/browser/browser_treeupdate_textleaf.js   |  38 ++
 .../tests/browser/browser_treeupdate_visibility.js | 196 ++++++
 .../tests/browser/browser_treeupdate_whitespace.js |  85 +++
 .../tests/browser/doc_treeupdate_ariadialog.html   |  23 +
 .../tests/browser/doc_treeupdate_ariaowns.html     |  44 ++
 .../tests/browser/doc_treeupdate_imagemap.html     |  21 +
 .../tests/browser/doc_treeupdate_removal.xhtml     |  11 +
 .../tests/browser/doc_treeupdate_visibility.html   |  78 +++
 accessible/tests/browser/events.js                 | 742 +++++++++++++++++++++
 accessible/tests/browser/head.js                   | 226 +++++++
 accessible/tests/mochitest/common.js               |   8 +-
 33 files changed, 3470 insertions(+), 3 deletions(-)
 create mode 100644 accessible/tests/browser/browser.ini
 create mode 100644 accessible/tests/browser/browser_caching_name.js
 create mode 100644 accessible/tests/browser/browser_events_caretmove.js
 create mode 100644 accessible/tests/browser/browser_events_hide.js
 create mode 100644 accessible/tests/browser/browser_events_show.js
 create mode 100644 accessible/tests/browser/browser_events_statechange.js
 create mode 100644 accessible/tests/browser/browser_events_textchange.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_ariadialog.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_ariaowns.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_canvas.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_cssoverflow.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_doc.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_gencontent.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_hidden.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_imagemap.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_list.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_list_editabledoc.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_listener.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_optgroup.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_removal.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_table.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_textleaf.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_visibility.js
 create mode 100644 accessible/tests/browser/browser_treeupdate_whitespace.js
 create mode 100644 accessible/tests/browser/doc_treeupdate_ariadialog.html
 create mode 100644 accessible/tests/browser/doc_treeupdate_ariaowns.html
 create mode 100644 accessible/tests/browser/doc_treeupdate_imagemap.html
 create mode 100644 accessible/tests/browser/doc_treeupdate_removal.xhtml
 create mode 100644 accessible/tests/browser/doc_treeupdate_visibility.html
 create mode 100644 accessible/tests/browser/events.js
 create mode 100644 accessible/tests/browser/head.js

Review commit: https://reviewboard.mozilla.org/r/44663/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44663/
Attachment #8738716 - Flags: review?(eitan)
Attachment #8738716 - Flags: review?(dbolter)
Assignee

Comment 32

3 years ago
Note in order to run tests with e10s successfully, you need patches from bug 1259023 applied.
Assignee

Comment 33

3 years ago
There are 3 categories of tests:
* e10s tests that test accessible events that have non-generic event interfaces within ipc.
* e10s tests that test caching (right now) of accessible properties. This patch only has these for name for now.
* e10s tests that test tree updates after various DOM manipulations.

In order to run tests with e10s run:
mach mochitest accessible/tests/browser --e10s 
(if you want to run tests without - just remove the --e10s flag)
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

Thanks! Super good to get coverage even if sophisticated for my rusty brain.
Attachment #8738716 - Flags: review?(dbolter) → feedback+
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

https://reviewboard.mozilla.org/r/44663/#review41593

I didn't actually dive into the tests yet. Mostly, because I think the common files need some reworking first. Please address these issues before a more extensive review of the actual tests.

::: accessible/tests/browser/events.js:175
(Diff revision 1)
> + * not present in parent process but, if available, DOMNode id is attached to an
> + * accessible object.
> + * @param  {nsIAccessibleEvent} aEvent accessible event
> + * @return {String?}                   DOMNode id if available
> + */
> +function getEventDOMNodeID(aEvent) {

Do we really need these functions for both events and accessible arguments? Can't we just have getAccessibleDOMNodeID()? Won't aEvent.accessible always be non-null regardless of e10s?

::: accessible/tests/browser/events.js:187
(Diff revision 1)
> +  }
> +  try {
> +    return aEvent.DOMNode.id;
> +  } catch (e) { /* This will fail if DOMNode is in different process. */ }
> +  try {
> +    return acc.id;

Fascinating, so if there is no DOMNode, nsIAccessible now has an id property?

::: accessible/tests/browser/events.js:233
(Diff revision 1)
> +/**
> + * Base event checker class that registers an event listener and creates an
> + * internal promise that is resovled when correct event is received and all
> + * event checks pass.
> + */
> +class EventChecker {

Classy!

But seriously, I think these classes are overkill. Promises and generators allow us to keep things simple and function based and not get wound up in the patterns used in the a11y event mochitest.

These classes have two overlapping methods match and handeEvent.

**match()**

From what I understand, the purpose of match() is to match a single expected event. 

Usually element ID and event type is all you want to match. There are exceptions like state change events, where you want to match a specific state change.

But generally, it should be straightforward, either the event happened, or it didn't. Kind of like a DOM click event. "Did we get a click on a specific target?"

Match methods should be minimal, so that failures are not unexplained timeouts.

**handleEvent()**

Once a given event is matched, there are a silo of assertions you want to do on that event. From what I can see here, most of the implementations are simply mirroring checks that match already did. I'll comment inline where that is the case.

Generally, I think this method should go away. So really, you just want match functions and not checker classes. This is what I think an example usage should be:

yield event = waitForEvent("button", EVENT_FOCUS);
ok(hasState(event.accessible, STATE_FOCUSED);

or, for example, a specialized wait function for state events:
yield event = waitForStateChangedEvent("checkbutton", STATE_CHECKED);

::: accessible/tests/browser/events.js:271
(Diff revision 1)
> +   * Default event handler that performs some common nsIAccessible event
> +   * interface tests (type, document, DOMNode id if available).
> +   * @param  {nsIAccessibleEvent} aEvent event to handle
> +   */
> +  handleEvent(aEvent) {
> +    is(aEvent.eventType, this.eventType,

If it is the wrong event type, there is a bug in the test code, not the content. Anyway, this would belong in the match() function.

::: accessible/tests/browser/events.js:277
(Diff revision 1)
> +      `Wrong event type. Expected: ${eventTypeToString(this.eventType)}, got ${eventTypeToString(aEvent.eventType)}`);
> +    let id = getEventDOMNodeID(aEvent);
> +    ok(aEvent.accessibleDocument instanceof nsIAccessibleDocument,
> +      'No accessible document present.');
> +    if (id) {
> +      is(id, this.id, `Wrong event DOMNode id. Expected: ${this.id}, got ${id}`);

You already to this in match()

::: accessible/tests/browser/events.js:294
(Diff revision 1)
> +   *                                           the event
> +   * @param  {Number}             aEventType   expected accessible event type
> +   * @param  {Function|Function*} aHandleEvent additional event handler function
> +   *                                           or generator
> +   */
> +  constructor(aID, aEventType, aHandleEvent) {

This class seems a bit redundant to me. Why not just have an optional handler arg in EventChecker?

But like I said above, I think all these classes are redundant.

::: accessible/tests/browser/events.js:340
(Diff revision 1)
> +   */
> +  match(aEvent) {
> +    if (aEvent instanceof nsIAccessibleStateChangeEvent) {
> +      let scEvent = aEvent.QueryInterface(nsIAccessibleStateChangeEvent);
> +      return getEventDOMNodeID(aEvent) === this.id &&
> +        scEvent.state === this.state;

You should also check isExtraState here, no?

::: accessible/tests/browser/events.js:353
(Diff revision 1)
> +   * @param  {nsIAccessibleEvent} aEvent event to match
> +   */
> +  handleEvent(aEvent) {
> +    let event = null;
> +    try {
> +      event = aEvent.QueryInterface(nsIAccessibleStateChangeEvent);

You already checked with instanceof, and queried the interface in the match() function.

::: accessible/tests/browser/events.js:360
(Diff revision 1)
> +      ok(false, `State change event was expected: ${e}`);
> +    }
> +
> +    if (!event) { return; }
> +
> +    is(event.state, this.state, 'Wrong state of the statechange event.');

You checked this in match().

::: accessible/tests/browser/events.js:361
(Diff revision 1)
> +    }
> +
> +    if (!event) { return; }
> +
> +    is(event.state, this.state, 'Wrong state of the statechange event.');
> +    is(event.isExtraState, this.isExtraState,

You should check this only in match().

::: accessible/tests/browser/events.js:414
(Diff revision 1)
> +      ok(false, `Hide event was expected: ${e}`);
> +    }
> +
> +    if (!event) { return; }
> +
> +    is(getAccessibleDOMNodeID(event.targetParent), this.parentID,

Like I said in the long comment above, I think checks like this should just be a top-level function.

testHiddenEvent(aParentID, aPreviousID, aNextID)

::: accessible/tests/browser/events.js:453
(Diff revision 1)
> +      event = aEvent.QueryInterface(nsIAccessibleCaretMoveEvent);
> +    } catch (e) {
> +      ok(false, `Caret move event was expected: ${e}`);
> +    }
> +
> +    if (!event) { return; }

You just silently give up?

::: accessible/tests/browser/events.js:501
(Diff revision 1)
> +      ok(false, `Text change event was expected: ${e}`);
> +    }
> +
> +    if (!event) { return; }
> +
> +    is(event.start, this.start,

same comment as hide events.

::: accessible/tests/browser/events.js:526
(Diff revision 1)
> +   * Editable state change handler that checks its state, extra state and state
> +   * value.
> +   * @param  {nsIAccessibleEvent} aEvent event to match
> +   */
> +  *handleEvent(aEvent) {
> +    yield super.handleEvent(aEvent);

Same comment as above about hide events. And generally, handleEvent being a generator is strange.

::: accessible/tests/browser/events.js:553
(Diff revision 1)
> +/**
> + * Accessible event observer that is used to by event checkers to used event
> + * handlers with.
> + * NOTE: Taken from accessible/tests/mochitest/events.js
> + */
> +let gA11yEventObserver = {

If you really want to put classes to good use, they would work well here.

* Encapsulate the logging with the observer.
* Abolish the global variables and make them members of this class (gA11yEventListeners, etc.)
* Make register, unregister member methods.
* Etc.

::: accessible/tests/browser/events.js:653
(Diff revision 1)
> +
> +/**
> + * Start or stop listening for accessible events.
> + * NOTE: Taken from accessible/tests/mochitest/events.js
> + */
> +function listenA11yEvents(aStartToListen){

Not sure what the purpose of this function is. Can't this be folded into addA11yEventListener/removeA11yEventListener?

If the listener count goes from 0 to 1, add observer, if it drops down to 0, remove observer.

::: accessible/tests/browser/events.js:682
(Diff revision 1)
> + */
> +function registerA11yEventListener(aEventType, aEventHandler) {
> +  return new Promise((resolve, reject) => {
> +    listenA11yEvents(true);
> +    let eventHandler = {
> +      handleEvent(aEvent) {

Why does this need to be an object with single field of handleEvent? Why not just have it be a function directly?

::: accessible/tests/browser/events.js:689
(Diff revision 1)
> +          return;
> +        }
> +        Task.spawn(function*() {
> +          unregisterA11yEventListener(aEventType, eventHandler);
> +          yield aEventHandler.handleEvent(aEvent);
> +          resolve();

As mentioned above, I would change aEventHandler to simply a flat aMatchFunc that returns true or false. Or! Better yet! Have the match func return event object, or null. Then it could return an event object queried to the relevant interface.

If it returns true, resolve(event).

::: accessible/tests/browser/events.js:704
(Diff revision 1)
> + * event listener (see registerA11yEventListener() function) when the listener
> + * is not needed.
> + */
> +function unregisterA11yEventListener(aEventType, aEventHandler) {
> +  removeA11yEventListener(aEventType, aEventHandler);
> +  listenA11yEvents(false);

So listenA11yEvents(true) is called outside of registerA11yEventListener(), but then inside of unregisterA11yEventListener()? The is more confusion. I think listenA11yEvents should be abolished, like I said.

In general, I think this whole thing could be flattened out to two functions: register/unregister. add/remove could be folded in to those functions.

::: accessible/tests/browser/events.js:717
(Diff revision 1)
> + * @return {Promise}             promise that resolves to an event that was
> + *                               matched and handled
> + */
> +function waitForEvent(aEventType, aID) {
> +  let event;
> +  let checker = new ExtendableEventChecker(aID, aEventType, aEvent => event = aEvent);

Why not just have the default handler resolve to the event? Then you don't need ExtendableEventChecker, and you don't need a custom handler either.

::: accessible/tests/browser/events.js:731
(Diff revision 1)
> + * @param  {String}             aMarkup   HTML markup string or a document to
> + *                                        test content with
> + * @param  {Function|Function*} aTask     a generator or a function with tests
> + *                                        to run
> + */
> +function addAccessibleOnloadTask(aMarkup, aTask) {

How is the purpose of this function different from accessibleTestTask that uses waitForA11yLoad?

Seems like there are a lot of potential test entry points.

::: accessible/tests/browser/head.js:38
(Diff revision 1)
> +  'http://example.com/browser/accessible/tests/browser/';
> +
> +/**
> + * Current browser element in test context that the content belongs to.
> + */
> +let currentBrowser;

This global makes me nervous. I would prefer if the current browser was a variable in the scope of the test function.

::: accessible/tests/browser/head.js:55
(Diff revision 1)
> +/**
> + * Send a character to content (works both with and without e10s).
> + * @param  {String}   aChar a sequence of characters to be sent to content
> + * @return {Promise}  promise indicating when all characters are sent
> + */
> +function sendChar(aChar) {

If currentBrowser was not global, this (and other) function would not be useful anymore.

::: accessible/tests/browser/head.js:83
(Diff revision 1)
> +function waitForA11yLoad(browser) {
> +  return ContentTask.spawn(browser, null, function*() {
> +    yield new Promise(resolve => {
> +      // Poll document's state until the test times out if necessary.
> +      let intervalId = setInterval(() => {
> +        let accDoc = getAccessible(document);

This could be outside of the interval callback, or at least not re-called when we already have a doc acc.

::: accessible/tests/browser/head.js:90
(Diff revision 1)
> +        accDoc.getState(state, {});
> +        if (!(state.value & STATE_BUSY)) {
> +          clearInterval(intervalId);
> +          resolve();
> +        }
> +      }, 0);

polling with 0 interval?  I guess this is an quivalent of an idle callback. But that seems kind of intense.

Another way to do it is observe an acc event state change, no?

*update:* I just reviewed events.js and noticed that there is another function that does just that. Wondering why we need both functions.

::: accessible/tests/browser/head.js:165
(Diff revision 1)
> +  return BrowserTestUtils.withNewTab({
> +    gBrowser,
> +    url: url,
> +  }, function*(browser) {
> +    yield SimpleTest.promiseFocus(browser);
> +    currentBrowser = browser;

Like I said earlier, this makes me nervous.

::: accessible/tests/browser/head.js:172
(Diff revision 1)
> +      'let { document, window, navigator } = content;',
> +      { name: 'common.js', dir: MOCHITESTS_DIR });
> +    yield waitForA11yLoad(browser);
> +    info(`e10s enabled: ${Services.appinfo.browserTabsRemoteAutostart}`);
> +    info(`Actually remote browser: ${browser.isRemoteBrowser}`);
> +    yield aTask(browser);

If aTask were a generator, wouldn't you need to do yield* ?

::: accessible/tests/mochitest/common.js:768
(Diff revision 1)
> +    }
> +    try {
>        msg += ", role: " + roleToString(acc.role);
>        if (acc.name)
>          msg += ", name: '" + shortenString(acc.name) + "'";
> -    } catch (e) {
> +    } catch (e) {}

When would these operations fail? Shouldn't we add that to the message?
Attachment #8738716 - Flags: review?(eitan)
Assignee

Comment 36

3 years ago
Thanks for the review, I will address the comments. The main reason I did that object'y stuff and wrapping of things like browser etc was to make things closely resemble existing mochitests and be less confusing with yields and generators. I guess there isn't much sense to that if we can improve things by what you suggested.
Assignee

Comment 37

3 years ago
https://reviewboard.mozilla.org/r/44663/#review41593

> Fascinating, so if there is no DOMNode, nsIAccessible now has an id property?

yes

> If aTask were a generator, wouldn't you need to do yield* ?

with browser chrome tests it works (i.e. the test yields until there's no next). it also seems like it's the style in all other tests.
Assignee

Comment 38

3 years ago
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44663/diff/1-2/
Attachment #8738716 - Attachment description: MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. → MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay
Attachment #8738716 - Flags: feedback+ → review?(eitan)
Assignee

Comment 39

3 years ago
OK addressed comments. Apologies if I missed something that you wanted me to fix.
> But seriously, I think these classes are overkill. Promises and generators
> allow us to keep things simple and function based and not get wound up in
> the patterns used in the a11y event mochitest.

So, I've said it before, and I haven't understood this patch well enough to look at this part of it, but personally I really prefer the mochitest-a11y style of using an explicit queue of expected events  Hiding thunks inline with syntactic sugar is awful on its own.  More importantly  though these tests are conceptually a state machine where event handlers perform the state transitions, so imho it makes sense to have explicit global state for what state the machine is in, and a bunch of separate functions to transition between states.  That said I'm probably not going to spend more time arguing about this.
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

https://reviewboard.mozilla.org/r/44663/#review42359

Almost great!

The three things I see in all the tests that I don't like:

1. Arrays of expected trees, and then accessing it via index. This makes it all hard to read. Either make them global variables with human readable names (eg. treeAfterHide), or inline them in the test function when they are small enough.
2. Its nice that you broke stuff up into smaller functions, but each function should be independant of the state of the previous function. For example one function shouldn't rely on a node removed in another function.
3. You pass accessibles to these functions as arguments but then you hard-code the ids of those accessibles in the functions.

::: accessible/tests/browser/browser.ini:16
(Diff revision 2)
> +  !/accessible/tests/mochitest/letters.gif
> +  !/accessible/tests/mochitest/moz.png
> +
> +# Caching tests
> +[browser_caching_name.js]
> +skip-if = e10s

Why all these skips on e10s? Are these just TODOs?

::: accessible/tests/browser/browser_caching_name.js:351
(Diff revision 2)
> +  testName(target.acc, expected);
> +  let onEvent;
> +  if (rule.recreated) {
> +    onEvent = waitForEvent(EVENT_REORDER, target.parent);
> +  } else if (rule.textchanged) {
> +    onEvent = waitForEvent(EVENT_TEXT_INSERTED, target.id);

What if the rule doesn't have `recreated` or `textchanged`? I would expect you would still need to wait for a certain event to know for certain that the name change took affect.

::: accessible/tests/browser/browser_caching_name.js:376
(Diff revision 2)
> +function* testElmRule(browser, target, rule, expected) {
> +  testName(target.acc, expected);
> +  let onEvent;
> +  if (['caption', 'img'].includes(rule.elm)) {
> +    onEvent = waitForEvent(EVENT_REORDER, target.id);
> +  } else if (['label', 'style'].includes(rule.elm)) {

I think choosing target.id or target.parent as event source by element type is misleading. It is because `label`/`style` are siblings of our test target, and `caption`/`image` are children of the target.

I don't have a great solution, just that this doesn't seem right.

::: accessible/tests/browser/browser_events_hide.js:11
(Diff revision 2)
> +          invokeSetStyle, nsIAccessibleEvent, is, Ci, getAccessibleDOMNodeID */
> +
> +'use strict';
> +
> +loadScripts({ name: 'role.js', dir: MOCHITESTS_DIR },
> +            { name: 'states.js', dir: MOCHITESTS_DIR });

Here, and elsewhere. It looks like you are using events.js. Why roles.js and states.js in this file?

::: accessible/tests/browser/browser_events_statechange.js:15
(Diff revision 2)
> +'use strict';
> +
> +loadScripts({ name: 'role.js', dir: MOCHITESTS_DIR },
> +            { name: 'states.js', dir: MOCHITESTS_DIR });
> +
> +const EVENT_STATE_CHANGE = nsIAccessibleEvent.EVENT_STATE_CHANGE;

Why dorns't this live in events.js?

::: accessible/tests/browser/browser_events_statechange.js:31
(Diff revision 2)
> +  // Set design mode on.
> +  yield ContentTask.spawn(browser, {}, () =>
> +    content.document.getElementById('iframe').contentDocument.designMode = 'on');
> +  let event = yield onStateChange;
> +
> +  checkStateChangeEvent(event.QueryInterface(nsIAccessibleStateChangeEvent),

Why not query interface in the check event function?

::: accessible/tests/browser/browser_events_statechange.js:45
(Diff revision 2)
> +    content.document.getElementById('checkbox').checked = true);
> +  let event = yield onStateChange;
> +
> +  checkStateChangeEvent(event.QueryInterface(nsIAccessibleStateChangeEvent),
> +    STATE_CHECKED, false, true);
> +  testStates(event.accessible, STATE_CHECKED, 0);

Looks like you could actually checked for the checked state with a fourth argument of `true`.

::: accessible/tests/browser/browser_events_statechange.js:69
(Diff revision 2)
> + */
> +addAccessibleTask(`
> +  <iframe id="iframe" src="${iframeSrc}"></iframe>
> +  <input id="checkbox" type="checkbox" />`, function*(browser) {
> +  // Test state change
> +  yield changeCheckInput(browser);

This is a lot of levels of generators. And the generator functions are separated from the markup. I would just put the tests directly here for readability's sake.

::: accessible/tests/browser/browser_events_textchange.js:27
(Diff revision 2)
> +}
> +
> +function* changeText(browser, id, value, events) {
> +  // Aggregate all event handlers into one promise that resolves when all events
> +  // are checked.
> +  let checked = Promise.all(events.map(({ isInserted, str, offset }) => {

Promise.all() does not assure the right order.

::: accessible/tests/browser/browser_events_textchange.js:31
(Diff revision 2)
> +  // are checked.
> +  let checked = Promise.all(events.map(({ isInserted, str, offset }) => {
> +    let onTextChange = waitForEvent(
> +      isInserted ? EVENT_TEXT_INSERTED : EVENT_TEXT_REMOVED, id);
> +    return onTextChange.then(event =>
> +      checkTextChangeEvent(event.QueryInterface(nsIAccessibleTextChangeEvent),

can't you just have the interface query in `checkTextChangeEvent`?

::: accessible/tests/browser/browser_events_textchange.js:36
(Diff revision 2)
> +      checkTextChangeEvent(event.QueryInterface(nsIAccessibleTextChangeEvent),
> +        id, str, offset, offset + str.length, isInserted, false));
> +  }));
> +  // Change text in the subtree.
> +  yield ContentTask.spawn(browser, { id, value }, ({ id, value }) =>
> +    content.document.getElementById(id).firstChild.data = value);

This doesn't seem safe. `data` is only valid in a text node. You can use textContent for better results.

::: accessible/tests/browser/browser_treeupdate_ariadialog.js:49
(Diff revision 2)
> +    // Make dialog visible and update its inner content.
> +    let onShow = waitForEvent(EVENT_SHOW, 'dialog');
> +    yield ContentTask.spawn(browser, {}, () => {
> +      let doc = content.document;
> +      doc.getElementById('dialog').style.display = 'block';
> +      doc.getElementById('table').style.visibility = 'visible';

What specifically are you testing for when you have two set to containers to visible like this?

::: accessible/tests/browser/browser_treeupdate_ariadialog.js:50
(Diff revision 2)
> +    let onShow = waitForEvent(EVENT_SHOW, 'dialog');
> +    yield ContentTask.spawn(browser, {}, () => {
> +      let doc = content.document;
> +      doc.getElementById('dialog').style.display = 'block';
> +      doc.getElementById('table').style.visibility = 'visible';
> +      doc.getElementById('a').textContent = 'link';

What does this do? You don't look for this in the tree test.

::: accessible/tests/browser/browser_treeupdate_ariadialog.js:53
(Diff revision 2)
> +      doc.getElementById('dialog').style.display = 'block';
> +      doc.getElementById('table').style.visibility = 'visible';
> +      doc.getElementById('a').textContent = 'link';
> +      let input = doc.getElementById('input');
> +      input.value = 'hello';
> +      input.focus();

I get that this is best practice for displaying a dialog, but does this affect the test? Shouldn't you be looking at the state of the input?

::: accessible/tests/browser/browser_treeupdate_ariaowns.js:26
(Diff revision 2)
> +      { PUSHBUTTON: [ ] }
> +    ]
> +  };
> +  testAccessibleTree(accessible, tree);
> +
> +  let onReorder = waitForEvent(EVENT_REORDER, 't1_container');

It doesn't seem right that these functions accept accessibles as arguments, but then have the the id of the accessibles hardcoded here.

I would pass the doc accessible, and get the accessible instance in the functions.

::: accessible/tests/browser/browser_treeupdate_ariaowns.js:45
(Diff revision 2)
> +  testAccessibleTree(accessible, tree);
> +}
> +
> +function* removeARIAOwns(browser, accessible) {
> +  let onReorder = waitForEvent(EVENT_REORDER, 't1_container');
> +  yield invokeSetAttribute(browser, 't1_container', 'aria-owns');

Don't you want to test the tree before removing it too?

*update:* I see that you do that in `changeARIAOwns` because you are using the same container. I would merge these two functions because one depends on the state of the prior one.

::: accessible/tests/browser/browser_treeupdate_ariaowns.js:61
(Diff revision 2)
> +  };
> +  testAccessibleTree(accessible, tree);
> +}
> +
> +function* setARIAOwns(browser, accessible) {
> +  let onReorder = waitForEvent(EVENT_REORDER, 't1_container');

same as above, I think these need to be smooshed together into a giant function. With plenty of newlines and heading comments..

::: accessible/tests/browser/browser_treeupdate_ariaowns.js:171
(Diff revision 2)
> +    ]
> +  };
> +  testAccessibleTree(accessible, tree);
> +}
> +
> +function* removeA11eteiner(browser, accessible) {

Remove what?

::: accessible/tests/browser/browser_treeupdate_ariaowns.js:199
(Diff revision 2)
> +  yield onReorder;
> +
> +  let tree = {
> +    SECTION: [ ]
> +  };
> +  testAccessibleTree(accessible1, tree);

another example of the accessible/id disconnect. `accessible1`? I needed to look up what it is elsewhere.

::: accessible/tests/browser/browser_treeupdate_ariaowns.js:210
(Diff revision 2)
> +  };
> +  testAccessibleTree(accessible2, tree);
> +}
> +
> +function* appendElToRecacheChildren(browser, accessible1, accessible2) {
> +  let onReorder = waitForEvent(EVENT_REORDER, 't3_container2');

Similar to the issues above. This function depends on the aria-owns "steal" from the previous test.

::: accessible/tests/browser/browser_treeupdate_canvas.js:15
(Diff revision 2)
> +
> +loadScripts({ name: 'role.js', dir: MOCHITESTS_DIR });
> +
> +const trees = [
> +  { CANVAS: [] },
> +  { DIALOG: [] }

Can't we just have two variables instead of an array? It would be more readable.

::: accessible/tests/browser/browser_treeupdate_cssoverflow.js:13
(Diff revision 2)
> +          waitForEvent, content, ContentTask, testAccessibleTree,
> +          EVENT_REORDER, findAccessibleChildByID, invokeSetStyle */
> +
> +loadScripts({ name: 'role.js', dir: MOCHITESTS_DIR });
> +
> +const trees = [

Same, don't know why this is an array.

::: accessible/tests/browser/browser_treeupdate_cssoverflow.js:55
(Diff revision 2)
> +  yield onReorder;
> +
> +  testAccessibleTree(accessible, trees[1]);
> +}
> +
> +function* changeScrollbarStyles(browser, accessible) {

I don't really understand this test. Either way, same problem as above. This test expects a certain state that was set in another function.

::: accessible/tests/browser/browser_treeupdate_doc.js:43
(Diff revision 2)
> +    }
> +  };
> +  testAccessibleTree(accessible, trees[treeName]);
> +}
> +
> +function* writeIFrameDoc(browser, accessible) {

Same problem as ariaowns, the argument is implicitly `inner-frame`. From here on out.

::: accessible/tests/browser/browser_treeupdate_gencontent.js:13
(Diff revision 2)
> +          content, ContentTask, testAccessibleTree, EVENT_REORDER,
> +          findAccessibleChildByID, invokeSetAttribute*/
> +
> +loadScripts({ name: 'role.js', dir: MOCHITESTS_DIR });
> +
> +const trees = [

Another obscure array..

::: accessible/tests/browser/browser_treeupdate_gencontent.js:19
(Diff revision 2)
> +  {
> +    SECTION: [ ] // container
> +  },
> +  {
> +    SECTION: [ { // container
> +      SECTION: [ { // inserted node

This isn't "inserted", this is `container2_child`, no?

::: accessible/tests/browser/browser_treeupdate_gencontent.js:35
(Diff revision 2)
> +      ] }
> +    ]
> +  },
> +  {
> +    SECTION: [ // container
> +      { SECTION: [ // inserted node

same

::: accessible/tests/browser/browser_treeupdate_hidden.js:13
(Diff revision 2)
> +          testAccessibleTree, EVENT_REORDER, findAccessibleChildByID,
> +          invokeSetAttribute */
> +
> +loadScripts({ name: 'role.js', dir: MOCHITESTS_DIR });
> +
> +const trees = [

Array. And in general, I think just passing literals inline in testAccessibleTree would be a lot more readable when it is a small tree.

::: accessible/tests/browser/browser_treeupdate_imagemap.js:102
(Diff revision 2)
> +    areaElm.setAttribute('shape', 'rect');
> +    mapNode.appendChild(areaElm);
> +  });
> +  yield onReorder;
> +
> +  testAccessibleTree(accessible, trees[2]);

This tree depends on the ops in the previous function.

::: accessible/tests/browser/browser_treeupdate_imagemap.js:113
(Diff revision 2)
> +    let mapNode = content.document.getElementById('map');
> +    mapNode.removeChild(mapNode.firstElementChild);
> +  });
> +  yield onReorder;
> +
> +  testAccessibleTree(accessible, trees[3]);

same

::: accessible/tests/browser/browser_treeupdate_imagemap.js:125
(Diff revision 2)
> +
> +  testAccessibleTree(event.accessible, trees[4]);
> +  return event.accessible;
> +}
> +
> +function* restoreNameOnMap(browser, accessible) {

This function should be merged with the one above (dependent state, etc.)

::: accessible/tests/browser/browser_treeupdate_imagemap.js:128
(Diff revision 2)
> +}
> +
> +function* restoreNameOnMap(browser, accessible) {
> +  let reorder = waitForEvent(EVENT_REORDER, 'container');
> +  yield invokeSetAttribute(browser, 'map', 'name', 'atoz_map');
> +  yield BrowserTestUtils.synthesizeMouse('#imgmap', 10, 10,

Interesting.. this needs to happen for a REORDER to happen?

::: accessible/tests/browser/browser_treeupdate_imagemap.js:146
(Diff revision 2)
> +  yield reorder;
> +
> +  testAccessibleTree(accessible, trees[6]);
> +}
> +
> +function* insertMap(browser, accessible) {

Same, this should be merged with the remove function.

::: accessible/tests/browser/browser_treeupdate_list_editabledoc.js:14
(Diff revision 2)
> +          EVENT_REORDER, findAccessibleChildByID, ROLE_LISTITEM, ROLE_LIST,
> +          invokeSetAttribute, ROLE_STATICTEXT */
> +
> +loadScripts({ name: 'role.js', dir: MOCHITESTS_DIR });
> +
> +const trees = [

array

::: accessible/tests/browser/browser_treeupdate_optgroup.js:13
(Diff revision 2)
> +          content, ContentTask, ok, testAccessibleTree, EVENT_REORDER,
> +          findAccessibleChildByID, isDefunct */
> +
> +loadScripts({ name: 'role.js', dir: MOCHITESTS_DIR });
> +
> +const trees = [

array

::: accessible/tests/browser/browser_treeupdate_table.js:13
(Diff revision 2)
> +          content, ContentTask, testAccessibleTree, EVENT_REORDER,
> +          findAccessibleChildByID */
> +
> +loadScripts({ name: 'role.js', dir: MOCHITESTS_DIR });
> +
> +const trees = [

array

::: accessible/tests/browser/browser_treeupdate_textleaf.js:23
(Diff revision 2)
> +  };
> +  testAccessibleTree(accessible, tree);
> +
> +  let onReorder = waitForEvent(EVENT_REORDER, id);
> +  yield ContentTask.spawn(browser, id, id =>
> +    content.document.getElementById(id).firstChild.data = '');

I said this somewhere else, I think firstChild.data is problematic.

::: accessible/tests/browser/browser_treeupdate_visibility.js:13
(Diff revision 2)
> +          waitForEvent, testAccessibleTree, ContentTask, EVENT_REORDER,
> +          findAccessibleChildByID, invokeSetStyle, content */
> +
> +loadScripts({ name: 'role.js', dir: MOCHITESTS_DIR });
> +
> +const trees = [

long and hard to read array.

::: accessible/tests/browser/browser_treeupdate_visibility.js:96
(Diff revision 2)
> +        { TEXT_LEAF: [] } ]},
> +      { SECTION: [ // child2
> +        { TEXT_LEAF: [] } ]} ]}
> +}];
> +
> +function* testTreeOnHide(browser, accessible, containerID, id, { before, after }) {

.. and here the function takes both an accessible and its ID. This makes a bit more sense than the pattern above, but I think it really should just take the doc accessible and an ID.

::: accessible/tests/browser/browser_treeupdate_whitespace.js:13
(Diff revision 2)
> +          content, ContentTask, testAccessibleTree, EVENT_REORDER,
> +          findAccessibleChildByID */
> +
> +loadScripts({ name: 'role.js', dir: MOCHITESTS_DIR });
> +
> +const trees = [

array

::: accessible/tests/browser/events.js:33
(Diff revision 2)
> + * @param  {String}  attr     attribute name
> + * @param  {String?} value    optional attribute value, if not present, remove
> + *                            attribute
> + * @return {Promise}          promise indicating that attribute is set/removed
> + */
> +function invokeSetAttribute(browser, id, attr, value) {

It seems like these invoke funcitions could attribute/style/focus would be more appropriate in head.js, no?

::: accessible/tests/browser/events.js:92
(Diff revision 2)
> + * present in parent process but, if available, DOMNode id is attached to an
> + * accessible object.
> + * @param  {nsIAccessible} accessible  accessible
> + * @return {String?}                   DOMNode id if available
> + */
> +function getAccessibleDOMNodeID(accessible) {

Shouldn't this also be somewhere besides events.js?

::: accessible/tests/browser/events.js:117
(Diff revision 2)
> + * looks for an accessible that matches based on its DOMNode id.
> + * @param  {nsIAccessible}  accessible root accessible
> + * @param  {String}         id         id to look up accessible for
> + * @return {nsIAccessible?}            found accessible if any
> + */
> +function findAccessibleChildByID(accessible, id) {

same. Looks usefule outside of events.js.

::: accessible/tests/browser/events.js:122
(Diff revision 2)
> +function findAccessibleChildByID(accessible, id) {
> +  if (getAccessibleDOMNodeID(accessible) === id) {
> +    return accessible;
> +  }
> +  for (let i = 0; i < accessible.children.length; ++i) {
> +    let found = findAccessibleChildByID(accessible.getChildAt(i), id);

Instead of a recursive search in JS, why not have an optimized path and use getElementById when the accessible is the doc accessible (which is 99% of the time?).

Also, this probably belongs in head.js too.

::: accessible/tests/browser/events.js:219
(Diff revision 2)
> +   * @param  {String}   eventType    accessible event type
> +   * @param  {Function} handleEvent  event handler function
> +   */
> +  register(eventType, handleEvent) {
> +    // Add observer when adding the first applicant only.
> +    if (!(this.applicantsCount++)) {

What if the same handler is added twice? applicantsCount will be 2, but there will only be one stored handler. If the handler is unregistered, there will be no handlers, but the applicantsCount will be 1, and the observer will still be registered.

::: accessible/tests/browser/events.js:227
(Diff revision 2)
> +    if (!(eventType in this.listeners)){
> +      this.listeners[eventType] = [];
> +    }
> +
> +    let listenersArray = this.listeners[eventType];
> +    if (!listenersArray.includes(handleEvent)) {

Just a suggestion: Use a set instead of an array. Then you don't ever worry about duplicates.

::: accessible/tests/browser/events.js:240
(Diff revision 2)
> +   * @param  {Function} handleEvent  event handler function
> +   */
> +  unregister(eventType, handleEvent) {
> +    // Remove observer when there are no more applicants only.
> +    // '< 0' case should not happen, but just in case: removeObserver() will throw.
> +    if (--this.applicantsCount <= 0) {

Similar issue as above. I could just run the counter down by calling unregister('foo', null).

The counter should just be incremented/decremented after a handler is successfully added or removed. Alternatively, you could get rid of the separate counter and actually count the stored handlers.

::: accessible/tests/browser/events.js:253
(Diff revision 2)
> +    let index = listenersArray.indexOf(handleEvent);
> +    if (index === -1) {
> +      return;
> +    }
> +
> +    listenersArray.splice(index, 1);

Removing a handler from a set would be so much easier..

::: accessible/tests/browser/events.js:279
(Diff revision 2)
> +      // event.
> +      if (domID !== id) {
> +        return;
> +      }
> +
> +      is(id, domID, `Correct event DOMNode id. Expected: ${id}, got ${domID}`);

You just checked that the id matches one line above. What is the purpose of this?

::: accessible/tests/browser/events.js:280
(Diff revision 2)
> +      if (domID !== id) {
> +        return;
> +      }
> +
> +      is(id, domID, `Correct event DOMNode id. Expected: ${id}, got ${domID}`);
> +      is(event.eventType, eventType,

The a11yEventObserver will only have this handler run on the right event type.

::: accessible/tests/browser/head.js:169
(Diff revision 2)
> +      url: url,
> +    }, function*(browser) {
> +      yield SimpleTest.promiseFocus(browser);
> +
> +      loadFrameScripts(browser,
> +        'let { document, window, navigator } = content;',

I thought frame scripts live in their own scopes. So I am not sure how assigning global variables in one script would work in another. But I guess it does?

::: accessible/tests/browser/head.js:179
(Diff revision 2)
> +
> +      let onDocLoad = waitForEvent(EVENT_DOCUMENT_LOAD_COMPLETE, 'body');
> +      browser.reload();
> +      let event = yield onDocLoad;
> +
> +      yield task(browser, event);

From all the tests I have seen, they just need the top-level doc acc. So why not just pass event.accessible?
Attachment #8738716 - Flags: review?(eitan)
(In reply to Trevor Saunders (:tbsaunde) from comment #40)
> > But seriously, I think these classes are overkill. Promises and generators
> > allow us to keep things simple and function based and not get wound up in
> > the patterns used in the a11y event mochitest.
> 
> So, I've said it before, and I haven't understood this patch well enough to
> look at this part of it, but personally I really prefer the mochitest-a11y
> style of using an explicit queue of expected events  Hiding thunks inline
> with syntactic sugar is awful on its own.  More importantly  though these
> tests are conceptually a state machine where event handlers perform the
> state transitions, so imho it makes sense to have explicit global state for
> what state the machine is in, and a bunch of separate functions to
> transition between states.  That said I'm probably not going to spend more
> time arguing about this.

IMHO generators make for easier and clearer tests. I am not the one who will touch these much, so I don't think I should get a say. But I was asked to review, so I am.
Assignee

Comment 43

3 years ago
https://reviewboard.mozilla.org/r/44663/#review42359

> Instead of a recursive search in JS, why not have an optimized path and use getElementById when the accessible is the doc accessible (which is 99% of the time?).
> 
> Also, this probably belongs in head.js too.

If I understand correctly, you are talking about content here. In that case, there is no access to content elements from parent so I can't get a content document this way.

> I thought frame scripts live in their own scopes. So I am not sure how assigning global variables in one script would work in another. But I guess it does?

Yeah seems to work just fine...
Assignee

Comment 44

3 years ago
https://reviewboard.mozilla.org/r/44663/#review42359

> Why all these skips on e10s? Are these just TODOs?

Yes, currently not everything is supported with e10s, for example SHOW and HIDE events are not passed to parrent ATM. These flags will eventually be removed.
Assignee

Comment 45

3 years ago
https://reviewboard.mozilla.org/r/44663/#review42359

> Interesting.. this needs to happen for a REORDER to happen?

Yeah bug 745788
Assignee

Comment 46

3 years ago
https://reviewboard.mozilla.org/r/44663/#review42359

> What if the rule doesn't have `recreated` or `textchanged`? I would expect you would still need to wait for a certain event to know for certain that the name change took affect.

Tried name change event and it is not always reliable.
Assignee

Comment 47

3 years ago
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44663/diff/2-3/
Attachment #8738716 - Flags: review?(eitan)
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

https://reviewboard.mozilla.org/r/44663/#review44207

I'm pretty happy with this! Just some concerns and one suggestion down below.

::: accessible/tests/browser/events.js:85
(Diff revisions 2 - 3)
>  
> -    let listenersArray = this.listeners[eventType];
> -    if (!listenersArray.includes(handleEvent)) {
> -      listenersArray.push(handleEvent);
> -    }
> -  },
> +        let domID = getAccessibleDOMNodeID(event.accessible);
> +        // If event's accessible does not match expected event type or DOMNode
> +        // id, skip thie event.
> +        if (domID === id && event.eventType === eventType) {
> +          ok(true, `Correct event DOMNode id. Expected: ${id}, got ${domID}`);

When I raised this issue earlier, I was wondering why we need to have a call to ok() in the first place for id and event type, not that you compare them twice.

I guess my issue is that these to lines are redundant, period. No?

::: accessible/tests/browser/events.js:91
(Diff revisions 2 - 3)
> -   */
> -  unregister(eventType, handleEvent) {
> -    // Remove observer when there are no more applicants only.
> -    // '< 0' case should not happen, but just in case: removeObserver() will throw.
> -    if (--this.applicantsCount <= 0) {
> -      Services.obs.removeObserver(this, "accessible-event");
> +          Services.obs.removeObserver(this, "accessible-event");

This all works nicely with simple flat generators. But I think this could unravel quickly. What if there are multiple pending waitForEvent promises with the same accessible ID and event type? For example, waitForMultipleEvents.

The order in which they get resolved would be dependent on what observer gets called first, which depends on the observer service implementation. If it calls them in the same order they are added, you're good, if it uses a hash table, or if it is reveresed, bad things will happen.

You can drop this issue if the observer service calls the observers in the right order.

::: accessible/tests/browser/events.js:96
(Diff revisions 2 - 3)
> -    if (!listenersArray.length) {
> -      delete this.listeners[eventType];
> -    }
> -  }
> +      }
> -};
> +    };
> -
> +    Services.obs.addObserver(eventObserver, "accessible-event", false);

Would I have reason to be concerned that perhaps addObserver is asynchronous? If it is, or theoretically could be, you have a potential for a race condition when you do waitForEvent and spawn a content task. You depend on the former being complete before the latter starts.

Consolidating the observers mitigates that risk but doesn't eliminate it, since you would have the same potential problem on the first waitForEvent.

Again, this is only if addObserver is async. The more I look, the more I believe it isn't.. so I am not marking this as an issue. Just sayin'

::: accessible/tests/browser/events.js:115
(Diff revisions 2 - 3)
> -      a11yEventObserver.unregister(eventType, handleEvent);
> -      resolve(event);
> +  return Promise.all(events.map(({ eventType, id }, idx) =>
> +    // In addition to waiting for an event, attach an order checker for the
> +    // event.
> +    waitForEvent(eventType, id).then(resolvedEvent => {
> +      // Verify that event happens in order.
> +      events.forEach((evt, i) => {

This seems a bit complicated for what you want to do.

in the function:
```
let currentIdx = 0;
```

in the promise callback:

```
is(idx, currentIdx++, ...);

```

::: accessible/.eslintrc:1
(Diff revision 3)
> +{

Why are you introducing an eslintrc in the top-level accessibility directory? Lets start with the only the browser tests, until we agree on a standard style.

You also seem to add exclusions for all the js directoruies in .eslintignore, so that can be removed too.
Attachment #8738716 - Flags: review?(eitan)
Assignee

Comment 50

3 years ago
(In reply to Eitan Isaacson [:eeejay] from comment #49)
> https://reviewboard.mozilla.org/r/44663/#review44311
> 
> Also, it looks like it is failing in try.

Yeah I stopped that since dependent bug was not landed at the time
Assignee

Comment 51

3 years ago
https://reviewboard.mozilla.org/r/44663/#review44207

> When I raised this issue earlier, I was wondering why we need to have a call to ok() in the first place for id and event type, not that you compare them twice.
> 
> I guess my issue is that these to lines are redundant, period. No?

Yeah makes sense, i will info(...) that stuff.
Assignee

Comment 52

3 years ago
https://reviewboard.mozilla.org/r/44663/#review44207

> This all works nicely with simple flat generators. But I think this could unravel quickly. What if there are multiple pending waitForEvent promises with the same accessible ID and event type? For example, waitForMultipleEvents.
> 
> The order in which they get resolved would be dependent on what observer gets called first, which depends on the observer service implementation. If it calls them in the same order they are added, you're good, if it uses a hash table, or if it is reveresed, bad things will happen.
> 
> You can drop this issue if the observer service calls the observers in the right order.

They are notified in the order added: https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/nsObserverList.cpp?from=nsObserverList%3A%3ANotifyObservers#112

> Would I have reason to be concerned that perhaps addObserver is asynchronous? If it is, or theoretically could be, you have a potential for a race condition when you do waitForEvent and spawn a content task. You depend on the former being complete before the latter starts.
> 
> Consolidating the observers mitigates that risk but doesn't eliminate it, since you would have the same potential problem on the first waitForEvent.
> 
> Again, this is only if addObserver is async. The more I look, the more I believe it isn't.. so I am not marking this as an issue. Just sayin'

I beleive they are synchronous.
In general both implementation do not handle a case well where we have 2 identical events for the same node. They would both try to handle it since propagation is not stopped. I think it just means that we do not have tests that test things like that. We could change the implementation if necessary if things change. Hopefully current implementation is simple enough to just work for what we need it.
Assignee

Comment 53

3 years ago
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44663/diff/3-4/
Attachment #8738716 - Flags: review?(eitan)
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

https://reviewboard.mozilla.org/r/44663/#review44947

Thanks!
Attachment #8738716 - Flags: review?(eitan) → review+
Backed out for failure in new test browser_events_statechange.js at least if run with e10s

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/93d4b13155d8

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=26419057&repo=mozilla-inbound
09:00:10     INFO -  4 INFO TEST-START | accessible/tests/browser/browser_events_statechange.js
09:00:10     INFO -  [Parent 1964] WARNING: g_type_add_interface_static: assertion `g_type_parent (interface_type) == G_TYPE_INTERFACE' failed: 'glib warning', file /builds/slave/m-in-l64-000000000000000000000/build/src/toolkit/xre/nsSigHandlers.cpp, line 142
09:00:10     INFO -  (firefox:1964): GLib-GObject-CRITICAL **: g_type_add_interface_static: assertion `g_type_parent (interface_type) == G_TYPE_INTERFACE' failed
09:00:11     INFO -  [Parent 1964] WARNING: g_type_add_interface_static: assertion `g_type_parent (interface_type) == G_TYPE_INTERFACE' failed: 'glib warning', file /builds/slave/m-in-l64-000000000000000000000/build/src/toolkit/xre/nsSigHandlers.cpp, line 142
09:00:11     INFO -  (firefox:1964): GLib-GObject-CRITICAL **: g_type_add_interface_static: assertion `g_type_parent (interface_type) == G_TYPE_INTERFACE' failed
09:00:11     INFO -  TEST-INFO | started process screentopng
09:00:13     INFO -  TEST-INFO | screentopng: exit 0
09:00:13     INFO -  5 INFO checking window state
09:00:13     INFO -  6 INFO Entering test bound
09:00:13     INFO -  7 INFO e10s enabled: true
09:00:13     INFO -  8 INFO Actually remote browser: true
09:00:13     INFO -  9 INFO TEST-PASS | accessible/tests/browser/browser_events_statechange.js | Accessible document present. -
09:00:13     INFO -  10 INFO TEST-UNEXPECTED-FAIL | accessible/tests/browser/browser_events_statechange.js | Uncaught exception - TypeError: content.document.getElementById(...) is null
09:00:13     INFO -  11 INFO Leaving test bound
09:00:13     INFO -  MEMORY STAT | vsize 1052MB | residentFast 266MB | heapAllocated 127MB
09:00:13     INFO -  12 INFO TEST-OK | accessible/tests/browser/browser_events_statechange.js | took 1224ms
09:00:13     INFO -  Not taking screenshot here: see the one that was previously logged
09:00:13     INFO -  13 INFO TEST-UNEXPECTED-FAIL | accessible/tests/browser/browser_events_statechange.js | Found an unexpected tab at the end of test run: data:text/html,%20%20%20%20%20%20%20%20<html>%20%20%20%20%20%20%20%20%20%20<head>%20%20%20%20%20%20%20%20%20%20%20%20<meta%20charset="utf-8"/>%20%20%20%20%20%20%20%20%20%20%20%20<title>Accessibility%20Test</title>%20%20%20%20%20%20%20%20%20%20</head>%20%20%20%20%20%20%20%20%20%20<body%20id="body">%20%20<iframe%20id="iframe"%20src="data:text/html,%20%20<html>%20%20%20%20<head>%20%20%20%20%20%20<meta%20charset='utf-8'/>%20%20%20%20%20%20<title>Inner%20Iframe</title>%20%20%20%20</head>%20%20%20%20<body%20id='iframe'></body>%20%20</html>"></iframe>%20%20<input%20id="checkbox"%20type="checkbox"%20/></body>%20%20%20%20%20%20%20%20</html> -
Flags: needinfo?(yzenevich)
Assignee

Comment 58

3 years ago
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44663/diff/4-5/
Assignee

Comment 59

3 years ago
FYI, Eitan, Alex asked not to open a bug for now while he's looking at the logs (https://pastebin.mozilla.org/8868907). That's why I don't have a bug reference in browser.ini
Flags: needinfo?(yzenevich)
Assignee

Updated

3 years ago
Attachment #8738716 - Flags: review+ → review?(eitan)
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

We discussed this earlier. Looks good!
Attachment #8738716 - Flags: review?(eitan) → review+
Assignee

Comment 63

3 years ago
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44663/diff/5-6/
Assignee

Comment 64

3 years ago
^ Cleaning up in case test fails (remove a11y even observers and a stale tab if available). Will open a window specific bug if I get any further with my testing.
Assignee

Comment 65

3 years ago
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

Eitan, could you take a look a couple of minor changes that make the testing a bit more stable. I'm also disabling this test on Windows with e10s for the time being, while I'm investigating the test failure.
Attachment #8738716 - Flags: review+ → review?(eitan)
Attachment #8738716 - Flags: review?(eitan) → review+
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

https://reviewboard.mozilla.org/r/44663/#review46345
Open a bug for the e10s on windows failure.
Assignee

Comment 69

3 years ago
Looks like document loaded event does not fire for the tab with test content:

e10s log:
https://pastebin.mozilla.org/8869230

non e10s log:
https://pastebin.mozilla.org/8869231
Assignee

Updated

3 years ago
Depends on: 1269369
Assignee

Comment 70

3 years ago
Comment on attachment 8738716 [details]
MozReview Request: Bug 1186029 - e10s compatible name caching, events and tree update tests. r=eeejay

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44663/diff/6-7/
Assignee

Comment 71

3 years ago
Hopefully final try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=635ba847c354

Currently these are disabled on Windows with e10s because of bug 1269369.
Assignee

Updated

3 years ago
Keywords: leave-open
Assignee

Updated

3 years ago
Depends on: 1272336
Assignee

Updated

3 years ago
Depends on: 1275938
Assignee

Updated

3 years ago
Depends on: 1275983
Assignee

Updated

3 years ago
Depends on: 1276721
Assignee

Comment 74

3 years ago
Attachment #8735888 - Attachment is obsolete: true
Attachment #8738716 - Attachment is obsolete: true
Attachment #8765606 - Flags: review?(eitan)
Assignee

Comment 76

3 years ago
Comment on attachment 8765606 [details] [diff] [review]
1186029 doc treeupdate patch

Ah looks like it still fails..
Attachment #8765606 - Flags: review?(eitan)
Depends on: 1305124
Yura just a reminder to enable some of these today if you can. (Maybe use this or a spin off bug)
Flags: needinfo?(yzenevich)
Assignee

Updated

2 years ago
Depends on: 1337401
Assignee

Comment 78

2 years ago
(In reply to David Bolter [:davidb] from comment #77)
> Yura just a reminder to enable some of these today if you can. (Maybe use
> this or a spin off bug)

Opened bug 1337401
Flags: needinfo?(yzenevich)

Updated

2 years ago
Whiteboard: aes+

Updated

2 years ago
Whiteboard: aes+

Updated

2 years ago
Depends on: 1337887
Assignee

Comment 79

2 years ago
This can probably be closed. a11y OTH tests are not e10s specific , so bug 1337887 is not really related to this.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.