1482454 - Enabled Accessibility Panel by Default.

Status: RESOLVED FIXED

(DevTools :: Accessibility Tools, defect, P1)

Description

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

For Fx63, the panel is going to be enabled by default. For that, several things need to be addressed:

* Flip the preference for the accessibility panel being enabled.
* Consult with Victoria about integrating "New" inside the accessibility tab.
* Determine and set the order of the new accessibility tab.
* Add links to accessibility panel MDN documentation:
  ** "Learn more" link on the startup screen (when the panel is disabled)
  ** (?) bubble in the panel toolbar linking to the MDN doc (when the panel is enabled)
  ** Telemetry for when users open documentation.

Comment 1

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

Harald, Martin mentioned I should ni? you regarding the order of the a11y tab. He mentioned new ones start at the 3rd position. Could you confirm, thanks?

Comment 2

[Victoria Wang [:victoria]]

Excited to see this in the works!

I don't remember exactly what the "New" label looked like — do you mean the one :gl added for Layout panel tab? Yes, that should probably work. I'm assuming it would be a small label to the right of the tab text, which would disappear after you first navigate to the tab.

Two last-minute bonus things I'd love to see happen before this gets a wider audience (I should have mentioned these before!):

1. A more welcoming message on startup - I've had some feedback from other UXers that it sounds too negative, but never got around to working on it. I will ask Content Strategy for some help here. Maybe we could just add one extra paragraph to the beginning, something like: 

"The Accessibility Inspector allows you to examine the current page's accessibility tree, which is used by screenreaders and other assistive technologies. _Learn more._"

2. Could we make it so that the dotted-line focus ring doesn't appear when using a mouse to click on the Properties column? :)

Comment 3

[Victoria Wang [:victoria]]

Here's an idea for the startup message that affects the existing copy as well:

>Accessibility Inspector allows you to examine the current page's accessibility tree, which is used by screenreaders and other assistive technologies. _Learn more._

>When accessibility features are activated, they affect the performance of other Developer Tools panels. You can turn them off at any time to improve performance.

Comment 4

[:Harald Kirschner :digitarald]

> He mentioned new ones start at the 3rd position. Could you confirm, thanks?

I am not sure what that 3rd position means as a starting position for new panels and where it would come from. We have most usage in 4 panels [1], which should probably inform their order.

> * Determine and set the order of the new accessibility tab.

Is there a reason not to have this after Storage? We would re-evaluate after seeing adoption.

>  ** Telemetry for when users open documentation.

Solved by UTM tagged links, no telemetry needed.

[1]: https://sql.telemetry.mozilla.org/queries/54336#143562

Comment 5

[Martin Balfanz [:mbalfanz]]

I didn't say it's a rule for new panels to start at the 3rd position, but I said it would be my personal preference for the order (Inspector, Console, A11y, ...) as this grouping makes most sense to me.  But that's just my opinion :)

Anyways, there is no reason not to have it after storage, and we can always re-evaluate later.

Comment 6

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

Yes, sorry all for confusion.

Comment 7

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

Attachment: 1482454 - Enable panel by default

Comment 8

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

Attachment: 1482454 - add "new!" bubble

Comment 9

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

Attachment: 1482454 - add "learn more"/"help" link to a11y panel, update welcome message

Comment 10

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

Attachment: 1482454 - add "learn more"/"help" link to a11y panel, update welcome message

Some polish based on Victoria's feedback

Comment 11

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

Attachment: 1482454 - upadte focus styling based on Victoria's request.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/744065f01f78
enabled accessibility panel by default. r=gl

Comment 13

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

Attachment: 1482454 - add "New" bubble

Updated based on Victoria's feedback.

Comment 14

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

Attachment: New bubble

Comment 15

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

Attachment: Startup panel

Comment 16

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

Attachment: Help toolbar icon

Comment 17

[Andreea Pavel [:apavel]]

Backed out for failing devtools at devtools/client/inspector/test/browser_inspector_addNode_01.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=744065f01f78c24de3f18276d0bb8d47e4709f4b

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=194345893&repo=mozilla-inbound&lineNumber=4699

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

Comment 18

[Victoria Wang [:victoria]]

Comment on attachment 9001710 [details]
New bubble

Nice! Somehow I just noticed the badge's baseline looks like it might be 1px higher than the tab title's baseline? Would be great to fix that if you can, no further review needed.

Comment 19

