1857266 - Explore capturing click events to perform accessibility checks when multiple windows are opened

Status: NEW

(Testing :: General, task, P3)

Description

[Anna Yeddi [:ayeddi]]

We need to investigate the multi-window situation and ensure these cases are covered by the Tier 2 a11y_checks.

From the Phab investigation:

:Gijs (he/him):

Have you tested this with tests that open multiple windows (e.g. that call `openNewBrowserWindow) ?

I ask because from code inspection, here is how I think this patch / the framework works right now.

1. browser-test.js gets loaded in the initial browser window
2. it loads EventUtils and a11yutils
3. the test runs
4. it opens some other window
5. confusingly, reading mochitest/api.js, my impression is browser-test.js gets loaded in every window on android, but not on desktop. So nothing ever invokes this._handler.addEventListener here.

... even if it did (ie if something re-invoked this), we only track one this._handler so we'd just attach a second listener to the same window and that's no good. But also the script seems designed to work in more than 1 situation in terms of the kind of window it's invoked in (given the event handler / domwindow stuff) so... what's up with that? :-)

I... I hope/guess I'm missing something? I'm just not sure what, ie what ensures that each window gets its own instance of A11yUtils? And do we care about this for all browser windows or all windows (also e.g. library window, migration wizard, about window, etc. etc.) ? Maybe this is fodder for a follow-up?

James Teh [:Jamie]:

we're almost certainly missing windows other than the one opened by browser-test.js. I vaguely recall I thought about this at some point, but clearly forgot. I guess I might have figured that most tests would test most UI in a single browser window, since once browser window is normally the same as any other. However, maybe we have a lot of tests that open different browser windows for whatever reason? We do seem to have a lot which use openNewBrowserWindow. And this certainly doesn't cover non-browser windows.

I inherited this patch from Yura, who is no longer with us. I'm honestly not sure why we had the this._handler conditional given that we only appear to support one window. Maybe that was the beginnings of trying to support multiple windows? 🤔

I'm not sure if we want each window to have its own instance of AccessibilityUtils or whether we just want to use the same instance for all windows. If I understand things correctly, even when a test opens a new browser window, it still uses EventUtils, etc. from the window created by browser-test.js. Since EventUtils calls into AccessibilityUtils to suppress event handling for drag and drop (part 3 in this stack), we probably want to use a single instance.

You linked to a piece in api.js which can detect new browser windows. Do you think it'd be reasonable to have that code somehow notify AccessibilityUtils so it can add an event listener? That seems tricky, since that isn't just used for browser tests IIUC. I guess we'd need to add setupForWindow and teardownForWindow (or whatever) methods to AccessibilityUtils which take a window. We could then get rid of the this._handler thing and rely on those methods being called.

In any case, we'll probably want to do this in a follow-up, because this is likely to cause a whole bunch of new failures and it's getting to the point where we really need to get this initial stack landed to prevent continual bitrot as new tests get introduced and things get shuffled around.