1453093 - Highlight/Un-highlight tools if necessary when toolbox opens.

Status: VERIFIED FIXED

(DevTools :: General, enhancement, P2)

Description

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

Right now toolbox's highlightTool and unhighlightTool methods are generally fired when panel is already open and another panel is being selected. There is however a situation where toolbox is newly opened and there should be a panel tab highlighted right away - for example, accessibility panel, where a11y service is already running (e.g. if the user has a screen reader on).

Comment 2

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

See STR in bug 1450696

Comment 3

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

Attachment: 1453093 patch

This is something I was thinking about, it also fixes bug 1455619

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8980045 [details] [diff] [review]
1453093 patch

There's some framework layer code added to the toolbox that I'd prefer Alex to review.
So transferring the review to him.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8980045 [details] [diff] [review]
1453093 patch

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

Thanks Yura, sorry again for the delay. It took me some time to really follow this patch.

Please see my comment about breaking remote debugging.

::: devtools/client/accessibility/accessibility-panel-highlighter.js
@@ +15,5 @@
> +    this.onPanelDestroy = this.onPanelDestroy.bind(this);
> +    this.toggleHighlight = this.toggleHighlight.bind(this);
> +
> +    this.toolbox.on(`${this.toolID}-ready`, this.onPanelReady);
> +    this.toolbox.on(`${this.toolID}-destroy`, this.onPanelDestroy);

onPanelDestroy looks redundant with the call to destroy.
Aren't they both called at the same time? from _toolUnregistered?

@@ +16,5 @@
> +    this.toggleHighlight = this.toggleHighlight.bind(this);
> +
> +    this.toolbox.on(`${this.toolID}-ready`, this.onPanelReady);
> +    this.toolbox.on(`${this.toolID}-destroy`, this.onPanelDestroy);
> +    Services.obs.addObserver(this, "a11y-init-or-shutdown");

By doing that, you are breaking remote debugging.
This will only work for debugging webpages from firefox, but it will misbehave from the browser toolbox, and for debugging remote devices.

Unfortunately, I think you have to instanciate the front to properly support this feature.
Hopefully, starting the a11y actor is cheap enough?
(You will again hit bug 1222047, which would make it way easier to start front for any given actor)

But if you want to keep things simple, you may put very bold comment saying it is only working on firefox desktop and keep the existing code in ui.js and here, in this class, only listen for a11y-init-or-shutdown event. The class will be much shorter.

@@ +46,5 @@
> +   * Toggle panel tab's highlighting if necessary.
> +   */
> +  toggleHighlight() {
> +    if (this.needsHighlight) {
> +      this.emit("highlight-tool");

I'm not sure PanelHighlighter class is any useful.
Here you could do:
  this.toolbox.highlightTool(this.toolId);

::: devtools/client/framework/toolbox.js
@@ +114,5 @@
>  
>    this._toolPanels = new Map();
>    this._inspectorExtensionSidebars = new Map();
>  
> +  this._panelHighlighters = new Map();

Please add a comment describing this Map, like frameMap at line 125.

@@ +1441,5 @@
>      deck.appendChild(panel);
>  
> +    if (toolDefinition.buildPanelHighlighter && !this._panelHighlighters.has(id)) {
> +      this._panelHighlighters.set(id, toolDefinition.buildPanelHighlighter(id, this));
> +    }

This is a nice addition, but:
* "highlighter" is a word already used in the inspector and refers to something really different. Highlighters are the actors bits that implements the node picker, rulers, measure, css grid, ...
* this API, with the current naming, looks specific to your precise usecase, but it should not. It looks like a nice generic API allowing to execute panel-specific code, before the panel is started.

So. I think this will be perfect if you keep this as-is but only rename all occurences of Highlighter to something more generic. What about "Startup"? buildPanelStartup, _panelStartups, ...?
Also I'm wondering if it would be better to use "Tool" instead of "Panel". It would better align with loadTool and unloadTool.

@@ +2537,5 @@
> +    }
> +
> +    let panelHighlighter = this._panelHighlighters.get(toolId);
> +    panelHighlighter.destroy();
> +    panelHighlighter = null;

I'm not sur this line is any useful.

Comment 6

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

Attachment: 1453093 part 1

Comment 7

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

Attachment: 1453093 part 2

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983158 [details] [diff] [review]
1453093 part 2

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

Thanks!

This patch looks good, I just have a concern with the usage of test-actor-registry.js.
That's the only thing that prevents r+ this patch right away.

::: devtools/client/accessibility/test/browser/browser_accessibility_panel_highlighter_multi_tab.js
@@ +14,5 @@
> +  await toggleAccessibility(options);
> +  await toggleAccessibility(options);
> +
> +  await checkHighlighted(toolbox1, true);
> +  await checkHighlighted(toolbox2, true);

To ease debugging this test, you may want to assert the state before the toggles and between the two.

::: devtools/client/accessibility/test/browser/head.js
@@ +16,5 @@
>    this);
>  
> +Services.scriptloader.loadSubScript(
> +  "chrome://mochitests/content/browser/devtools/client/shared/test/test-actor-registry.js",
> +  this);

