1428430 - Extend accessibility spec/actor/fronts to include necessary functionality for the a11y inspector.

Status: RESOLVED FIXED

(DevTools :: Accessibility Tools, enhancement, P2)

Description

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

New functionality includes:

* highlighter related methods and events
* ancestry lookup for accessibles that allows to select arbitrary accessible objects on the page (picker, context menu, etc)
* enable/disable functionality for accessibility service.

Comment 1

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

Attachment: 1428430 patch

This is almost ready, I'm attaching mostly for reference for bug 1428431

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8943488 [details] [diff] [review]
1428430 patch

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

Alex: do you mind giving a look at some of the changes in devtools/server/actors/accessibility.js? I'd like you to review the AccessibilityParent class, the call to setupInParent and related code (towards the bottom), since that's a thing I've never done and I feel there may be traps that you know about. I'll review the rest.

::: devtools/server/actors/accessibility.js
@@ +681,5 @@
> +      event.stopPropagation();
> +      event.preventDefault();
> +    };
> +
> +    this._onPick = async event => {

I know the DOM Walker does this too, but I think it's a mistake to define these event handlers (onPick, onHovered and onKey) inside here. Because everytime the pick method is called, these 3 functions are re-created.
Can you move them to the prototype of the class, so we just always reuse the same functions?

@@ +728,5 @@
> +    };
> +
> +    this._onKey = async event => {
> +      if (!this._currentAccessible || !this._currentAccessible.DOMNode ||
> +          !this._isPicking) {

Do we need to check if this._isPicking is true here? This event handler only receives events when we're picking anyway.
So maybe remove this? Or also add it to the 2 other event handler functions for consistency.

@@ +798,5 @@
> +      while (parent && parent != doc) {
> +        parent = parent.parentAcc;
> +      }
> +    } catch (error) {
> +      throw new Error(`Failed to get ancestor for ${accessible}: ${error}`);

When does the while loop above throw an exception?

@@ +842,5 @@
>    }
>  });
>  
>  /**
> + * A generic highlighter actor class that instantiate a highlighter given its

This is a copu/paste from CustomHighlighterActor that doesn't make sense here.

@@ +847,5 @@
> + * type name and allows to show/hide it.
> + */
> +const AccessibleHighlighterActor = ActorClassWithSpec(customHighlighterSpec, {
> +  /**
> +   * Create a highlighter instance given its typename

Same for this comment.

@@ +904,5 @@
> +    }
> +  }
> +});
> +
> +class AccessibilityParent {

Please add some jsdoc to this class.
Also, please move it down closer to where you instantiate it, so the file reads easier.

::: devtools/shared/fronts/accessibility.js
@@ +160,5 @@
>      Front.prototype.initialize.call(this, client, form);
>      this.actorID = form.accessibilityActor;
>      this.manage(this);
> +
> +    Services.obs.addObserver(this, "a11y-consumers-changed");

A while ago, we made a big effort to make our front-end as web-like as possible, which meant removing XUL, etc. but also de-chroming it i.e. making our front-end code able to run without chrome privileges.
This observer (and the observe method further down) don't fall in this category I guess.
Isn't there a way for you to do this server-side instead? And send events from the server to the client when needed?

@@ +209,5 @@
> +  },
> +
> +  get canBeDisabled() {
> +    if (this.enabled) {
> +      let a11yService = Cc["@mozilla.org/accessibilityService;1"].getService(

Same thing here, this wouldn't work in a content-privileged environment, right? 
Can you look into moving this as an actor-method instead?

Comment 3

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

Attachment: 1428430 patch v2

Moved all chrome bits to actors.

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8947584 [details] [diff] [review]
1428430 patch v2

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

Thanks Yura.
One thing comes to mind: these actors have landed in an earlier Firefox version already, and now you're changing them. So we have to be mindful of backward compat.
Indeed, it is possible (thanks to remote debugging capabilities) to connect a toolbox to a debugger server running an older version.
So, when you'll write the client-side code that uses this, you'll need to do some feature detection and disable the tool if you're connected to actors that are too old.
This backward compatibility code will be needed on the client-side, not here. And that part hasn't landed yet. So you'll need to keep this in mind in the other patch (in bug 1151468 I believe, in getAccessibilityActor).

::: devtools/server/actors/accessibility.js
@@ +369,5 @@
> +    this.onPick = this.onPick.bind(this);
> +    this.onHovered = this.onHovered.bind(this);
> +    this.onKey = this.onKey.bind(this);
> +
> +    this.highlighter = CustomHighlighterActor(this, isXUL(this.tabActor.window) ?

Creating a CustomHighlighterActor with "XULWindowAccessibleHighlighter" will currently throw an error (see bug 1428431 comment 9, and https://searchfox.org/mozilla-central/source/devtools/server/actors/highlighters.js#455-466).

Are you going to change the CustomHighlighterActor code to avoid this?
If not, you should just use it for the AccessibleHighlighter class, and not for the XULWindowAccessibleHighlighter class.

@@ +653,5 @@
>      }
> +  },
> +
> +  /**
> +   * Publick method used to show an accessible object highlighter on the client

nit: publick -> public

@@ +832,5 @@
> +      this._isPicking = true;
> +      this._startPickerListeners();
> +    }
> +
> +    return null;

Do you need to return null here? It looks like that's the only value you can return anyway, so might not be needed.

@@ +842,5 @@
> +  pickAndFocus: function () {
> +    let pickResults = this.pick();
> +    this._highlighterEnv.window.focus();
> +
> +    return pickResults;

Similarly, why would you need to return here?

@@ +846,5 @@
> +    return pickResults;
> +  },
> +
> +  /**
> +   * Attach an element to DOMWalkerActor's tree.

this.domWalker is a WalkerActor, right? Not a DOMWalkerActor.

@@ +1033,5 @@
> +
> +    return this.conn.parentMessageManager;
> +  },
> +
> +  onEvent(msg) {

onMessage would be a better name here. The word event is also used for devtools protocol events, but the word message (at least here) is only use for the messagemanager. So, less chances of being confused.

@@ +1100,5 @@
> +    delete this.disabling;
> +  },
> +
> +  /**
> +   * Observe Accessibility service init and shutdown events. It relays the these

nit: the these -> these

@@ +1112,5 @@
> +   * @param  {String} data
> +   *         "0" corresponds to shutdown and "1" to init.
> +   */
> +  observe(subject, topic, data) {
> +    if (topic === "a11y-init-or-shutdown") {

Can you add a comment explaining the different events expected here?

@@ +1133,5 @@
> +  /**
> +   * Get or create AccessibilityWalker actor, similar to DOMWalker.
> +   *
> +   * @param  {Object} domWalker
> +   *         DOMWalkerActor that is needed for lookingup and attaching

WalkerActor instead of DOMWalkerActor, right?

@@ +1216,5 @@
> +   * @param {Object} mm
> +   *        Message manager that corresponds to the current content tab.
> +   */
> +  setMessageManager(mm) {
> +    if (this.messageManager) {

Can there be a case where this.messageManager === mm?
If so, maybe we should bail out early of this function when it happens.

In any case, can you add a short comment here explaining why we swap from an old to a new manager?

@@ +1228,5 @@
> +    }
> +  }
> +
> +  /**
> +   * Content AccessibilityActor message listeber.

nit: listeber -> listener

@@ +1254,5 @@
> +    }
> +  }
> +
> +  observe(subject, topic, data) {
> +    if (topic === "a11y-consumers-changed") {

These platform events are quite specific, and someone without exposure to a11y in Firefox will have a hard time maintaining this code. Can you add some comments here? Something that explains what events are expected and what they mean.

::: devtools/server/tests/browser/browser_accessibility_simple.js
@@ +41,5 @@
> +  yield onEvent;
> +  checkAccessibilityState(accessibility,
> +    { enabled: false, canBeDisabled: true, canBeEnabled: true });
> +
> +  info("Initialize accessibility serivce");

nit: serivce --> service 
(same below)

Comment 5

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

Attachment: 1428430 patch v3

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8948499 [details] [diff] [review]
1428430 patch v3

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

I think this looks good now.
I still would like Alex to give a look at the parent/child setup here before landing.

::: devtools/server/actors/accessibility.js
@@ +700,5 @@
> +    return this.highlighter.show(node, Object.assign({}, options, bounds));
> +  },
> +
> +  /**
> +   * Publick method used to hide an accessible object highlighter on the client

typo: publick -> public

@@ +1123,5 @@
> +   *         "0" corresponds to shutdown and "1" to init.
> +   */
> +  observe(subject, topic, data) {
> +    if (topic === "a11y-init-or-shutdown") {
> +      // This event is fired accessibility service is initialized or shut down.

typo: is fired *when* accessibility

@@ +1150,5 @@
> +    } else if (!this.disabling && topic === "nsPref:changed" &&
> +               data === PREF_ACCESSIBILITY_FORCE_DISABLED) {
> +      // PREF_ACCESSIBILITY_FORCE_DISABLED preference change event. When set to
> +      // >=1, it means that the user wants to disable accessibility service and
> +      // prevent it from starting in the future. Node: we also check

typo: Node -> Note

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8948499 [details] [diff] [review]
1428430 patch v3

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

Sorry for taking so long to look at this patch.
Here is some comments, I only looked at the actor implementation.

Are you using this actor in some WIP patch to implement a frontend or is it only used by tests so far?

::: devtools/server/actors/accessibility.js
@@ +231,5 @@
> +      return null;
> +    }
> +
> +    const attachment = domWalker.getNodeFromActor(this.actorID,
> +                                                  ["rawAccessible", "DOMNode"]);

It is unclear why you go through walker and getNodeFromActor
Couldn't you do:
  const attachment = this.rawAccessible.DOMNode;
?

@@ +396,5 @@
>      events.on(tabActor, "window-ready", this.onLoad);
>    },
>  
> +  setA11yServiceGetter() {
> +    DevToolsUtils.defineLazyGetter(this, "a11yService", () => {

Is it really necessary to use a lazy getter?
a11yService is used by getDocument and so by getAccessibleFor and children. Aren't they used immediately?

@@ +397,5 @@
>    },
>  
> +  setA11yServiceGetter() {
> +    DevToolsUtils.defineLazyGetter(this, "a11yService", () => {
> +      Services.obs.addObserver(this, "accessible-event");

Couldn't you listen for this once and remove the observer only in destroy?

@@ +791,5 @@
> +    if (!this._currentAccessible) {
> +      return;
> +    }
> +
> +    if (this._hoveredAccessible !== this._currentAccessible) {

I understand the purpose of _hoveredAccessible. It is used to emit picker-accessible-hovered only when the hovered node changed.
But _currentAccessible is more vague to me. I don't see why it is updated on hover, I would expect it to be set only from onPick. But then it would only be used from onKey, which is also surprising.

@@ +797,5 @@
> +      if (bounds) {
> +        this.highlighter.show(DOMNode, bounds);
> +      }
> +
> +      events.emit(this, "picker-accessible-hovered", this._currentAccessible);

You are emitting a lot of events from the actor.
Do you plan to use them all in your frontend?
Emitting too many events may have an impact on performance.

@@ +877,5 @@
> +  async _findAndAttachAccessible(event) {
> +    let target = event.originalTarget || event.target;
> +    let { node } = this.domWalker.attachElement(target);
> +    let accessible = await this.getAccessibleFor(node);
> +    if (accessible.indexInParent === -1) {

Shouldn't you use a11yservice to get the rawAccessible and indexInparent?
That would prevent going through attachElement+getAccessibleFor and create an AccessibleActor when indexInParent is -1.

@@ +885,5 @@
> +
> +    const doc = await this.getDocument();
> +    // There is a chance that ancestry lookup can fail if the accessible is in
> +    // the detached subtree. At that point the root accessible object would be
> +    // defunct and accessing it via parent property will throw.

What is a detached subtree? Is it a DOMElement not in a document?
If that's the case you could check for target.parentNode.

@@ +985,5 @@
> +    }));
> +  },
> +
> +  get enabled() {
> +    return Services.appinfo.accessibilityEnabled;

Will that work when the actor is in the child process?
If I follow your code correctly, you are going to instanciate
the accessibility xpcom only in the parent process, via AccesibilityParent. But not in the child process, where this actor runs. And so will Services.appinfo.accessibilityEnabled be toggled across processes?

@@ +1047,5 @@
> +
> +    switch (topic) {
> +      case "initialized":
> +        this.canBeEnabled = data.canBeEnabled;
> +        this.canBeDisabled = data.canBeDisabled;

The setters for canBeEnabled/canBeDisabled are hard to understand,
I think it would be easier to follow by setting _canBeEnabled/_canBeDisabled directly from here.

@@ +1078,5 @@
> +
> +    if (DebuggerServer.isInChildProcess) {
> +      this.messageManager.sendAsyncMessage(this._msgName, { action: "enable" });
> +    } else {
> +      this.walker.a11yService;

A small comment here would be helpful

@@ +1102,5 @@
> +    if (DebuggerServer.isInChildProcess) {
> +      this.messageManager.sendAsyncMessage(this._msgName, { action: "disable" });
> +    } else {
> +      Services.prefs.setIntPref(PREF_ACCESSIBILITY_FORCE_DISABLED, 1);
> +      Services.prefs.setIntPref(PREF_ACCESSIBILITY_FORCE_DISABLED, this.userPref);

Why do you need to set the pref twice?

@@ +1362,5 @@
> +
> +    this.disabling = true;
> +    this.accService = null;
> +    Services.prefs.setIntPref(PREF_ACCESSIBILITY_FORCE_DISABLED, 1);
> +    Services.prefs.setIntPref(PREF_ACCESSIBILITY_FORCE_DISABLED, this.userPref);

Why do you set the pref twice?

@@ +1407,5 @@
> +    }
> +  };
> +}
> +
> +exports.setupParentProcess = setupParentProcess;

It would be great to move setupParentProcess and AccessibilityParent in a dedicated module. That would allow loading only what is strictly necessary in the parent process.
You would need to tweak the call to setupInParentProcess accordingly.

Comment 8

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

Yes complete patch (WIP, that's split up in a number of other patches in their separate bugs including this one) can be found here https://bug1151468.bmoattachments.org/attachment.cgi?id=8949122 . It is used in accessibility-pabel (devtools/client/accessibility)

Comment 9

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

Comment on attachment 8948499 [details] [diff] [review]
1428430 patch v3

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

::: devtools/server/actors/accessibility.js
@@ +231,5 @@
> +      return null;
> +    }
> +
> +    const attachment = domWalker.getNodeFromActor(this.actorID,
> +                                                  ["rawAccessible", "DOMNode"]);

Yes you are right, I can do:

const attachment = domWalker.attachElement(this.rawAccessible.DOMNode);
return attachment ? attachment.node : null;

I still have to attach it because this.rawAccessible.DOMNode is not an actor but an actual DOM node.

@@ +396,5 @@
>      events.on(tabActor, "window-ready", this.onLoad);
>    },
>  
> +  setA11yServiceGetter() {
> +    DevToolsUtils.defineLazyGetter(this, "a11yService", () => {

Accessibility and AccessibleWalker actors are created when the panel starts up. However until the user enables the service via AccessibilityActor.enable, nothing that's related to accessibility service itself starts up. Creating a reference to an accessibility service will trigger all the machinery to start up, which is what we want to avoid.

@@ +397,5 @@
>    },
>  
> +  setA11yServiceGetter() {
> +    DevToolsUtils.defineLazyGetter(this, "a11yService", () => {
> +      Services.obs.addObserver(this, "accessible-event");

That's what happens here. DevToolsUtils.defineLazyGetter calls the getter function once, deletes the getter and sets the property to the value returned here. So it is called only once. The observer is also removed on reset/destroy, where the lazy getter is reset too.

@@ +791,5 @@
> +    if (!this._currentAccessible) {
> +      return;
> +    }
> +
> +    if (this._hoveredAccessible !== this._currentAccessible) {

Sounds good, I will keep one (the reason both were there is because I used highlighter actor pick implementation), hope it's OK I keep the name _currentAccessible and drop _hoveredAccessible, since it's more meaningful in contexts other than onHovered function.

Also, onKey will eventually have keyboard tree navigation (arrows) but it's not part of this iteration.

@@ +797,5 @@
> +      if (bounds) {
> +        this.highlighter.show(DOMNode, bounds);
> +      }
> +
> +      events.emit(this, "picker-accessible-hovered", this._currentAccessible);

Yes these will be used similarly to inspector markup view. In fact preview, pick and hover events functionality would be the same. Since these only fire when the target actually changes, I hope, it should not be much less performant than the markup one.

@@ +877,5 @@
> +  async _findAndAttachAccessible(event) {
> +    let target = event.originalTarget || event.target;
> +    let { node } = this.domWalker.attachElement(target);
> +    let accessible = await this.getAccessibleFor(node);
> +    if (accessible.indexInParent === -1) {

Good catch, I can make it more performant here.

@@ +885,5 @@
> +
> +    const doc = await this.getDocument();
> +    // There is a chance that ancestry lookup can fail if the accessible is in
> +    // the detached subtree. At that point the root accessible object would be
> +    // defunct and accessing it via parent property will throw.

There's a chance that we can hold a reference to an accessible object that has been detached from its parent (separate from DOMNodes and DOM tree). In some cases it does match the DOM but in some it will not, for example when aria-hidden is used on an element.

@@ +985,5 @@
> +    }));
> +  },
> +
> +  get enabled() {
> +    return Services.appinfo.accessibilityEnabled;

Yes that is correct. Accessibility should only be started parent process; that in turn will trigger start of the accessibility services in child processes (via IPC). For clarity each process runs its instance of the accessibility service. So in actor, enabled flag is/has to be in sync with the enabled state of the accessibility service in that process.

@@ +1102,5 @@
> +    if (DebuggerServer.isInChildProcess) {
> +      this.messageManager.sendAsyncMessage(this._msgName, { action: "disable" });
> +    } else {
> +      Services.prefs.setIntPref(PREF_ACCESSIBILITY_FORCE_DISABLED, 1);
> +      Services.prefs.setIntPref(PREF_ACCESSIBILITY_FORCE_DISABLED, this.userPref);

I will add a comment. Essentially when the pref is set to 1. We force disable accessibility service and prevent it from starting up again, that is why it is set back to what user set value. This is the only way to guarantee immediate shutdown of the service.

@@ +1362,5 @@
> +
> +    this.disabling = true;
> +    this.accService = null;
> +    Services.prefs.setIntPref(PREF_ACCESSIBILITY_FORCE_DISABLED, 1);
> +    Services.prefs.setIntPref(PREF_ACCESSIBILITY_FORCE_DISABLED, this.userPref);

See above.

Comment 10

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

Attachment: 1428430 patch v4

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949492 [details] [diff] [review]
1428430 patch v4

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

Thanks for addressing my previous comments, I still have some doubt about DOMNode/domWalker purpose here.

::: devtools/server/actors/accessibility.js
@@ +117,5 @@
> +  try {
> +    let extState = {};
> +    accessible.getState({}, extState);
> +    // extState.value is a bitmask. We are applying bitwise AND to mask out
> +    // irrelelvant states.

irrelelvant => irrelevant

@@ +245,5 @@
> +    }
> +
> +    const attachment = domWalker.attachElement(this.rawAccessible.DOMNode);
> +    return attachment ? attachment.node : null;
> +  },

I think it would help to add a comment on DOMNode saying this is an inspector NodeActor.

@@ +363,5 @@
>        help: this.help,
>        keyboardShortcut: this.keyboardShortcut,
>        childCount: this.childCount,
> +      DOMNode: this.DOMNode && this.DOMNode.form(),
> +      domWalker,

It looks like you are passing data you don't really need here:
* domWalker
I'm not sure you use this anywhere?
* DOMNode
It looks like it is only used for highlightAccessibleForNode, but highlightAccessibleForNode should instead receive an accessible node, and from it you would have access to the DOMNode.
async highlightAccessibleForNode(accessible, options = {}) {
    let bounds = accessible.bounds;	
    if (!bounds) {
      return false;
    }
    return this.highlighter.show(accessible.DOMNode, Object.assign({}, options, bounds));
},

It will transfer less data making it slightly faster, simplify highlightAccessibleForNode and most likely also simplify the callsites on the client side.

@@ +368,5 @@
>        domNodeType: this.domNodeType,
> +      indexInParent: this.indexInParent,
> +      states: this.states,
> +      actions: this.actions,
> +      attributes: this.attributes

I haven't reviewed you usage of all these fields,
but keep in mind you should transfer only what is strictly needed. Any data that aren't displayed/used should be fetched via some method.

@@ +699,5 @@
> +   */
> +  async highlightAccessibleForNode(node, options = {}) {
> +    if (!node) {
> +      return false;
> +    }

As you specified:
   node: Arg(0, "domnode"),
in your spec file, and not:
   node: Arg(0, "nullable:domnode"),

protocol.js ensures that the client code pass a valid domnode object here, so you shouldn't receive a null `node` here.

@@ +739,5 @@
> +  /**
> +   * Check is event handling is allowed.
> +   */
> +  _isEventAllowed: function ({ view }) {
> +    let { window } = this._highlighterEnv;

It would be clearer if you use this.tabActor.window here.

@@ +874,5 @@
> +   * This pick method also focuses the highlighter's target window.
> +   */
> +  pickAndFocus: function () {
> +    this.pick();
> +    this._highlighterEnv.window.focus();

Same here, it would be clearer with:
this.tabActor.window.focus()

@@ +917,5 @@
> +  /**
> +   * Start picker content listeners.
> +   */
> +  _startPickerListeners: function () {
> +    let target = this._highlighterEnv.pageListenerTarget;

Could you use this.tabActor.chromeEventHandler instead?
It should be more generic than pageListenerTarget.
Ideally tools should be using than instead of crafting custom helper like this pageListenerTarget.

@@ +1107,5 @@
> +      // accessibility service shutdown in all processes.
> +      Services.prefs.setIntPref(PREF_ACCESSIBILITY_FORCE_DISABLED, 1);
> +      // Set PREF_ACCESSIBILITY_FORCE_DISABLED back to what user set (or
> +      // default). Otherwise accessibility will never start again.
> +      Services.prefs.setIntPref(PREF_ACCESSIBILITY_FORCE_DISABLED, this.userPref);

This code is still unclear.
* What is the point of setting the pref to 1 twice when userPref is equal to 1?
* What is the point of disabling it (first setIntPref) if we reenable it right after (second setIntPref) when userPref is 0? Or is it that the first setIntPref will really disable it, while the second won't re-enable it, but let a change for it to start up again on some particular user action?

::: devtools/shared/fronts/accessibility.js
@@ +70,4 @@
>  
> +      domWalker = types.getType("domwalker").read(domWalker, this);
> +      return types.getType("domnode").read(this._form.DOMNode, domWalker);
> +    });

Ideally such field would be retrieved via a method and you wouldn't have to do all these cryptic protocol.js code.
But as suggested in the actor file, I don't think you need to expose DOMNode not domWalker to the client.

Comment 12

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

Attachment: 1428430 patch v5

Alright, I tinkered around with the panel front end and got it working with accessible actor implementation where neither domnode nor domwalker are passed between actor/front! The rest of the comments addressed/discussed in IRC.

Comment 13

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

Attachment: 1428430 patch v6

Small modification, completely removed coupling with between dom walker and accessibility walker actors.

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8951362 [details] [diff] [review]
1428430 patch v6

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

Thanks for removing the usage of inspector Node/Walker actors, it looks much more simplier!

It looks good overall, but I don't quite follow your "navigate" listeners.
Otherwise it is mostly suggestion on how to ease the review by writing smaller, more focused patches.
It starts to be quite hard to review this one due to its size.

::: devtools/server/actors/accessibility-parent.js
@@ +180,5 @@
> +    Services.prefs.setIntPref(PREF_ACCESSIBILITY_FORCE_DISABLED, 1);
> +    // Set PREF_ACCESSIBILITY_FORCE_DISABLED back to previous default or user
> +    // set value. This will not start accessibility service until the use
> +    // activates it again. It simply ensures that accessibility service can
> +    // start again (when value is below 1).

s/use/user/

::: devtools/server/actors/accessibility.js
@@ +420,5 @@
> +      events.once(this.tabActor, "navigate", () => {
> +        this.highlighter.destroy();
> +        this.highlighter = CustomHighlighterActor(this, isXUL(this.rootWin) ?
> +          "XULWindowAccessibleHighlighter" : "AccessibleHighlighter");
> +      });

Listening for "navigate" with events.once is surprising,
all actors are listening for this immediately/always, in their constructor, with events.on.
Like what you do with will-navigate and onUnload.
Could you do the same with navigate?
If not, could that be listened by AccessibleHighlighter instead?

Also, why do you have to destroy the highlighter between two pages?
I see that highlighterActor does that, but only when changing between XUL and HTML documents:
https://searchfox.org/mozilla-central/source/devtools/server/actors/highlighters.js#153-157

@@ +586,4 @@
>            // If debugging chrome, wait for top level content document loaded,
>            // otherwise wait for root document loaded.
> +          if (event.DOMNode == this.rootDoc) {
> +            events.emit(this, "document-ready", this.addRef(rawAccessible));

Are you sure there will always be a listener for this event *and* that it will care about having an AccessibleActor instance upfront?

i.e couldn't you pass the rawAccessible to the listener and let it choose when to create the AccessibleActor?

@@ +624,5 @@
>        case EVENT_REORDER:
>          if (accessible) {
> +          accessible.children().forEach(child =>
> +            events.emit(child, "index-in-parent-change", child.indexInParent));
> +          events.emit(accessible, "reorder", rawAccessible.childCount, this);

I have to warn you again, all these events sent over RDP are going to slow down you tool.
I still get the feeling that you are sending too many events over RDP. Here you could send only one event for all updated children instead of one per child.

Also, this patch looks like a list of random modifications. At some point it will be easier to move forward by splitting it up in meaningful independant pieces.

@@ +644,5 @@
> +          events.emit(accessible, "text-change", this);
> +          if (NAME_FROM_SUBTREE_RULE_ROLES.has(rawAccessible.role)) {
> +            events.emit(accessible, "name-change", rawAccessible.name,
> +              event.DOMNode == this.rootDoc ?
> +                undefined : this.getRef(rawAccessible.parent), this);

Same here, you are emitting two events in a row.
Note that the inspector uses a throttle algorithm to gather all updates.
But please do not implement that in this patch. What you can do is keep these new events for another patch/bug where:
* you also modify to frontend to use these new events,
* make a clear case about why you need to change them,
* introduce related unit test

@@ +1057,5 @@
> +    if (!this.enabled || !this.canBeDisabled) {
> +      return;
> +    }
> +
> +    this.disabling = true;

This is really brittle. Could you possibly followup to expose an API to shutdown the a11y service instead of hacking around the pref change to 1?
This makes a significant piece of the actor really cryptic.

@@ +1061,5 @@
> +    this.disabling = true;
> +    let shutdownPromise = this.once("shutdown");
> +    if (this.walker) {
> +      this.walker.reset();
> +    }

You are already calling walker.reset from observe, before emitting "shutdown". Is there any value in calling it here?

@@ +1074,5 @@
> +      Services.prefs.setIntPref(PREF_ACCESSIBILITY_FORCE_DISABLED, 1);
> +      // Set PREF_ACCESSIBILITY_FORCE_DISABLED back to previous default or user
> +      // set value. This will not start accessibility service until the use
> +      // activates it again. It simply ensures that accessibility service can
> +      // start again (when value is below 1).

s/use/user/

@@ +1160,5 @@
> +    });
> +
> +    if (this.walker) {
> +      this.walker.reset();
> +    }

walker.reset should be called when calling disable().
Is there any point in calling it here also?

Comment 15

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

Attachment: 1428430 patch v7

* Got rid of navigate, you're right highlighter target takes care of this.
* Updated document-ready to not create actor if no listeners are present.
* Got rid of redundant reset calls
* Will open a follow up to throttle events

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8953543 [details] [diff] [review]
1428430 patch v7

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

Looks good Yura, thanks for addressing all my comments.

Please remember to submit focused patches next time, this patch was really hard to review as it modifies too many unrelated things at once.

::: devtools/server/actors/accessibility.js
@@ +420,5 @@
> +      if (newRootDoc !== this.rootDoc) {
> +        if (this.rootDoc) {
> +          this.purgeSubtree(this.a11yService.getAccessibleFor(this.rootDoc));
> +        }
> +        this.refMap.clear();

* Wouldn't `newRootDoc !== this.rootDoc` be always true?

* if rootDoc is null, doesn't it mean that the actor is destroy? If that's the case, it would be easier to return early in this method rather than check for rootDoc everywhere.

Comment 17

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

Attachment: 1428430 follow up patch

Comment 18

[Alexandre Poirot [:ochameau]]

Comment on attachment 8954789 [details] [diff] [review]
1428430 follow up patch

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

Thanks for this cleanup.
It looks like you can also simplify the observe method.

::: devtools/server/actors/accessibility.js
@@ +546,5 @@
> +          this.purgeSubtree(rawAccessible);
> +          // If it's a top level document, clear entire cache and notify
> +          // listeners about the document being ready.
> +          if (event.DOMNode == this.rootDoc) {
> +            this.refMap.clear();

I'm wondering if you should move refMap.clear into purgeSubTree,
in many places you call purgeSubTree and then right after check for `if (domNode == rootDoc) refMap.clear()`

Otherwise, could you at least share this code within this function. If I follow you logic correctly, it should be:
// It looks like you don't do anything if isBusy is true,
// so it would be easier to return early like this:
if (isBusy) {
  return;
}
if (rawAccessible instanceof Ci.nsIAccessibleDocument) {
  // Remove its existing cache from tree.
  this.purgeSubtree(rawAccessible);
  // If it's a top level document, clear entire cache.
  if (event.DOMNode == this.rootDoc) {
    this.refMap.clear();
  }
}
Then, followed by:
// Accessible document is recreated.
if (!isEnabled && rawAccessible instanceof Ci.nsIAccessibleDocument) {
  events.emit(this, "document-ready", rawAccessible);
}
// Only propagate state change events for active accessibles.
// Also, here, can `accessible` ever be null?
// If not, we can simplify this if condition.
if (accessible && isEnabled) {
  events.emit(accessible, "states-change", accessible.states);
}

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a638660a49fa
added enable, disable, highlighter, picker a11y functionality. r=pbro, ochameau

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d9813d8cb47
Fix for ESLint faiure in devtools/server/actors/accessibility.js.

Comment 21

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/a638660a49fa
https://hg.mozilla.org/mozilla-central/rev/7d9813d8cb47

Comment 22

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1428430) for devtools failure on /builds/worker/workspace/build/src/accessible/base/nsAccessibilityService.cpp:1045 on a CLOSED TREE

Log of the failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=165347906&repo=mozilla-inbound&lineNumber=20755

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/533de41e4cb3c8356e254f662371c0e852ce18ca

Push that got backout:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a638660a49fa22780b515fb4770742d39c2105bf&filter-searchStr=devtools

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7d9813d8cb47e68706be16f61d4abeac32dfc25e&filter-searchStr=devtools

Comment 23

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/5410375734d6
Backed out 2 changesets for devtools failure on /builds/worker/workspace/build/src/accessible/base/nsAccessibilityService.cpp:1045 a=backout

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26fa0c926ba6
added enable, disable, highlighter, picker a11y functionality. r=pbro, ochameau

Comment 25

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/26fa0c926ba6