857946 - [AccessFu] After closing all tabs, newly loaded pages are no longer accessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. With TalkBack on, run Fennec.
2. From the home screen, choose a page. Any will do.
3. Focus and double-tap the "1 tab" button.
4. From the tabs tray, find the Close Tab button to the right of the entry for the page you just opened, and double-tap it.
5. You're now back at the home screen. Choose the same page again, or a different one, doesn't matter.

Expected: Page should load and TalkBack should be able to read the content as before.
Actual: Nothing is spoken, the only thing that TalkBack can access now is the browser chrome. Content is no longer accessible.

This can be reproduced in all current versions, 20 to 23.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 857946 patch v1

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 746230 [details] [diff] [review]
857946 patch v1

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

This looks good. Might not completely remedy the design issues, but it fixes a bug. So I would land this.

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: other thoughts

Comment 4

[Eitan Isaacson [:eeejay]]

Comment on attachment 747001 [details] [diff] [review]
other thoughts

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

Overall the right direction.

::: accessible/src/jsat/EventManager.jsm
@@ +28,5 @@
> +        this.contentScope = aConentScope;
> +        this.contentScope.addEventListener = this.contentScope.addEventListener.
> +          bind(this.contentScope)
> +        this.contentScope.removeEventListener = this.contentScope.
> +          removeEventListener.bind(this.contentScope)

This looks scary, aren't you modifying addEventListener/removeEventListener in-place? Calling it from content-script.js would stop working, no?
Don't you want to do:
this.addEventListener = this.contentScope.addEventListener.bind(this.contentScope);

(also missing semi-colons)

@@ +36,1 @@
>          Services.obs.addObserver(this, 'accessible-event', false);

We only want one observer per-process, since this gets expensive. I would have a module-scoped object called something like AccessibilityEventObserver which would truly be singleton (and move EventManager to per-window, as this patch essentially does).
You would then do here:
AccessibilityEventObserver.addListener(this);
and removeListener on stop().

AEO would add itself as an observer of accessible-event when listener count goes from 0 -> 1. When there are 0 listeners, AEO would remove itself as an accessible-event observer.  AEO would send the event to the appropriate EventManager by doing the following check: aEvent.accessibleDocument.window == listener.contentScope.content

@@ +41,5 @@
> +             Ci.nsIWebProgress.NOTIFY_LOCATION));
> +        this.contentScope.addEventListener('scroll', this, true);
> +        this.contentScope.addEventListener('resize', this, true);
> +        // XXX: Ideally this would be an a11y event. Bug #742280.
> +        this.contentScope.addEventListener('DOMActivate', this, true);

It is great to have these directly in here.

::: accessible/src/jsat/content-script.js
@@ +14,4 @@
>  
>  Logger.debug('content-script.js');
>  
> +// A Lazy getter for an instance of the EventManager.

If you want to import a js module in a lazy fashion, their is a function for that too, XPCOMUtils.defineLazyModuleGetter (we really should use it everywhere in AccessFu).

