Enabled Accessibility Panel by Default.

RESOLVED FIXED in Firefox 63

Status

defect
P1
normal
RESOLVED FIXED
11 months ago
Last month

People

(Reporter: yzen, Assigned: yzen)

Tracking

({dev-doc-complete})

unspecified
Firefox 63
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 12 obsolete attachments)

4.01 KB, patch
Details | Diff | Splinter Review
51.94 KB, image/png
victoria
: ui-review+
Details
106.42 KB, image/png
victoria
: ui-review+
Details
59.64 KB, image/png
victoria
: ui-review+
Details
7.47 KB, patch
Details | Diff | Splinter Review
17.63 KB, patch
flod
: review+
Details | Diff | Splinter Review
19.16 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
Assignee

Description

11 months ago
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.
Assignee

Comment 1

11 months ago
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?
Flags: needinfo?(hkirschner)
Assignee

Updated

11 months ago
Blocks: 1482461
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? :)
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.
> 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
Flags: needinfo?(hkirschner)
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.
Assignee

Comment 6

11 months ago
Yes, sorry all for confusion.
Assignee

Comment 7

10 months ago
Attachment #9001228 - Flags: review?(gl)
Assignee

Comment 8

10 months ago
Posted patch 1482454 - add "new!" bubble (obsolete) — Splinter Review
Attachment #9001243 - Flags: review?(gl)

Updated

10 months ago
Attachment #9001228 - Flags: review?(gl) → review+
Assignee

Updated

10 months ago
Keywords: leave-open
Assignee

Comment 10

10 months ago
Some polish based on Victoria's feedback
Attachment #9001245 - Attachment is obsolete: true
Attachment #9001245 - Flags: review?(gl)
Assignee

Updated

10 months ago
Attachment #9001663 - Flags: review?(gl)

Comment 12

10 months ago
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/744065f01f78
enabled accessibility panel by default. r=gl
Assignee

Comment 13

10 months ago
Posted patch 1482454 - add "New" bubble (obsolete) — Splinter Review
Updated based on Victoria's feedback.
Attachment #9001243 - Attachment is obsolete: true
Attachment #9001243 - Flags: review?(gl)
Attachment #9001709 - Flags: review?(gl)
Assignee

Comment 14

10 months ago
Posted image New bubble
Attachment #9001710 - Flags: ui-review?(victoria)
Assignee

Comment 15

10 months ago
Posted image Startup panel
Attachment #9001711 - Flags: ui-review?(victoria)
Assignee

Comment 16

10 months ago
Posted image Help toolbar icon
Attachment #9001712 - Flags: ui-review?(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.
Attachment #9001710 - Flags: ui-review?(victoria) → ui-review+
Comment on attachment 9001711 [details]
Startup panel

Perfect!
Attachment #9001711 - Flags: ui-review?(victoria) → ui-review+
Comment on attachment 9001712 [details]
Help toolbar icon

Really nice!
Attachment #9001712 - Flags: ui-review?(victoria) → ui-review+
Assignee

Comment 21

10 months ago
Asking for re-review as I had to fix test failures in the inspector (context menu related)
Attachment #9001228 - Attachment is obsolete: true
Flags: needinfo?(yzenevich)
Attachment #9001729 - Flags: review?(gl)
Assignee

Comment 22

10 months ago
Addressed comment 18
Attachment #9001709 - Attachment is obsolete: true
Attachment #9001709 - Flags: review?(gl)
Attachment #9001745 - Flags: review?(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
Attachment #9001729 - Flags: review?(gl) → review+
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 {}.
Attachment #9001664 - Flags: review?(gl) → review+
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
Attachment #9001663 - Flags: review?(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.
Attachment #9001745 - Flags: review?(gl) → review+

Comment 27

10 months ago
Pushed by yura.zenevich@gmail.com:
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
Assignee

Comment 30

10 months ago
Updated to insert Learn more links in a l10n-friendly manner.
Attachment #9001663 - Attachment is obsolete: true
Flags: needinfo?(yzenevich)
Attachment #9002069 - Flags: review?(gl)
Attachment #9002069 - Flags: review?(francesco.lodolo)
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)
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.
Assignee

Comment 33

10 months ago
updated properties file
Attachment #9002069 - Attachment is obsolete: true
Attachment #9002069 - Flags: review?(gl)
Attachment #9002069 - Flags: review?(francesco.lodolo)
Attachment #9002478 - Flags: review?(gl)
Attachment #9002478 - Flags: review?(francesco.lodolo)
Assignee

Comment 34

10 months ago
Sorry wrong patch.
Attachment #9002478 - Attachment is obsolete: true
Attachment #9002478 - Flags: review?(gl)
Attachment #9002478 - Flags: review?(francesco.lodolo)
Attachment #9002548 - Flags: review?(gl)
Attachment #9002548 - Flags: review?(francesco.lodolo)
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.
Attachment #9002548 - Flags: review?(gl) → review+
Attachment #9002548 - Flags: review?(francesco.lodolo) → review+
Assignee

Comment 36

10 months ago
(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.
Assignee

Comment 37

10 months ago
Carrying over :gl's r+ for inspector and prefs. Marking Alex for review of toolbox changes.
Attachment #9001729 - Attachment is obsolete: true
Attachment #9002880 - Flags: review?(poirot.alex)
Assignee

Comment 38

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

Still see failure on try, removing r? for now.
Attachment #9002880 - Flags: review?(poirot.alex)
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

10 months ago
Pushed by yura.zenevich@gmail.com:
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
Assignee

Comment 42

10 months ago
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.
Attachment #9002880 - Attachment is obsolete: true
Attachment #9003619 - Flags: review?(poirot.alex)
Assignee

Comment 44

10 months ago
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.
Attachment #9004306 - Flags: review?(poirot.alex)
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.
Attachment #9003619 - Flags: review?(poirot.alex)
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
(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?
Assignee

Comment 49

10 months ago
(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.
Assignee

Comment 50

10 months ago
(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).
Assignee

Comment 51

10 months ago
Attachment #9003619 - Attachment is obsolete: true
Attachment #9004306 - Attachment is obsolete: true
Attachment #9004306 - Flags: review?(poirot.alex)
Attachment #9005205 - Flags: review?(poirot.alex)
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 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.
Assignee

Comment 56

10 months ago
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).
Assignee

Comment 57

10 months ago
(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 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.
Attachment #9005433 - Flags: review?(poirot.alex) → review+
Assignee

Comment 59

10 months ago
(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

10 months ago
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c3d80c5d22
enabled accessibility panel by default. r=gl, ochameau
Assignee

Updated

10 months ago
See Also: → 1488009
Assignee

Updated

10 months ago
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Keywords: leave-open
Resolution: --- → FIXED
Depends on: 1492265
Assignee

Updated

Last month
See Also: → 1551573
You need to log in before you can comment on or make changes to this bug.