[Victoria Wang [:victoria]]

Comment on attachment 9001711 [details]
Startup panel

Perfect!

Comment 20

[Victoria Wang [:victoria]]

Comment on attachment 9001712 [details]
Help toolbar icon

Really nice!

Comment 21

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

Attachment: 1482454 - Enable panel by default

Asking for re-review as I had to fix test failures in the inspector (context menu related)

Comment 22

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

Attachment: 1482454 - add "New" bubble

Addressed comment 18

Comment 23

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 9001729 [details] [diff] [review]
1482454 - Enable panel by default

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

::: devtools/client/inspector/inspector.js
@@ +1697,5 @@
>        click: () => this.showAccessibilityProperties(),
>        disabled: true
>      });
> +    // Accessibility startup component maintains accessibilityFront that is up-to-date
> +    // with state of accessibility service on the server side.

s/with state of/with the state of the

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 9001664 [details] [diff] [review]
1482454 - upadte focus styling based on Victoria's request.

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

::: devtools/client/accessibility/accessibility.css
@@ +10,1 @@
>  :root {

:root {} is actually the default variables for the light theme. I think what you want to do here is set the colors for the dark theme so you should move these into a :root.theme-dark {} and move the .theme-light variables back into :root {}.

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 9001663 [details] [diff] [review]
1482454 - add "learn more"/"help" link to a11y panel, update welcome message

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

The current strings aren't necessarily correct from l10n point of view. You will see what we had to do to integrate a learn more link with a description in the 3 pane onboarding tooltip. Also, please ask for a review from flod about the strings for l10n.

https://bugzilla.mozilla.org/show_bug.cgi?id=1446944
https://searchfox.org/mozilla-central/source/devtools/client/locales/en-US/inspector.properties#480
https://searchfox.org/mozilla-central/source/devtools/client/inspector/shared/three-pane-onboarding-tooltip.js

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 9001745 [details] [diff] [review]
1482454 - add "New" bubble

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

::: devtools/client/framework/components/ToolboxTab.js
@@ +87,5 @@
> +              {
> +                className: "devtools-tab-badge"
> +              },
> +              badge
> +            ) : null

Move null to the next line.