Why are you importing this?

We would like to get rid of this actor registry (bug 1449622), so it would be great to not add yet another callsite for it.

@@ +48,5 @@
> +  });
> +
> +  let a11yService = Cc["@mozilla.org/accessibilityService;1"].getService(
> +    Ci.nsIAccessibilityService);
> +  await initPromise;

It looks like this code will timeout if a previous test leave the service up and running, right?
Can we wait for initPromise only if the service isn't already up and running? or assert that the service isn't already running to have an explicit error rather than a timeout.

@@ +109,5 @@
> +  let enableButton = doc.getElementById("accessibility-enable-button");
> +  // If enable button is not found, asume the tool is already enabled.
> +  if (enableButton) {
> +    EventUtils.sendMouseEvent({ type: "click" }, enableButton, win);
> +  }

It would help debugging tests by asserting that it is enabled in a `else` branch. Or is this what the code right after in waitUntilState already does?

::: devtools/client/framework/toolbox.js
@@ +2622,5 @@
> +    }
> +
> +    const startup = this._toolStartups.get(toolId);
> +    await startup.destroy();
> +    this._toolStartups.delete(toolId);

It might be slightly more solid if you remove it from the Map before calling destroy. It avoid most likely inexistant race where calling unloadToolStartup for the same tool twice, will call start.destroy twice.

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983157 [details] [diff] [review]
1453093 part 1

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

Thanks for this new patch.
Looks good overall, but I would like to see the code moved from toolbox to startup module before giving final r+.

::: devtools/client/framework/toolbox.js
@@ +2670,5 @@
> +      this._initAccessibility = (async function() {
> +        this._accessibility = new AccessibilityFront(this._target.client,
> +                                                     this._target.form);
> +        await this._accessibility.getWalker();
> +        if (!("_supportsLatestAccessibility" in this)) {

I'm not sure this check is useful as _initAccessibility already prevents calling this function twice.

@@ +2673,5 @@
> +        await this._accessibility.getWalker();
> +        if (!("_supportsLatestAccessibility" in this)) {
> +          this._supportsLatestAccessibility =
> +            await this._target.actorHasMethod("accessibility", "enable");
> +        }

When you add such backward compatibility like this, it is handy to put a comment mentioning the first Firefox version supporting the new actor method.
It helps cleaning up the codebase later from very old compatibility code like this.

@@ +2695,5 @@
> +  },
> +
> +  _updateAccessibilityToolHighlight: async function() {
> +    const definition = this.getToolDefinition("accessibility");
> +    if (!definition || !definition.isTargetSupported(this._target)) {

Can `definition` ever be null here?

@@ +2699,5 @@
> +    if (!definition || !definition.isTargetSupported(this._target)) {
> +      return;
> +    }
> +
> +    await this.initAccessibility();

I don't quite follow why we need this wait.
_updateAccessibilityToolHighlight is called by _updateAccessibilityState, itself being called at the end of initAccessibility. So accessibility should already be initialized here?

@@ +2705,5 @@
> +      this.highlightTool("accessibility");
> +    } else {
> +      this.unhighlightTool("accessibility");
> +    }
> +  },

The idea behind making buildPanelHighlighter more generic was an opportunity to prevent polluting Toolbox class with any more panel specifics like these three methods.

So, here I would suggest to more these methods to startup.js and create/store the front there.
It means that panel.js should then be able to get the startup instance via toolbox class (in order to retrieve the front). But I think it makes sense, right?

::: devtools/shared/fronts/accessibility.js
@@ +190,5 @@
> +      return walker;
> +    });
> +  }, {
> +    impl: "_getWalker"
> +  }),

Why do you need such custom method for getWalker?

Comment 10

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

(In reply to Alexandre Poirot [:ochameau] from comment #9)
> Comment on attachment 8983157 [details] [diff] [review]
> 1453093 part 1
> ::: devtools/shared/fronts/accessibility.js
> @@ +190,5 @@
> > +      return walker;
> > +    });
> > +  }, {
> > +    impl: "_getWalker"
> > +  }),
> 
> Why do you need such custom method for getWalker?

Perhaps you might have a better idea. Essentially I need to call a method on the actor in order actorHasMethod check (for backward compatibility) work in the browser toolbox. From what I understand it's related to restrictions for target's getActorDescription method.

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983158 [details] [diff] [review]
1453093 part 2

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

::: devtools/client/accessibility/test/browser/head.js
@@ +16,5 @@
>    this);
>  
> +Services.scriptloader.loadSubScript(
> +  "chrome://mochitests/content/browser/devtools/client/shared/test/test-actor-registry.js",
> +  this);