@@ +17,5 @@
> +// A Lazy getter for an instance of the EventManager.
> +XPCOMUtils.defineLazyGetter(this, "EventManager", function () {
> +  let scope = {};
> +  Cu.import('resource://gre/modules/accessibility/EventManager.jsm', scope);
> +  return Object.create(scope.EventManager);

This is overly-complex for making EventListener non-singleton. I would simply do it the usual way:
function EventListener() {...}
EventListener.prototype = {...}

let eventListner = new EventListner();

@@ +67,5 @@
>            vc.moveFirst(TraversalRules.Simple);
> +        else {
> +          sendAsyncMessage('AccessFu:Present', Presentation.pivotChanged(
> +            vc.position, null, Ci.nsIAccessiblePivot.REASON_NONE));
> +        }

This is a welcome change.

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 857946 v3

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 857946 v4

Added some logging and better error handling.

Comment 7

[Eitan Isaacson [:eeejay]]

Comment on attachment 747441 [details] [diff] [review]
Patch for 857946 v4

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

Overall looks good. AEO could be dramatically simplified. If what I suggest there does not work, you should be using a WeakMap for listeners where the key is the window. That will:
a. Not hold a strong reference to the window (which is dangerous in global js modules).
b. Listener lookup will be one line.

Anyway, would love to see another revision, so I am removing the review flag.

::: accessible/src/jsat/AccessibilityEventObserver.jsm
@@ +12,5 @@
> +  'resource://gre/modules/Services.jsm');
> +XPCOMUtils.defineLazyModuleGetter(this, "Logger",
> +  'resource://gre/modules/accessibility/Utils.jsm');
> +
> +this.EXPORTED_SYMBOLS = ['AccessibilityEventObserver'];

I think this should just live in EventManager.jsm and be private there.

@@ +19,5 @@
> +
> +  /**
> +   * A map containing [EventManager, content window] pairs.
> +   */
> +  eventManagers: new Map(),

You don't need this. See below...

@@ +28,5 @@
> +   *
> +   * @param aEventManager EventManager
> +   *        An EventManager object that was loaded into the specific content.
> +   */
> +  addListener: function addListener(aEventManager) {

You don't need to take an argument, just just increase listener counter by one. See below...

@@ +32,5 @@
> +  addListener: function addListener(aEventManager) {
> +    this.eventManagers.set(aEventManager, aEventManager.contentScope.content);
> +    // Since at least one EventManager was registered, start listening.
> +    Logger.info('AccessibilityEventObserver.addListener. Total:',
> +      this.eventManagers.size);

Maybe change these to debug()?

@@ +43,5 @@
> +   *
> +   * @param aEventManager EventManager
> +   *        An EventManager object that was stopped in the specific content.
> +   */
> +  removeListener: function removeListener(aEventManager) {

You just need to decrement listener count, no need for argument.

@@ +69,5 @@
> +    let event = aSubject.QueryInterface(Ci.nsIAccessibleEvent);
> +    if (!event.accessibleDocument) {
> +      return;
> +    }
> +    // Match the content window to its EventManager.

All you need to do here is check for an active eventManager in a window:

let eventManager = event.accessibleDocument.window.eventManager;
if (eventManager && eventManager.started) {
  eventManager.handleAccEvent(event);
}

::: accessible/src/jsat/EventManager.jsm
@@ +10,5 @@
> +  'resource://gre/modules/accessibility/Utils.jsm');
> +XPCOMUtils.defineLazyModuleGetter(this, "Logger",
> +  'resource://gre/modules/accessibility/Utils.jsm');
> +XPCOMUtils.defineLazyModuleGetter(this, "AccessibilityEventObserver",
> +  'resource://gre/modules/accessibility/AccessibilityEventObserver.jsm');

Like I wrote earlier, I would simply have this object here and scoped privately.

@@ +23,5 @@
>  
>  this.EventManager = {
>    editState: {},
>  
> +  set contentScope(aContentScope) {

I don't think it is worthwhile creating setters and getters for something you only do once. There doesn't see to be any added benefit.

::: accessible/src/jsat/content-script.js
@@ +21,5 @@
>  
> +// A Lazy getter for an instance of the EventManager.
> +XPCOMUtils.defineLazyGetter(this, 'eventManager', function () {
> +  return Object.create(EventManager);
> +});

This seems a bit overkill, and less readable.
I would simply have
let eventManager = null;
and then instantiate one if needed before start().
Also, I am partial to new EventManager(), there are half a dozen ways to do things in js, they are all legit, but lets stick to one if there is no added benefit.

@@ +135,5 @@
>  
>        let node = aAccessible.DOMNode || aAccessible.parent.DOMNode;
>  
>        function dispatchMouseEvent(aEventType) {
> +        let evt = content.document.createEvent('MouseEvents');

Why is this line touched? Whitespace?

@@ +142,5 @@
>          node.dispatchEvent(evt);
>        }
>  
> +      dispatchMouseEvent('mousedown');
> +      dispatchMouseEvent('mouseup');

And these?

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #7)
> Comment on attachment 747441 [details] [diff] [review]
> Patch for 857946 v4
> 
> Review of attachment 747441 [details] [diff] [review]:
> -----------------------------------------------------------------

> >        function dispatchMouseEvent(aEventType) {
> > +        let evt = content.document.createEvent('MouseEvents');
> 
> Why is this line touched? Whitespace?
> 
> @@ +142,5 @@
> >          node.dispatchEvent(evt);
> >        }
> >  
> > +      dispatchMouseEvent('mousedown');
> > +      dispatchMouseEvent('mouseup');
> 
> And these?

In both places I just changed double to single quotes (seems like that's what is mostly used in jsat).

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 857946 v5

Couple of comments:
* AccessibilityEventObserver could still be less complicated as I'm still keeping track of _contents in addition to size. Without it, I can't think of a way of detecting the real size of the WeakMap (other than when EventManagers explicitly call stop). It might be not necessary (at least on android) since when AccessFu is enabled, the size does not really go to 0 (even when all tabs are closed).
* size getter is pretty complicated because of ^ and the fact that dead contents are cleaned up there (so the real size is returned).
* I am still using XPCOMUtils.defineLazyGetter for an eventManager in content-script so new EventManager is called exactly once. This way we do not need to check if it's already created on AccessFu:start. Hope that's ok.

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 747926 [details] [diff] [review]
Patch for 857946 v5

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

Overall looks good. Probably needs some more testing before landing?

::: accessible/src/jsat/EventManager.jsm
@@ +294,5 @@
> +
> +  /**
> +   * An array of eventManagers WeakMap's keys.
> +   */
> +  _contents: [],

Having an array of weakmap keys defeats the purpose of weakmap, since it is now a hard reference.

@@ +300,5 @@
> +  /**
> +   * A total number of registered eventManagers.
> +   */
> +  get size() {
> +    return this._contents.reduce(function accumulator(size, content, index) {

All you need is an integer attribute that you increment and decrement on adding and removal of listeners.

@@ +324,5 @@
> +   */
> +  addListener: function addListener(aEventManager) {
> +    let content = aEventManager.contentScope.content;
> +    this.eventManagers.set(content, aEventManager);
> +    this._contents.push(content);

This will break if the same event manager is added twice. this._contents.length will be one more than the size of the weakmap.
if (!this.eventManagers.has(content))
  this.listenerCount++;

@@ +339,5 @@
> +   *        An EventManager object that was stopped in the specific content.
> +   */
> +  removeListener: function removeListener(aEventManager) {
> +    let content = aEventManager.contentScope.content;
> +    if (!this.eventManagers.has(content)) {

You could simply call delete() here, it will return true if an item was actually deleted.

@@ +344,5 @@
> +      return;
> +    }
> +    // If an EventManager was previously registered, unregister it.
> +    this.eventManagers.delete(content);
> +    this._contents.splice(this._contents.indexOf(content), 1);

this.listenerCount--;

@@ +365,5 @@
> +    if (!event.accessibleDocument) {
> +      return;
> +    }
> +    let content = event.accessibleDocument.window;
> +    if (!this.eventManagers.has(content)) {

If the content is not the chrome window (in desktop firefox), we should log a warning, because we are somehow failing to deliver a11y events.

Also, in the case of content being a child frame, we would need to walk up the hierarchy to the parent window.

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #9)
> Created attachment 747926 [details] [diff] [review]
> Patch for 857946 v5
> 
> Couple of comments:
> * AccessibilityEventObserver could still be less complicated as I'm still
> keeping track of _contents in addition to size. Without it, I can't think of
> a way of detecting the real size of the WeakMap (other than when
> EventManagers explicitly call stop). It might be not necessary (at least on
> android) since when AccessFu is enabled, the size does not really go to 0
> (even when all tabs are closed).
> * size getter is pretty complicated because of ^ and the fact that dead
> contents are cleaned up there (so the real size is returned).

Like I suggested above, a simple integer will probably do a better job.

> * I am still using XPCOMUtils.defineLazyGetter for an eventManager in
> content-script so new EventManager is called exactly once. This way we do
> not need to check if it's already created on AccessFu:start. Hope that's ok.

I'm fine with that. Not my personal preference, but I could live with this :)

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #10)
> Comment on attachment 747926 [details] [diff] [review]
> Patch for 857946 v5
> 
> Review of attachment 747926 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also, in the case of content being a child frame, we would need to walk up
> the hierarchy to the parent window.

From the documentation looks like |content| is "The current top level window in the frame or null."

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 857946 v6

Hope I understood you correctly re:
> If the content is not the chrome window (in desktop firefox), we should log
> a warning, because we are somehow failing to deliver a11y events.
> 
> Also, in the case of content being a child frame, we would need to walk up
> the hierarchy to the parent window.

Note, while running jsat tests, those warnings are logged quite a bit.

Comment 14

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #13)
> Created attachment 748484 [details] [diff] [review]
> Patch for 857946 v6
> 
> Hope I understood you correctly re:
> > If the content is not the chrome window (in desktop firefox), we should log
> > a warning, because we are somehow failing to deliver a11y events.
> > 
> > Also, in the case of content being a child frame, we would need to walk up
> > the hierarchy to the parent window.
> 
> Note, while running jsat tests, those warnings are logged quite a bit.

The end result should be that there are no warnings in Firefox desktop or during mochitests. The warnings should indicate when no listener was found for the event AND it is not the chrome window. I'm going to test this and suggest tweaks for making that work, since I am using desktop Firefox all the time now with AccessFu.

Comment 15

[Eitan Isaacson [:eeejay]]

Comment on attachment 748484 [details] [diff] [review]
Patch for 857946 v6

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

Looking good. Ready for landing after nits.

::: accessible/src/jsat/EventManager.jsm
@@ +61,5 @@
>        Logger.logException(x);
>      }
>    },
>  
>    stop: function stop() {

Put in an XXX comment here that says something about this not being called when tabs close.

::: accessible/src/jsat/content-script.js
@@ +19,4 @@
>  
>  Logger.debug('content-script.js');
>  
> +let eventManager;

Initialize this to null.

Comment 16

[Eitan Isaacson [:eeejay]]

Comment on attachment 748484 [details] [diff] [review]
Patch for 857946 v6

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

Some additional notes.

::: accessible/src/jsat/EventManager.jsm
@@ +23,5 @@
>  
> +function EventManager(aContentScope) {
> +  this.contentScope = aContentScope;
> +  this.addEventListener = this.contentScope.addEventListener.bind(
> +    this.contentScope)

Semicolon

@@ +25,5 @@
> +  this.contentScope = aContentScope;
> +  this.addEventListener = this.contentScope.addEventListener.bind(
> +    this.contentScope)
> +  this.removeEventListener = this.contentScope.removeEventListener.bind(
> +    this.contentScope)

Semicolon

@@ +61,5 @@
>        Logger.logException(x);
>      }
>    },
>  
>    stop: function stop() {

Put in an XXX comment here that says something about this not being called when tabs close.

@@ +352,5 @@
> +    let content = event.accessibleDocument.window.top;
> +    if (Utils.MozBuildApp === 'browser' &&
> +        !(content instanceof Ci.nsIDOMChromeWindow)) {
> +      Logger.warning(
> +        'AccessibilityEventObserver.observe: content is not a ChromeWindow');

This is not what I meant. See below.

@@ +357,5 @@
> +      return;
> +    }
> +    // Match the content window to its EventManager.
> +    let eventManager = this.eventManagers.get(content);
> +    if (!eventManager || !eventManager._started) {

If this is NOT a chrome window, log a warning. Just move the conditional above into this block.

::: accessible/src/jsat/content-script.js
@@ +19,4 @@
>  
>  Logger.debug('content-script.js');
>  
> +let eventManager;

Initialize this to null.

Comment 17

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 857946 v7

I added 2 methods to AEO: |start| and |stop|. Stop also clears the WeakMap and is used if the AccessFu is not initialized but the observer is still registered.

Comment 18

[Eitan Isaacson [:eeejay]]

Comment on attachment 749275 [details] [diff] [review]
Patch for 857946 v7

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

Looks good! Just a warning message tweak below... Upload a new patch, and I'll land it in incoming.

::: accessible/src/jsat/EventManager.jsm
@@ +397,5 @@
> +    if (!eventManager || !eventManager._started) {
> +      if (Utils.MozBuildApp === 'browser' &&
> +          !(content instanceof Ci.nsIDOMChromeWindow)) {
> +        Logger.warning(
> +          'AccessibilityEventObserver.observe: content is not a ChromeWindow');

The warning should tell us that we are not handling potential events of interest. Maybe something like,

"AccessibilityEventObserver.observe: ignored event:",
Logger.eventToString(event), "accessible:", Logger.accessibleToString(event.accessible), "document:",
Logger.accessibleToString(event.accessibleDocument)

Comment 19

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 857946 v8

Carrying over the r+.

Comment 20

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 857946 v9

Added a hierarchical lookup of the eventManager based on given content.

Comment 21

[Eitan Isaacson [:eeejay]]

Comment on attachment 749410 [details] [diff] [review]
Patch for 857946 v9

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

Looks good. I tested on B2G, and ran in to the issue below with OOP content.

::: accessible/src/jsat/EventManager.jsm
@@ +21,4 @@
>  
>  this.EXPORTED_SYMBOLS = ['EventManager'];
>  
> +function EventManager(aContentScope) {

this.EventManager = function EventManager(aContentScope) {

@@ +31,5 @@
> +    this.contentScope);
> +  this.webProgress = this.contentScope.docShell.
> +    QueryInterface(Ci.nsIInterfaceRequestor).
> +    getInterface(Ci.nsIWebProgress);
> +}

remember semicolon after change above.

@@ +383,5 @@
> +    }
> +    let parent = content.parent;
> +    if (parent === content) {
> +      // There is no parent or the parent is of a different type.
> +      return;

return null;

@@ +397,5 @@
> +      return;
> +    }
> +    try {
> +      if (!Utils.win) {
> +        this.stop();

Is this how you check to see if AccessFu is initialized? This won't work in OOP content since Utils.win will always be null.

@@ +401,5 @@
> +        this.stop();
> +        return;
> +      }
> +    } catch (x) {
> +      // AccessFu is not initialized.

What exception are you catching here?

Comment 22

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #21)
> @@ +401,5 @@
> > +        this.stop();
> > +        return;
> > +      }
> > +    } catch (x) {
> > +      // AccessFu is not initialized.
> 
> What exception are you catching here?

If Utils._win is falsy, Utils._win.get() will throw.

Comment 23

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #22)
> (In reply to Eitan Isaacson [:eeejay] from comment #21)
> > @@ +401,5 @@
> > > +        this.stop();
> > > +        return;
> > > +      }
> > > +    } catch (x) {
> > > +      // AccessFu is not initialized.
> > 
> > What exception are you catching here?
> 
> If Utils._win is falsy, Utils._win.get() will throw.

So the getter needs to be fixed to return null.

Comment 24

[Eitan Isaacson [:eeejay]]

With further testing, I discovered that besides 'remote-browser-frame-shown' being observed, we also need to observe 'in-process-browser-or-app-frame-shown' for non-OOP cases.

Comment 25

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 857946 v10

Comment 26

[Eitan Isaacson [:eeejay]]

Comment on attachment 751351 [details] [diff] [review]
Patch for 857946 v10

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

Looks great!

::: accessible/src/jsat/EventManager.jsm
@@ +396,5 @@
> +    if (aTopic !== 'accessible-event') {
> +      return;
> +    }
> +    let event = aSubject.QueryInterface(Ci.nsIAccessibleEvent);
> +    if (!event.accessibleDocument) {

Is this ever null? If it is, I think we should warn about that too.

Comment 27

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 857946 v11

Added a warning. Carrying over the r+

Comment 28

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50416303e2bb

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/50416303e2bb