::: devtools/client/themes/toolbox.css
@@ +125,5 @@
> +  font-weight: 500;
> +  margin-inline-start: 5px;
> +}
> +
> +.devtools-tab.selected .devtools-tab-badge {

I don't think we need this since the badge disappears after the tab is highlighted. So, the devtools-tab-badge span is never visible when the tab is selected.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f06260e3db1c
enabled accessibility panel by default. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cdd772dae68
Display a "New" indicator to promote the accessibility panel. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ada76566234
update a11y panel's tree/sidebar keyboard focus styling. r=gl

Comment 28

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (Bug 1482454) for devtools failures CLOSED TREES

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3ada76566234066a9adf2cc1daf8b52121dea27b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&filter-classifiedState=unclassified&filter-searchStr=OS%20X%2010.10%20debug%20Mochitests%20with%20e10s%20test-macosx64%2Fdebug-mochitest-devtools-chrome-e10s-1%20M-e10s(dt1)&selectedJob=194551855

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=194551855&repo=mozilla-inbound&lineNumber=8080

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/7800cf98a6e7d80dbd8cb3c66fdc06020e303c44

Comment 29

[Cosmin Sabou [:CosminS]]

Please also look over this failure: https://treeherder.mozilla.org/logviewer.html#?job_id=194560522&repo=mozilla-inbound&lineNumber=2374

Comment 30

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

Attachment: 1482454 - add "learn more"/"help" link to a11y panel, update welcome message

Updated to insert Learn more links in a l10n-friendly manner.

Comment 31

[Francesco Lodolo [:flod]]

Comment on attachment 9002069 [details] [diff] [review]
1482454 - add "learn more"/"help" link to a11y panel, update welcome message

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

::: devtools/client/locales/en-US/accessibility.properties
@@ +88,5 @@
>  
> +# LOCALIZATION NOTE (accessibility.description.general.p1): A title text for the first
> +# paragraph, used when accessibility service description is provided before accessibility
> +# inspector is enabled.
> +accessibility.description.general.p1=Accessibility Inspector lets you examine the current page's accessibility tree, which is used by screenreaders and other assistive technologies. %S

- Always use proper apostrophes (’) in strings, otherwise tests will fail.
- So far we always wrote 'screen reader", not "screenreader" https://transvision.mozfr.org/?recherche=screen+reader&repo=gecko_strings&sourcelocale=en-US&locale=it&search_type=strings_entities

Also explain in the comment what %S is replaced with (I assume the learn more link)

Comment 32

[Ionuț Goldan [:igoldan]]

When this landed in comment 27, we noticed these many DAMP regressions:

== Change summary for alert #15110 (as of Fri, 17 Aug 2018 11:27:44 GMT) ==

Regressions:

  8%  damp simple.netmonitor.open.DAMP windows10-64 opt e10s stylo     254.75 -> 275.42
  8%  damp complicated.netmonitor.open.DAMP windows10-64 opt e10s stylo270.41 -> 291.89
  7%  damp simple.netmonitor.close.DAMP windows10-64 opt e10s stylo    21.11 -> 22.57
  6%  damp simple.webconsole.open.DAMP windows10-64 opt e10s stylo     314.02 -> 331.61
  4%  damp inspector.layout.open windows10-64 opt e10s stylo           438.26 -> 455.86
  4%  damp simple.styleeditor.open.DAMP windows10-64 opt e10s stylo    233.25 -> 241.90
  4%  damp simple.webconsole.close.DAMP windows10-64 opt e10s stylo    25.24 -> 26.13
  2%  damp simple.jsdebugger.open.DAMP windows10-64 opt e10s stylo     753.58 -> 770.23

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=15110


The backout that followed canceled them.

Comment 33

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

Attachment: 1482454 - add "learn more"/"help" link to a11y panel, update welcome message

updated properties file

Comment 34

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

Attachment: 1482454 - add "learn more"/"help" link to a11y panel, update welcome message

Sorry wrong patch.

Comment 35

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 9002548 [details] [diff] [review]
1482454 - add "learn more"/"help" link to a11y panel, update welcome message

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

::: devtools/client/accessibility/accessibility.css
@@ +110,5 @@
> +  padding: 0;
> +  background-color: var(--theme-body-color);
> +  width: 16px;
> +  height: 16px;
> +  mask: url("chrome://devtools/skin/images/help.svg") no-repeat;

I don't think we typically use mask over background-image. So, I would prefer it we used background-image for this case.

@@ +165,5 @@
>    margin: auto;
>  }
>  
> +.description .link {
> +  color: var(--blue-60);

Did you also consider how this color will look in the dark theme? For reference, this is what I did with my learn more link variables.
https://searchfox.org/mozilla-central/source/devtools/client/themes/tooltips.css#16 which I think you can simply reuse by adding the variables into accessibility.css

@@ +180,5 @@
> +  border-radius: 2px;
> +}
> +
> +.description .link:active {
> +  color: var(--blue-70);

Same as above.

::: devtools/client/accessibility/components/LearnMore.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +// React

This comment doesn't seem necessary since it should be clear from the requires that this is importing React.

@@ +10,5 @@
> +const { p, a } = require("devtools/client/shared/vendor/react-dom-factories");
> +
> +const { openDocLink } = require("devtools/client/shared/link");
> +
> +const openDocOnClick = event => {

Would prefer if this was inside the LearnMore class.

@@ +27,5 @@
> +/**
> + * Localization friendly component for rendering a block of text with a "Learn more" link
> + * as a part of it.
> + */
> +class LearnMore extends Component {

s/LearnMore/LearnMoreLink which I think is more correct naming here.

@@ +31,5 @@
> +class LearnMore extends Component {
> +  static get propTypes() {
> +    return {
> +      href: PropTypes.string,
> +      learnMoreKey: PropTypes.string.isRequired,

s/learnMoreKey/learnMoreStringKey

@@ +33,5 @@
> +    return {
> +      href: PropTypes.string,
> +      learnMoreKey: PropTypes.string.isRequired,
> +      l10n: PropTypes.object.isRequired,
> +      messageKey: PropTypes.string.isRequired,

s/messageKey/messageStringKey

@@ +39,5 @@
> +    };
> +  }
> +
> +  static get defaultProps() {
> +    return defaultProps;

We could simplify this by doing 

return {
 ...
};

That way we don't have to define the defaultProps constant.

@@ +46,5 @@
> +  render() {
> +    const { href, learnMoreKey, l10n, messageKey, onClick } = this.props;
> +    const learnMoreString = l10n.getStr(learnMoreKey);
> +    const messageString = l10n.getFormatStr(messageKey, learnMoreString);
> +    // Split the paragraph string with the link as a separator, and include the link into

Add a new line before this since it begins a new logical block.

@@ +50,5 @@
> +    // Split the paragraph string with the link as a separator, and include the link into
> +    // results.
> +    const re = new RegExp(`(\\b${learnMoreString}\\b)`);
> +    const contents = messageString.split(re);
> +

I think we can remove this line

@@ +52,5 @@
> +    const re = new RegExp(`(\\b${learnMoreString}\\b)`);
> +    const contents = messageString.split(re);
> +
> +    contents[1] = a({ className: "link", href, onClick }, contents[1]);
> +    return (

Add a new line before this.

@@ +58,5 @@
> +    );
> +  }
> +}
> +
> +// Exports from this module

This comment is a bit reductant since it doesn't add any additional information when you read the subsequent line module.exports = LearnMore; 

Would prefer to see this removed.

Comment 36

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

(In reply to Gabriel [:gl] (ΦωΦ) from comment #35)
> Comment on attachment 9002548 [details] [diff] [review]
> 1482454 - add "learn more"/"help" link to a11y panel, update welcome message
> 
> Review of attachment 9002548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/accessibility/accessibility.css
> @@ +110,5 @@
> > +  padding: 0;
> > +  background-color: var(--theme-body-color);
> > +  width: 16px;
> > +  height: 16px;
> > +  mask: url("chrome://devtools/skin/images/help.svg") no-repeat;
> 
> I don't think we typically use mask over background-image. So, I would
> prefer it we used background-image for this case.

I only found one other place where we have a help icon - debugger. I essentially used their implementation for consistency.

Comment 37

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

Attachment: 1482454 - Enable panel by default

Carrying over :gl's r+ for inspector and prefs. Marking Alex for review of toolbox changes.

Comment 38

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

Comment on attachment 9002880 [details] [diff] [review]
1482454 - Enable panel by default

Still see failure on try, removing r? for now.

Comment 39

[Alexandre Poirot [:ochameau]]

Comment on attachment 9002880 [details] [diff] [review]
1482454 - Enable panel by default

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

::: devtools/client/framework/toolbox.js
@@ +553,5 @@
> +      // If in testing environment, wait for tool startups to finish loading, so we don't
> +      // have to explicitly wait for this in tests.
> +      if (flags.testing) {
> +        await toolStartupsConnections;
> +      }

What is the rational to put this here rather than anywhere else?

I think it would be benefitial to have a dedicated method as this function becomes hairy.

Are you sure we don't want to wait in production? Wouldn't that lead to races in production?

Otherwise, I'm wondering if that would be easier to keep the code as-is and instead do, in this method:
  await this._buildTabs();
And in _buildTabs, wait for all _buildPanelForTool calls:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#1134

@@ +1546,4 @@
>      }
> +
> +    this._buildPanelForTool(definition);
> +    await this._loadToolStartup(definition.id);

Why do you modify addAdditionalTool?
It is only used by WebExtension, and A11Y panel is a built-in tool.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80e54dbe8329
Display a "New" indicator to promote the accessibility panel. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb838fd92f0
update a11y panel's tree/sidebar keyboard focus styling. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c44264ed1fe
add learn more links across accessibility panel. r=gl, flod

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/80e54dbe8329
https://hg.mozilla.org/mozilla-central/rev/bcb838fd92f0
https://hg.mozilla.org/mozilla-central/rev/5c44264ed1fe

Comment 42

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

Attachment: 1482454 - Enable panel by default

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e9187a84c0c4e441cbf0661e61d81af173e1ef9

(In reply to Alexandre Poirot [:ochameau] from comment #39)
> Comment on attachment 9002880 [details] [diff] [review]
> 1482454 - Enable panel by default
> 
> Review of attachment 9002880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/framework/toolbox.js
> @@ +553,5 @@
> > +      // If in testing environment, wait for tool startups to finish loading, so we don't
> > +      // have to explicitly wait for this in tests.
> > +      if (flags.testing) {
> > +        await toolStartupsConnections;
> > +      }
> 
> What is the rational to put this here rather than anywhere else?

Originally startup components would need to be build at the time of panels built, in reality we just need to make sure they complete before initial selection is made.

> 
> I think it would be benefitial to have a dedicated method as this function
> becomes hairy.

Done.

> 
> Are you sure we don't want to wait in production? Wouldn't that lead to
> races in production?

Yes this makes sense.

> 
> Otherwise, I'm wondering if that would be easier to keep the code as-is and
> instead do, in this method:
>   await this._buildTabs();
> And in _buildTabs, wait for all _buildPanelForTool calls:

A lot of pathways, especially from within open method, expect buildTabs to run synchronously (mostly related to markup rendering). This would involve a lot more re-ordering and moving things around. I was hesitant at this point to try doing that.

>  
> https://searchfox.org/mozilla s-central/source/devtools/client/framework/
> toolbox.js#1134
> 
> @@ +1546,4 @@
> >      }
> > +
> > +    this._buildPanelForTool(definition);
> > +    await this._loadToolStartup(definition.id);
> 
> Why do you modify addAdditionalTool?
> It is only used by WebExtension, and A11Y panel is a built-in tool.

Removed.

Comment 43

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

Attachment: 1482454 - Enable panel by default (alternative)

Comment 44

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

Comment on attachment 9004306 [details] [diff] [review]
1482454 - Enable panel by default (alternative)

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

Alternative where target would now be responsible for initializing and destroying a11y actor.

Comment 45

[Alexandre Poirot [:ochameau]]

Comment on attachment 9003619 [details] [diff] [review]
1482454 - Enable panel by default

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

::: devtools/client/framework/toolbox.js
@@ +547,5 @@
>          this.component.setCanRender();
>        }, {timeout: 16});
>  
> +      // Finish loading tool startups before selecting a tool panel.
> +      await this._connectToolStartups();

I'm quite concerned about comment 32, reporting significant regression in tools opening time.
Did you fixed anything to limit this regression?
Did you tried pushing this patch to DAMP?
(Following page explains how to do so:
 https://docs.firefox-dev.tools/contributing/performance.html#run-performance-tests)
I'm expecting the regression to be even bigger as the patch you pushed wasn't waiting in production codepath here.

Otherwise, I don't immediately see why you need all these changes.
Wouldn't ` await this.startup.ready;` in panel.js be enough?
This line should delay the ready event for this panel.

@@ +2715,5 @@
>        } else {
>          this.panelDefinitions = this.panelDefinitions.concat(definition);
>        }
>        this._buildPanelForTool(definition);
> +      await this._loadToolStartup(toolId);

I find it confusing to have the tool startup being created into _buildPanelForTool,
and later on, here, right after calling this function, wait for its completion.
It would be easier to follow if buildPanelForTool was async and waiting for `startup.ready`.

Then, do you think that would resolving the comment I made about buildTabs?
If making buildTabs async is an issue, we may at least make it generate a promise:
  this.allTabsReady = Promise.all(definitions.map(definition => this._buildPanelForTool(definition)));
Otherwise, it would be great to know about which code depends on buildTabs being running synchronously.

Finally, if we are able to do that we could await this.allTabsReady; in toolbox.open rather than `await this._connectToolStartups()` which is indirectly related to buildPanelForTools, but in the current state of code it is very hard to know it.

Comment 46

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

Tested the less intrusive patch (the one that still has the r?) https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=7b701ab4a41f850d160522629adce592c496266c&framework=12&showOnlyComparable=1&selectedTimeRange=172800 and it looks fine.

Comment 47

[Alexandre Poirot [:ochameau]]

Comment on attachment 9004306 [details] [diff] [review]
1482454 - Enable panel by default (alternative)

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

This patch looks much better!
I'm only concerned about the impact on toolbox load time.
I'll try to profile you a diff of profiles before/after your patch to see if we can mitigate the regression.
I imagine it is solely related to accessibility actor's setup.

::: devtools/client/inspector/inspector.js
@@ +1698,5 @@
>        disabled: true
>      });
> +    // Accessibility startup component maintains accessibilityFront that is up-to-date
> +    // with the state of the accessibility service on the server side.
> +    const startup = this._toolbox.getToolStartup("accessibility");

I'm wondering if that would be better to fetch the front directly rather than doing it via startup object?
  const accessibilityFront = this.target.getFront("accessibility");

::: devtools/client/netmonitor/test/head.js
@@ +354,5 @@
>      await waitForAllNetworkUpdateEvents();
>      info("All pending requests finished.");
>  
>      const onDestroyed = monitor.once("destroyed");
> +    await monitor.toolbox.destroy();

Oh, that may help fix a bunch of issues.
Out of curiosity, what test(s) was/were failing?

I imagine you can drop:
  const onDestroyed = monitor.once("destroyed");
and
  const onDestroyed = monitor.once("destroyed");
as it looks redundant with:
  await monitor.toolbox.destroy();

Because destroyed is emitted on panel.destroy:
  https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/panel.js#38
And toolbox.destroy waits for this event:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#2890

Comment 48

[Alexandre Poirot [:ochameau]]

(In reply to Yura Zenevich [:yzen] from comment #46)
> Tested the less intrusive patch (the one that still has the r?)
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> central&newProject=try&newRevision=7b701ab4a41f850d160522629adce592c496266c&f
> ramework=12&showOnlyComparable=1&selectedTimeRange=172800 and it looks fine.

The summary value of DAMP isn't reliable. You have to look at subtests:
  https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=7b701ab4a41f850d160522629adce592c496266c&originalSignature=28c4fee1ef7d4e5d184aa628df7c94de208b7678&newSignature=28c4fee1ef7d4e5d184aa628df7c94de208b7678&showOnlyConfident=1&framework=12&selectedTimeRange=172800
  You can still see a regression on tool opening time.

Otherwise here is a profile comparison of cold.inspector.open:
  https://perfht.ml/2ww1tVe
The first profile is mozilla-central and the second is with your latest patch (attachment 9004306 [details] [diff] [review])

If you look for "accessiblity" in the call tree and select either "second profile" or "only second", you will see that there is a slight overhead because of the front loading and also because of the _updatePickerButton function that forces a react render.

Do we have to update the picker button when accessiblity panel isn't opened yet?

Comment 49

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

(In reply to Alexandre Poirot [:ochameau] from comment #47)
> Comment on attachment 9004306 [details] [diff] [review]
> 1482454 - Enable panel by default (alternative)
> 
> Review of attachment 9004306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch looks much better!
> I'm only concerned about the impact on toolbox load time.
> I'll try to profile you a diff of profiles before/after your patch to see if
> we can mitigate the regression.
> I imagine it is solely related to accessibility actor's setup.
> 
> ::: devtools/client/inspector/inspector.js
> @@ +1698,5 @@
> >        disabled: true
> >      });
> > +    // Accessibility startup component maintains accessibilityFront that is up-to-date
> > +    // with the state of the accessibility service on the server side.
> > +    const startup = this._toolbox.getToolStartup("accessibility");
> 
> I'm wondering if that would be better to fetch the front directly rather
> than doing it via startup object?
>   const accessibilityFront = this.target.getFront("accessibility");

Good catch, will fix.

> 
> ::: devtools/client/netmonitor/test/head.js
> @@ +354,5 @@
> >      await waitForAllNetworkUpdateEvents();
> >      info("All pending requests finished.");
> >  
> >      const onDestroyed = monitor.once("destroyed");
> > +    await monitor.toolbox.destroy();
> 
> Oh, that may help fix a bunch of issues.
> Out of curiosity, what test(s) was/were failing?

devtools/client/netmonitor/test/browser_net_telemetry_throttle_changed.js was leaking in debug because the test would shut down with unresolved startup initialization. Unrelatedly, it was also erroring on some emulation calls not completing because target was being destroyed already.

> 
> I imagine you can drop:
>   const onDestroyed = monitor.once("destroyed");
> and
>   const onDestroyed = monitor.once("destroyed");
> as it looks redundant with:
>   await monitor.toolbox.destroy();
> 
> Because destroyed is emitted on panel.destroy:
>  
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/
> panel.js#38
> And toolbox.destroy waits for this event:
>  
> https://searchfox.org/mozilla-central/source/devtools/client/framework/
> toolbox.js#2890

Yep, will remove that.

Comment 50

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

(In reply to Alexandre Poirot [:ochameau] from comment #48)
> (In reply to Yura Zenevich [:yzen] from comment #46)
> > Tested the less intrusive patch (the one that still has the r?)
> > https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> > central&newProject=try&newRevision=7b701ab4a41f850d160522629adce592c496266c&f
> > ramework=12&showOnlyComparable=1&selectedTimeRange=172800 and it looks fine.
> 
> The summary value of DAMP isn't reliable. You have to look at subtests:
>  
> https://treeherder.mozilla.org/perf.html#/
> comparesubtest?originalProject=mozilla-
> central&newProject=try&newRevision=7b701ab4a41f850d160522629adce592c496266c&o
> riginalSignature=28c4fee1ef7d4e5d184aa628df7c94de208b7678&newSignature=28c4fe
> e1ef7d4e5d184aa628df7c94de208b7678&showOnlyConfident=1&framework=12&selectedT
> imeRange=172800
>   You can still see a regression on tool opening time.
> 
> Otherwise here is a profile comparison of cold.inspector.open:
>   https://perfht.ml/2ww1tVe
> The first profile is mozilla-central and the second is with your latest
> patch (attachment 9004306 [details] [diff] [review])
> 
> If you look for "accessiblity" in the call tree and select either "second
> profile" or "only second", you will see that there is a slight overhead
> because of the front loading and also because of the _updatePickerButton
> function that forces a react render.
> 
> Do we have to update the picker button when accessiblity panel isn't opened
> yet?

Yep, see it now, you're right, picker button update does not need to happen in startup initialization. Only when panel is actually selected (which updates the picker item in any case).

Comment 51

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

Attachment: 1482454 - Enable panel by default v2

Comment 52

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

And here's the try with perf jobs:

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd8169a7aee2e9590f448f63a031cd34b717ffa7

And prefhereder:
 
 https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=cd8169a7aee2e9590f448f63a031cd34b717ffa7

Comment 53

[Alexandre Poirot [:ochameau]]

There is still a visible regression, but no more on cold opening, it seems to be more on warn opening. cold is when you open toolbox for the first time while firefox is started versus warn is when it has already been opened against a previous tab.

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=cd8169a7aee2e9590f448f63a031cd34b717ffa7&originalSignature=28c4fee1ef7d4e5d184aa628df7c94de208b7678&newSignature=28c4fee1ef7d4e5d184aa628df7c94de208b7678&showOnlyConfident=1&framework=12&selectedTimeRange=172800

Comment 54

[Alexandre Poirot [:ochameau]]

Comment on attachment 9005205 [details] [diff] [review]
1482454 - Enable panel by default v2

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

Performance may be good enough if we remove the buttons picker react update.
Could you address that and re-push to try to confirm this?

If that's the case, I think your patch is good to land!

::: devtools/client/accessibility/accessibility-startup.js
@@ +16,5 @@
>  class AccessibilityStartup {
>    constructor(toolbox) {
>      this.toolbox = toolbox;
>  
> +    this._updateAccessibilityToolHighlight =

You may rename this to `_updateToolHighlight` as we already know this will be about accessibility given the module name.

@@ +67,5 @@
> +
> +        this._accessibility.on("init", this._updateAccessibilityToolHighlight);
> +        this._accessibility.on("shutdown", this._updateAccessibilityToolHighlight);
> +        this._accessibility.on("init", this._updatePickerButton);
> +        this._accessibility.on("shutdown", this._updatePickerButton);

> Yep, see it now, you're right, picker button update does not need to happen in startup initialization. Only when panel is actually selected (which updates the picker item in any case).

So, I'm confused. Can't you remove these two lines and let the panel call toolbox.updatePickerButton?
i.e. move these two lines here:
  https://searchfox.org/mozilla-central/source/devtools/client/accessibility/panel.js#93
  and call toolbox.updatePickerButton from panel.js
?

In term of performance it would save a react render during toolbox opening and only update the picker if we actually open the a11y panel.

Comment 55

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

Attachment: 1482454 - Enable panel by default v3

Another try: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=621425d95a6a125f7200f84c96f9d85f032e1a4c

https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=621425d95a6a125f7200f84c96f9d85f032e1a4c

Comment 56

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

Hopefully I got this right: https://perfht.ml/2NzGOqr
There's still (an expected) call to _updateToolHighlight. I could also check if the tool is already highlighted/unhilighted and not call setState in toolbox controller (via highlightTool/unhighlightTool).

Comment 57

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

(In reply to Yura Zenevich [:yzen] from comment #56)
> Hopefully I got this right: https://perfht.ml/2NzGOqr
> There's still (an expected) call to _updateToolHighlight. I could also check
> if the tool is already highlighted/unhilighted and not call setState in
> toolbox controller (via highlightTool/unhighlightTool).

Just to clarify, first profile is the one with the patch.

Comment 58

[Alexandre Poirot [:ochameau]]

Comment on attachment 9005433 [details] [diff] [review]
1482454 - Enable panel by default v3

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

(In reply to Yura Zenevich [:yzen] from comment #56)
> Hopefully I got this right: https://perfht.ml/2NzGOqr
> There's still (an expected) call to _updateToolHighlight. I could also check
> if the tool is already highlighted/unhilighted and not call setState in
> toolbox controller (via highlightTool/unhighlightTool).

Whaa you managed to use the script, that's fantastic :)
Against which marker/test did you compared the two runs?
(complicated.netmonitor.open.DAMP is a good one to use)

DAMP seems to still report some regression:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=621425d95a6a125f7200f84c96f9d85f032e1a4c&originalSignature=28c4fee1ef7d4e5d184aa628df7c94de208b7678&newSignature=28c4fee1ef7d4e5d184aa628df7c94de208b7678&showOnlyConfident=1&framework=12&selectedTimeRange=172800

I thought that react's render was the main regression, but it looks like it is not.
Hopefully, a second tweak to (un)highlight could bring this regression down.

Otherwise, the only alternative to avoid the regression would be to remove this highlight/unhighlight feature:
* remove a11y startup so that we avoid forcing a11y actor load,
* instead, the a11y panel header button state is only updated if you open the tool at least once.
* note that we can make the a11y button unhighlighted by default and only update its state when you open the panel. I imagine in 99% of cases, the a11y will be disabled by default except for people managing to enable it outside of devtools. (Is that possible?)

Having said all that, I think it is reasonable to land this patch (could you at least tweak _updateToolHighlight/highlightTool/unhighlightTool to avoid the setState) and look at improving performance in a followup. We should just be careful to uplift perf fixes to FF63.

Comment 59

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

(In reply to Alexandre Poirot [:ochameau] PTO back on 17th from comment #58)
> Comment on attachment 9005433 [details] [diff] [review]
> 1482454 - Enable panel by default v3
> 
> Review of attachment 9005433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Yura Zenevich [:yzen] from comment #56)
> > Hopefully I got this right: https://perfht.ml/2NzGOqr
> > There's still (an expected) call to _updateToolHighlight. I could also check
> > if the tool is already highlighted/unhilighted and not call setState in
> > toolbox controller (via highlightTool/unhighlightTool).
> 
> Whaa you managed to use the script, that's fantastic :)
> Against which marker/test did you compared the two runs?
> (complicated.netmonitor.open.DAMP is a good one to use)
> 
> DAMP seems to still report some regression:
> https://treeherder.mozilla.org/perf.html#/
> comparesubtest?originalProject=mozilla-
> central&newProject=try&newRevision=621425d95a6a125f7200f84c96f9d85f032e1a4c&o
> riginalSignature=28c4fee1ef7d4e5d184aa628df7c94de208b7678&newSignature=28c4fe
> e1ef7d4e5d184aa628df7c94de208b7678&showOnlyConfident=1&framework=12&selectedT
> imeRange=172800
> 
> I thought that react's render was the main regression, but it looks like it
> is not.
> Hopefully, a second tweak to (un)highlight could bring this regression down.
> 
> Otherwise, the only alternative to avoid the regression would be to remove
> this highlight/unhighlight feature:
> * remove a11y startup so that we avoid forcing a11y actor load,
> * instead, the a11y panel header button state is only updated if you open
> the tool at least once.
> * note that we can make the a11y button unhighlighted by default and only
> update its state when you open the panel. I imagine in 99% of cases, the
> a11y will be disabled by default except for people managing to enable it
> outside of devtools. (Is that possible?)

To clarify, platform accessibility service can be enabled to users of assistive technologies that use firefox or users of other software that use accessibility API on a given platform. All in all about 7% of our users. The idea behind highlighted state (cross tab especially) was to let the users know that if a11y is started in one of the tabs, it's started for the whole browser and all tabs are effected (even if a11y is not the current panel in other tabs).

> 
> Having said all that, I think it is reasonable to land this patch (could you
> at least tweak _updateToolHighlight/highlightTool/unhighlightTool to avoid
> the setState) and look at improving performance in a followup. We should
> just be careful to uplift perf fixes to FF63.

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c3d80c5d22
enabled accessibility panel by default. r=gl, ochameau

Comment 61

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a0c3d80c5d22

Comment 62

[Chris Mills [:cmills]]

Added to the Fx63 rel notes:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63