Thanks for your comment on irc.
Yes, openInspector is depending on this.

Actually the issue is that this loadSubScript here in head.js:
https://searchfox.org/mozilla-central/source/devtools/client/inspector/test/head.js#21-24
Should be moved to shared-head.js (that implements openInspector *and* is imported from head.js)
https://searchfox.org/mozilla-central/source/devtools/client/inspector/test/shared-head.js

Also move this line to make eslint happy:
https://searchfox.org/mozilla-central/source/devtools/client/inspector/test/head.js#7

(please address that in a distinct changeset in this bug.)

Comment 12

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

(In reply to Alexandre Poirot [:ochameau] from comment #8)
> Comment on attachment 8983158 [details] [diff] [review]
> 1453093 part 2
> 
> Review of attachment 8983158 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +109,5 @@
> > +  let enableButton = doc.getElementById("accessibility-enable-button");
> > +  // If enable button is not found, asume the tool is already enabled.
> > +  if (enableButton) {
> > +    EventUtils.sendMouseEvent({ type: "click" }, enableButton, win);
> > +  }
> 
> It would help debugging tests by asserting that it is enabled in a `else`
> branch. Or is this what the code right after in waitUntilState already does?

Yes waitUntilState only resolves if it is enabled.

Comment 13

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

Attachment: 1453093 patch v3

I had to squash both patches together since toolbox functionality is moved over into startup. Hopefully all comments are addressed at this point.

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8984218 [details] [diff] [review]
1453093 patch v3

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

Thanks Yura, it looks good.

The addition of Startup class should help us simplify existing codebase, I'm really happy of your work here :)

::: devtools/client/accessibility/accessibility-panel.js
@@ +190,5 @@
>      this.picker && this.picker.stop();
>    },
>  
> +  get startup() {
> +    return this._toolbox.getToolStartup("accessibility");

Note that you may pass the startup object to AccessibilityPanel's constructor rather than fetching it like that.
Feel free to do that in a followup.

::: devtools/client/accessibility/accessibility-startup.js
@@ +49,5 @@
> +                                                     this.target.form);
> +        this._walker = await this._accessibility.getWalker();
> +        // Only works with FF61+ targets
> +        this._supportsLatestAccessibility =
> +          await this.target.actorHasMethod("accessibility", "enable");

So calling getWalker before actorHasMethod is enough to ensure actorHasMethod works as expected?
If yes, I think it would be super useful to add a comment before the call to getWalker.

@@ +112,5 @@
> +  _updateAccessibilityToolHighlight() {
> +    const definition = this.toolbox.getToolDefinition("accessibility");
> +    if (!definition.isTargetSupported(this.target)) {
> +      return;
> +    }

My comprehension is that AccessibilityStartup won't be instanciated if the target is not supported.
Actually, it is important to ensure it isn't to save ressources.
It should be thanks to this code:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#1412-1414

So this isTargetSupported check should be unnecessary.

::: devtools/client/accessibility/test/browser/head.js
@@ +16,5 @@
>    this);
>  
> +Services.scriptloader.loadSubScript(
> +  "chrome://mochitests/content/browser/devtools/client/shared/test/test-actor-registry.js",
> +  this);

Please have a look at comment 11, I think this should be moved to inspector/test/shared-head.js.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/621b7afe0d4a
move accessible actor instantiation and panel tab highlighting into tool startup component. r=ochameau

Comment 16

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

Comment on attachment 8984218 [details] [diff] [review]
1453093 patch v3

[Feature/Bug causing the regression]: Not a regression, another high priority bug fix for a11y panel identified by QA
[User impact if declined]: Devtools tab highlighter is not going to work correctly, a11y picker will have cases where it's in the incorrect state, basically this bug + bug 1455619 + bug 1466883
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet, but was verified by QA (try build)
[Needs manual test from QE? If yes, steps to reproduce]: yes, see descriptions for bug 1455619 and bug 1466883
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Not too risky
[Why is the change risky/not risky?]: really only affects the a11y devtools panel, covered by tests
[String changes made/needed]: None

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/621b7afe0d4a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8984218 [details] [diff] [review]
1453093 patch v3

High-priority fix for the a11y inspector shipping in 61 (though off by default). Approved for 61.0b13.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This needs a rebased patch for uplift.

Comment 20

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

Attachment: 1453093 beta patch

Here's the rebased patch, thanks, Ryan.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5b5e620e33ee59c1820d1bef687f015d63c2185c

Comment 22

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed due to Bug 1466883  and Bug 1455619 being verified and the steps mentioned in Comment 2 are no longer reproducible. This was tested on Windows 10 64bit, macOS 10.10.5 and Ubuntu 16.04 64bit.