David Bolter [:davidb] (NeedInfo me for attention) Reporter
	Description • 9 years ago

I thought we had a bug filed for this? (Please CLOSE-DUPE if so)

If you found this bug and want this, you can vote here: https://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/5892131-inspect-accessibility-information

We have hopes and plans for activity here in the coming months. Eitan is point.

Who does Ux for devtools?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 1 • 9 years ago

I am aware of some bugs to include the screen reader simulator (bug 907737, bug 1090288), but I am guessing that's not what you mean here.

As for UX... we don't really have anyone at the moment. :( Hoping that will improve soon.  For now Stephen Horlander might be a good contact, but I believe he may be busy with other projects.

	Eitan Isaacson [:eeejay]
	Comment 2 • 9 years ago

Here are some user stories and mockups:
https://wiki.mozilla.org/Accessibility/DevTools

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 3 • 8 years ago

This is a mockup of what would be nice as the first pass. It would be an additional sidebar panel (similar to style, box model, etc) and the properties would correspond to current markup view selection.

	Eitan Isaacson [:eeejay]
	Comment 4 • 8 years ago

(In reply to Yura Zenevich [:yzen] from comment #3)
> Created attachment 8754936 [details]
> Mockup of the UI
> 
> This is a mockup of what would be nice as the first pass. It would be an
> additional sidebar panel (similar to style, box model, etc) and the
> properties would correspond to current markup view selection.

Looks good! Some feedback:
 - Calculated role and name should be on top as a header and not buried in a list.
 - The aria attributes are not interesting, IMHO. Those are readily visible in the inspector. A user would want to look at that panel to see what the *outcome* of setting certain DOM attributes (aria and others).

Here is a list of things I would find useful to display, this doesn't all need to be supported initially:
 - Accessible description
 - Accessible attributes (key/value).
 - Accessible states
 - Accessible actions
 - Accessible relations
 - Accessible bounds (not a priority, imho)
 - Accessible group position (level, set size, position in set)
 - Accessible parent (if explicitly set with aria-owns)
 - nsIAccessibleValue attributes if supported
 - nsIAccessibleTable attributes if supported
 - Maybe text interfaces?

	David Bolter [:davidb] (NeedInfo me for attention) Reporter
	Comment 5 • 8 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #4)
> (In reply to Yura Zenevich [:yzen] from comment #3)
> > Created attachment 8754936 [details]

>  - The aria attributes are not interesting, IMHO. Those are readily visible
> in the inspector. A user would want to look at that panel to see what the
> *outcome* of setting certain DOM attributes (aria and others).

Would this be the same as "computed values" or not quite?

Also, let's get hholmes involved sooner than later.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 6 • 8 years ago

(In reply to David Bolter [:davidb] from comment #5)
> (In reply to Eitan Isaacson [:eeejay] from comment #4)
> > (In reply to Yura Zenevich [:yzen] from comment #3)
> > > Created attachment 8754936 [details]
> 
> >  - The aria attributes are not interesting, IMHO. Those are readily visible
> > in the inspector. A user would want to look at that panel to see what the
> > *outcome* of setting certain DOM attributes (aria and others).
> 
> Would this be the same as "computed values" or not quite?
> 
> Also, let's get hholmes involved sooner than later.

Yes and yes (she's f? for the mockup)

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 8 • 8 years ago

Comment on attachment 8754936 [details]
Mockup of the UI

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #7)
> Created attachment 8756355 [details]
> debugger-accordion.png

This is a screenshot of where we're thinking of going with the Debugger for accordions—I think the style will work here too. (If you want to copy + paste you can grab my design files here: https://github.com/helenvholmes/design-stuff/blob/master/debugger.sketch)

I think it might also be useful to bring up Bug 1272148 + Bug 1258390, which are inline editors for the Rules view. The whole point is to surface functionality which users might not otherwise be familiar with. (I think of this OpenType playground as a really great example of that: http://clagnut.com/sandbox/css3/) If we were to do something similar in this panel and surface potentially relevant attributes to add to something to make it more accessible, what do you think that would look like for this panel that you've designed so far? Would you do warnings for things users might consider adding that aren't currently there?

	Nancy Pang [:npang]
	Comment 9 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/67420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67420/

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 10 • 8 years ago
mozreview-review

Comment on attachment 8775154 [details]
Bug 1151468 - Add accessibility inspection functionality to our DevTools

https://reviewboard.mozilla.org/r/67420/#review71768

Removing for now as this needs to be rebased. We also need to figure out the reason why it does not work in browser toolbox.

::: devtools/client/inspector/inspector-panel.js:83
(Diff revision 1)
>   * - computed-view-property-collapsed
>   *      Fired when a property is collapsed in the computed rules view
>   * - computed-view-sourcelinks-updated
>   *      Fired when the stylesheet source links have been updated (when switching
>   *      to source-mapped files)
> +* - accessibility-view-property-expanded

nit: indentation

::: devtools/client/inspector/inspector-panel.js:442
(Diff revision 1)
>      this.sidebar.addExistingTab(
>        "ruleview",
>        strings.GetStringFromName("inspector.sidebar.ruleViewTitle"),
>        defaultTab == "ruleview");
>  
>      this.sidebar.addExistingTab(

So as we talked earlier we should have the tab under pref, I think the pref should be visible as a checkbox in devtools settings under the Inspector section. Similar to how devtools.inspector.showUserAgentStyles pref is implemented.

::: devtools/client/inspector/inspector-panel.js:460
(Diff revision 1)
>  
>      this.sidebar.on("select", this._setDefaultSidebar);
>  
>      this.ruleview = new RuleViewTool(this, this.panelWin);
>      this.computedview = new ComputedViewTool(this, this.panelWin);
> +    this.accessibilityview = new AccessibilityViewTool(this, this.panelWin);

Same here regarding the initialization under pref.
The additiona and tool creation should be grouped together, I would say.

::: devtools/client/inspector/inspector.xul:111
(Diff revision 1)
> +                        placeholder="&filterPropPlaceholder;"/>
> +            <html:button id="accessibilityview-searchinput-clear" class="devtools-searchinput-clear"></html:button>
> +          </html:div>
> +        </html:div>
> +
> +        <html:div id="accessibilityview-container" class="accessibilityview">

Is class "accessibilityview" used anywhere? If not, we should remove it.

::: devtools/client/inspector/inspector.xul:114
(Diff revision 1)
> +        </html:div>
> +
> +        <html:div id="accessibilityview-container" class="accessibilityview">
> +          <html:div id="accessibility-container-focusable" tabindex="-1">
> +            <html:div id="accessibilityPropertyContainer"></html:div>
> +            <html:div id="accessibilityNoResults" hidden="">

nit: hidden="" -> hidden

::: devtools/client/locales/en-US/styleinspector.dtd:20
(Diff revision 1)
>  
>  <!-- LOCALIZATION NOTE (filterStylesPlaceholder): This is the placeholder that goes in
>    -  the search box when no search term has been entered. -->
>  <!ENTITY filterStylesPlaceholder      "Filter Styles">
>  
> +<!-- LOCALIZATION NOTE (filterPropPlaceholder): This is the placeholder that goes in

I would also mention the loaction of the search box (e.g. accessibility properties search term)

::: devtools/client/locales/en-US/styleinspector.dtd:42
(Diff revision 1)
>  <!ENTITY togglePseudoClassPanel  "Toggle pseudo-classes">
>  
> +<!-- LOCALIZATION NOTE (noAccessibilityPropertiesFound): In the case where there are no
> +  -  Accesibility properties to display e.g. due to search criteria this message is
> +  -  displayed. -->
> +<!ENTITY noAccessibilityPropertiesFound     "No Accessibility properties found.">

maybe properties -> information?

::: devtools/client/locales/en-US/styleinspector.dtd:50
(Diff revision 1)
>    -  properties to display e.g. due to search criteria this message is
>    -  displayed. -->
>  <!ENTITY noPropertiesFound     "No CSS properties found.">
> +
> +<!-- FIXME: notes -->
> +<!ENTITY computedViewTitle     "Computed">

Aren't these already defined in inpector.properties? If so we do not need these.

::: devtools/client/shared/output-parser.js:102
(Diff revision 1)
> +   * @param  {Bool} labeled
> +   *         Output is name of field and value
> +   * @return {DocumentFragment}
> +   *         A document fragment containing property value.
> +   */
> +  parseAccessibleProperty: function (name, value, labeled = false) {

What if we do something like:
parseAccessibleProperty: function (value, name) {
  this._appendTextNode(name ? `${name}: ${value}` : value);
  return this._toDOM();
}

::: devtools/client/themes/inspector.css:111
(Diff revision 1)
>  
>  /* "no results" warning message displayed in the ruleview and in the computed view */
>  
>  #ruleview-no-results,
> -#computedview-no-results {
> +#computedview-no-results,
> +#accessibilityNoResults {

I would follow the naming pattern as above.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 11 • 7 years ago

FYI - try build with tools a11y inspector:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4140b12bd931650bd83c555741d9a466c6696e11

	Patrick Brosset <:pbro>
	Comment 13 • 7 years ago

Comment on attachment 8888040 [details] [diff] [review]
1151468 Accessibility Spec/Actor/Front patch

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

This looks like a great start. I have made a few comments below. But nothing very major. The code is easy to read and follow, and seems simple enough.

However, can you provide a script example showing how this new functionality is intended to be used from the client-side? I have a good idea, but seeing this would help a lot.
Some of the methods return very specific things, and usually adding this kind of specific things go with adding the associated UI. It's hard to assess the value without it.
Maybe you have a very detailed product spec already that is driving this, though, and I'd like to know about it.

Also, note that I'll be on PTO starting next week, so you'll need to ask someone else for future reviews.

::: devtools/server/actors/accessibility.js
@@ +11,5 @@
> +  Actor,
> +  ActorClassWithSpec
> +} = require("devtools/shared/protocol");
> +const promise = require("promise");
> +const events = require("sdk/event/core");

We are getting rid of our dependencies on the SDK because it's going away in 57. Can you ask Matteo (:zer0) about what to use here instead please?

@@ +163,5 @@
> +    let win = this.walker.rootWin;
> +    return {
> +      bounds: this.bounds,
> +      devicePixelRatio: win.devicePixelRatio,
> +      offset: { x: win.mozInnerScreenX, y: win.mozInnerScreenY }

I don't really know what the UI is going to look like and features it will expose (beyond the screenshots attached to this bug), so for me this list of properties seems a bit random. Why are these 3 properties being returned here? Will you be adding others in the future?

@@ +247,5 @@
> +    Actor.prototype.initialize.call(this, conn);
> +    this.tabActor = tabActor;
> +    this.rootWin = tabActor.window;
> +    this.rootDoc = tabActor.window.document;
> +    this.domWalker = domWalker;

Why does this actor need a reference to the domWalker? It seems to send it to the client in the form method, but doesn't use it itself.

@@ +249,5 @@
> +    this.rootWin = tabActor.window;
> +    this.rootDoc = tabActor.window.document;
> +    this.domWalker = domWalker;
> +    this.refMap = new Map();
> +    this.readyDeferred = promise.defer();

A comment line before this would be useful to understand what this is.

@@ +341,5 @@
> +  getRef(rawAccessible) {
> +    return this.refMap.get(rawAccessible);
> +  },
> +
> +  addref(rawAccessible) {

nit: addRef instead of addref

@@ +399,5 @@
> +      return Promise.resolve(doc);
> +    }
> +  },
> +
> +  getAccessibleFor(domnode) {

Rename this argument to either domNode or just node.

@@ +426,5 @@
> +            this.readyDeferred.resolve();
> +          }
> +        }
> +        if (accessible) {
> +          // Only propagate state change events for active accesibles.

nit: s/accesibles/accessibles

::: devtools/server/tests/browser/browser_accessibility_node.js
@@ +24,5 @@
> +    childCount: 1,
> +    domNodeType: 1
> +  });
> +
> +  // Actions

I suggest turning all of your comments inside test files to info() statements with the same strings in them. This way, you can still read them when reading the code in a text editor, but you can also see them when reading logs on treeherder when things go wrong. They add context to what the test is currently doing and make it easier to understand where problem occur.

::: devtools/shared/fronts/accessibility.js
@@ +14,5 @@
> +  accessibleWalkerSpec,
> +  accessibilitySpec
> +} = require("devtools/shared/specs/accessibility");
> +
> +const events = require("sdk/event/core");

Same comment as earlier about using SDK modules.

@@ +20,5 @@
> +
> +const AccessibleFront = FrontClassWithSpec(accessibleSpec, {
> +  initialize(client, form) {
> +    Front.prototype.initialize.call(this, client, form);
> +    this.id = ACCESSIBLE_FRONT_ID++;

Not sure why this is needed? Fronts and Actors share an actorID property that uniquely identify them already. So this doesn't seem useful.

@@ +95,5 @@
> +exports.AccessibleFront = AccessibleFront;
> +
> +const AccessibleWalkerFront = FrontClassWithSpec(accessibleWalkerSpec, {
> +  initialize(client, form) {
> +    this.id = "ACCESSIBLE_WALKER";

What is this id property? What's he need for this?

@@ +105,5 @@
> +  }),
> +
> +  form(json) {
> +    this.actorID = json.actor;
> +    this.domWalker = json.domWalker ?

I see now why you have the domWalker in the actor. But I wonder if you need to pass from the front to the actor and then back at all. It seems like an unnecessary round trip since you don't need the domWalker on the server.
Why not remove all references to it from here and from the actors.
And then only rewrite the getDOMNode method on AccessibleFront to be like this instead:

getDOMNode(domWalker) {
  return domWalker.getNodeFromActor(this.actorID, ["rawAccessible", "DOMNode"]);
},

Consumers of this will need to have a reference to the domWalker to do this obviously, but with your solution too.

@@ +113,5 @@
> +
> +exports.AccessibleWalkerFront = AccessibleWalkerFront;
> +
> +const AccessibilityFront = FrontClassWithSpec(accessibilitySpec, {
> +  initialize(client, { accessibilityActor }) {

nit: No need to destructure the second argument since you immediately recreate an object to pass it to the parent class initialize method.

	Gabriel [:gl] (ΦωΦ)
	Comment 17 • 7 years ago

Comment on attachment 8889495 [details] [diff] [review]
1151468 Accessibility Spec/Actor/Front patch v2

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

::: devtools/server/actors/accessibility.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

.eslintignore currently ignores all actors by default. Add an exception for this new file, so it's linted properly from the start.

@@ +6,5 @@
> +
> +const { Cc, Ci, Cu } = require("chrome");
> +const { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm");
> +const Services = require("Services");
> +const {

Can probably just fit in one line
const { Actor, ActorClassWithSpec } = require("devtools/shared/protocol");

@@ +10,5 @@
> +const {
> +  Actor,
> +  ActorClassWithSpec
> +} = require("devtools/shared/protocol");
> +const promise = require("promise");

Replace this promise usage with require(devtools/shared/defer).defer. See Bug 1387123 - Replace all usages of require(promise).defer by require(devtools/shared/defer).defer

s/const promise = require("promise");/const defer = require("devtools/shared/defer");

@@ +22,5 @@
> +const nsIAccessibleEvent = Ci.nsIAccessibleEvent;
> +const nsIAccessibleStateChangeEvent = Ci.nsIAccessibleStateChangeEvent;
> +const nsIPropertyElement = Ci.nsIPropertyElement;
> +
> +const EVENT_ACCELERATOR_CHANGE = nsIAccessibleEvent.EVENT_ACCELERATOR_CHANGE;

Can we simplify all of this with destructuring?

const { EVENT_... , EVENT_... } = nsIAccessibleEvent;

@@ +40,5 @@
> +  nsIAccessibleEvent.EVENT_TEXT_ATTRIBUTE_CHANGED;
> +const EVENT_VALUE_CHANGE = nsIAccessibleEvent.EVENT_VALUE_CHANGE;
> +
> +const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +

We could use a top level JSDoc for the file similar to http://searchfox.org/mozilla-central/source/devtools/server/actors/layout.js#13

@@ +45,5 @@
> +/**
> + * The AccessibleActor provides information about a given accessible object: its
> + * role, name, states, etc.
> + */
> +let AccessibleActor = ActorClassWithSpec(accessibleSpec, {

s/let/const

@@ +46,5 @@
> + * The AccessibleActor provides information about a given accessible object: its
> + * role, name, states, etc.
> + */
> +let AccessibleActor = ActorClassWithSpec(accessibleSpec, {
> +  initialize(walker, acc) {

s/acc/rawAccessible to be more clear what this is.

@@ +52,5 @@
> +    this.walker = walker;
> +    this.rawAccessible = acc;
> +  },
> +
> +  marshallPool() {

This needs a comment about why you need a marshallPool. In this case, you can probably copy existing JSDoc from the inspector.

@@ +64,5 @@
> +  },
> +
> +  /**
> +   * Indicates if the raw accessible is no longer alive.
> +   * @return boolean

Add a new line to separate the function description and @param/return

s/boolean/{Boolean}

@@ +67,5 @@
> +   * Indicates if the raw accessible is no longer alive.
> +   * @return boolean
> +   */
> +  get isDefunct() {
> +    let defunct = false;

We can set this to true, and don't have to set it in the catch again.

@@ +68,5 @@
> +   * @return boolean
> +   */
> +  get isDefunct() {
> +    let defunct = false;
> +    try {

Add a new line before and after the try block

@@ +71,5 @@
> +    let defunct = false;
> +    try {
> +      let extState = {};
> +      this.rawAccessible.getState({}, extState);
> +      defunct = extState.value & Ci.nsIAccessibleStates.EXT_STATE_DEFUNCT;

s/&/&& i assume

@@ +72,5 @@
> +    try {
> +      let extState = {};
> +      this.rawAccessible.getState({}, extState);
> +      defunct = extState.value & Ci.nsIAccessibleStates.EXT_STATE_DEFUNCT;
> +    } catch (x) {

s/x/e

@@ +210,5 @@
> +    };
> +  }
> +});
> +
> +exports.AccessibleActor = AccessibleActor;

I prefer to have all the exports at the bottom. So, it is clear what is exported in the entire file.

@@ +213,5 @@
> +
> +exports.AccessibleActor = AccessibleActor;
> +
> +/**
> + * The AccessibleWalkerActor stores a cache of accessible actors that represent

s/accessible actors/AccessibleActors

@@ +230,5 @@
> +    this.refMap = new Map();
> +    // Accessibility Walker should only be considered ready, when raw accessible
> +    // object for root document is fully initialized (e.g. does not have a
> +    // 'busy' state)
> +    this.readyDeferred = promise.defer();

s/promise.defer/defer

@@ +232,5 @@
> +    // object for root document is fully initialized (e.g. does not have a
> +    // 'busy' state)
> +    this.readyDeferred = promise.defer();
> +
> +    XPCOMUtils.defineLazyGetter(this, "a11yService", () => {

We actually have DevToolsUtil.defineLazyGetter that we should use instead of XPCOMUtils. I am not sure if XPCOMUtils is actually doing anything differently, but if we are able to use DevToolsUtil here, then we should do it for consistency. 

See http://searchfox.org/mozilla-central/source/devtools/server/css-logic.js#1469 for an example.

@@ +247,5 @@
> +  },
> +
> +  onUnload({ window }) {
> +    let doc = window.document;
> +    let actor = this.getRef(doc);

Add a new line after this.

@@ +255,5 @@
> +      return;
> +    }
> +
> +    // Purge document's subtree from accessible actors cache.
> +    this.purgeSubtree(this.a11yService.getAccessibleFor(this.doc));

Add a new line after this.

@@ +260,5 @@
> +    // If document is a root document, clear it's reference and cache.
> +    if (this.rootDoc === doc) {
> +      this.rootDoc = null;
> +      this.refMap.clear();
> +      this.readyDeferred = promise.defer();

s/promise.defer/defer

@@ +310,5 @@
> +
> +    Actor.prototype.destroy.call(this);
> +  },
> +
> +  form() {

This is probably not needed.

@@ +336,5 @@
> +
> +  /**
> +   * Clean up accessible actors cache for a given accessible's subtree.
> +   *
> +   * @param {nsIAccessible} rawAccessible

s/@param {/@param  {

Note the 2 spaces. If there was a @return below it this would all line up with the "{"

@@ +346,5 @@
> +        this.purgeSubtree(child);
> +      }
> +    }
> +
> +    this.refMap.delete(rawAccessible);

Add a new line after this.

@@ +358,5 @@
> +   * A helper method. Accessibility walker is assumed to have only 1 child which
> +   * is the top level document.
> +   */
> +  children() {
> +    return Promise.all([this.getDocument()]);

I don't think we need to do a Promise.all for a single promise.

@@ +365,5 @@
> +  /**
> +   * A promise for a root document accessible actor that only resolves when its
> +   * corresponding document accessible object is fully loaded.
> +   *
> +   * @returns {Promise}

s/returns/return

@@ +369,5 @@
> +   * @returns {Promise}
> +   */
> +  getDocument() {
> +    let doc = this.addRef(this.a11yService.getAccessibleFor(this.rootDoc));
> +    let states = doc.getState();

Add a new line after this. I prefer to separate if/try statements into a clean block

@@ +387,5 @@
> +
> +  /**
> +   * Accessible event observer function.
> +   *
> +   * @param {nsIAccessibleEvent} subject  accessible event object.

Move "accessible event object" to a new line.

@@ +397,5 @@
> +
> +    switch (event.eventType) {
> +      case EVENT_STATE_CHANGE:
> +        let { state, isEnabled } = event.QueryInterface(nsIAccessibleStateChangeEvent);
> +        let states = [...this.a11yService.getStringStates(state, 0)];

Add a new line after this.

@@ +408,5 @@
> +            this.rootWin.gBrowser.selectedBrowser.contentDocument == DOMNode)) {
> +            this.readyDeferred.resolve();
> +          }
> +        }
> +        if (accessible) {

Add a new line before and after this.

@@ +477,5 @@
> + * The AccessibilityActor is a top level container actor that initializes
> + * accessible walker and is the top-most point of interaction for accessibility
> + * tools UI.
> + */
> +let AccessibilityActor = ActorClassWithSpec(accessibilitySpec, {

Maybe you don't even need this and can do something similar to http://searchfox.org/mozilla-central/source/devtools/server/actors/inspector.js#2691

::: devtools/server/tests/browser/head.js
@@ +264,5 @@
> +}
> +
> +function checkA11yFront(front, expected, expectedFront) {
> +  ok(front, "The accessibility front is created");
> +  if (expectedFront) {

Add a new line before and after this if statement.

::: devtools/shared/fronts/accessibility.js
@@ +21,5 @@
> +  marshallPool() {
> +    return this.walker;
> +  },
> +
> +  form(json) {

This seems incorrect since we already declare the form in the AccesibleActor. We should be adding getter methods to fetch from this existing form. See http://searchfox.org/mozilla-central/source/devtools/shared/fronts/layout.js#11 and https://bugzilla.mozilla.org/show_bug.cgi?id=1308257

::: devtools/shared/specs/accessibility.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Add a new line after this.

@@ +53,5 @@
> +  },
> +
> +  methods: {
> +    getActions: {
> +      request: { },

s/{ }/{}/g

@@ +85,5 @@
> +    }
> +  }
> +});
> +
> +exports.accessibleSpec = accessibleSpec;

Move all the exports to the end of the file

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 18 • 7 years ago

(In reply to Gabriel [:gl] (ΦωΦ) from comment #17)
> Comment on attachment 8889495 [details] [diff] [review]
> 1151468 Accessibility Spec/Actor/Front patch v2
> 
> Review of attachment 8889495 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +71,5 @@
> > +    let defunct = false;
> > +    try {
> > +      let extState = {};
> > +      this.rawAccessible.getState({}, extState);
> > +      defunct = extState.value & Ci.nsIAccessibleStates.EXT_STATE_DEFUNCT;
> 
> s/&/&& i assume

extState.value is a bitmask so we are ANDing with the defunct state.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 19 • 7 years ago

(In reply to Gabriel [:gl] (ΦωΦ) from comment #17)
> Comment on attachment 8889495 [details] [diff] [review]
> 1151468 Accessibility Spec/Actor/Front patch v2
> 
> Review of attachment 8889495 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +358,5 @@
> > +   * A helper method. Accessibility walker is assumed to have only 1 child which
> > +   * is the top level document.
> > +   */
> > +  children() {
> > +    return Promise.all([this.getDocument()]);
> 
> I don't think we need to do a Promise.all for a single promise.

Children method for AccessibleActors returns a promise that resolves to array. Walker is set up to have only 1 child - an AccessibleActor for a top level document. I wanted to make sure that both of them return the same thing - a promise resolving to an array. If I drop Promise.all I would still have to do something like - this.getDocument().then(doc => [doc]) . Hopefully I interpreted your comment right.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 20 • 7 years ago

(In reply to Gabriel [:gl] (ΦωΦ) from comment #17)
> Comment on attachment 8889495 [details] [diff] [review]
> 1151468 Accessibility Spec/Actor/Front patch v2
> 
> Review of attachment 8889495 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +477,5 @@
> > + * The AccessibilityActor is a top level container actor that initializes
> > + * accessible walker and is the top-most point of interaction for accessibility
> > + * tools UI.
> > + */
> > +let AccessibilityActor = ActorClassWithSpec(accessibilitySpec, {
> 
> Maybe you don't even need this and can do something similar to
> http://searchfox.org/mozilla-central/source/devtools/server/actors/inspector.
> js#2691

I noticed Patrick mentions in bug 1308257 that walker should stay as simple as possible and only contain information about the DOM tree. Since the accessibility tree and events are separate, I was hoping to keep it outside of the walker, though I see what you mean here. Let me know if you still want me to go ahead with this request.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 21 • 7 years ago

Hopefully all comments addressed.

	Patrick Brosset <:pbro>
	Comment 22 • 7 years ago

Comment on attachment 8895879 [details] [diff] [review]
1151468 Accessibility Spec/Actor/Front patch v3

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

Looking good. I'd like to take one more pass once you address the below comments and we should be good. Thanks!

::: devtools/server/actors/accessibility.js
@@ +8,5 @@
> +const DevToolsUtils = require("devtools/shared/DevToolsUtils");
> +const Services = require("Services");
> +const { Actor, ActorClassWithSpec } = require("devtools/shared/protocol");
> +const defer = require("devtools/shared/defer");
> +const events = require("sdk/event/core");

You can't use sdk/event/core anymore. The sdk is going away with 57, and we're migrating DevTools away from it.
Julian recently landed a massive change for DevTools in bug 1137935. See https://hg.mozilla.org/mozilla-central/rev/d9be0f1c5dcd

@@ +226,5 @@
> +  },
> +
> +  form() {
> +    return {
> +      actor: this.actorID,

Without knowing the UI it's a little hard to know if it would be useful, but shouldn't attributes, state, actions and indexInParent also be part of this form, as properties? This way maybe the client-side doesn't need to request them directly?

::: devtools/server/tests/browser/browser_accessibility_node_events.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* eslint-disable mozilla/no-cpows-in-tests */

Why do you need this exception? This seems dangerous. At some stage cpows are going to be fail tests so we don't want to add one more test on the list of tests to fix then.

::: devtools/server/tests/browser/head.js
@@ +252,5 @@
>  function getCookieId(name, domain, path) {
>    return `${name}${SEPARATOR_GUID}${domain}${SEPARATOR_GUID}${path}`;
>  }
> +
> +async function emitA11yEvent(emitter, name, handler, task) {

Please add some doc comments for this function, so it's easy to know what it does.

@@ +253,5 @@
>    return `${name}${SEPARATOR_GUID}${domain}${SEPARATOR_GUID}${path}`;
>  }
> +
> +async function emitA11yEvent(emitter, name, handler, task) {
> +  let { resolve, promise } = defer();

I lost track of how many promise implementations we have these days and which ones we should be using. Can you ask ochameau on IRC if there's something else you should be using or if this is fine? I know he's working on cleaning up our promises usage in DevTools at the moment (bug 1384527 and bug 1387128).

@@ +262,5 @@
> +  await ContentTask.spawn(gBrowser.selectedBrowser, {}, task);
> +  await promise;
> +}
> +
> +function checkA11yFront(front, expected, expectedFront) {

Please add some doc comments for this function, so it's easy to know what it does.

@@ +274,5 @@
> +    is(front[key], expected[key], `accessibility front has correct ${key}`);
> +  }
> +}
> +
> +async function waitForA11yShutdown() {

Please add some doc comments for this function, so it's easy to know what it does.

::: devtools/shared/fronts/accessibility.js
@@ +15,5 @@
> +  accessibleWalkerSpec,
> +  accessibilitySpec
> +} = require("devtools/shared/specs/accessibility");
> +
> +const events = require("sdk/event/core");

You can't use sdk/event/core anymore. The sdk is going away with 57, and we're migrating DevTools away from it.
Julian recently landed a massive change for DevTools in bug 1137935. See https://hg.mozilla.org/mozilla-central/rev/d9be0f1c5dcd

::: devtools/shared/specs/accessibility.js
@@ +126,5 @@
> +const accessibilitySpec = generateActorSpec({
> +  typeName: "accessibility",
> +
> +  methods: {
> +    getWalker: {

Do we need a getWalker method though? Here you have 2 sort of top level actors. The general accessibility actor, and the accessiblewalker actor. But I don't think you'll ever do anything with the accessibility actor other than get the walker.
So maybe just have 1 actor, which would be the walker itself. The client-side would instantiate this one and that's it.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 23 • 7 years ago

Comment on attachment 8895879 [details] [diff] [review]
1151468 Accessibility Spec/Actor/Front patch v3

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

Some responses:

::: devtools/server/actors/accessibility.js
@@ +226,5 @@
> +  },
> +
> +  form() {
> +    return {
> +      actor: this.actorID,

The UI is similar to the Inspector UI where there will be a tree of accessible objects. The only things visible in the tree are a name and a role so I was trying not to call xpcom methods unless necessary. We do query states, attrs, etc when the user selects an accessible and we render its details in the sidebar. Those properties can also update dynamically, when the appropriate a11y event is received.

::: devtools/server/tests/browser/head.js
@@ +253,5 @@
>    return `${name}${SEPARATOR_GUID}${domain}${SEPARATOR_GUID}${path}`;
>  }
> +
> +async function emitA11yEvent(emitter, name, handler, task) {
> +  let { resolve, promise } = defer();

Sorry, I haven't been able to reach ochameau just yet, however, I am using the same approach/module seen in this head.js file (see https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/browser/head.js#210) so perhaps it's OK?

::: devtools/shared/specs/accessibility.js
@@ +126,5 @@
> +const accessibilitySpec = generateActorSpec({
> +  typeName: "accessibility",
> +
> +  methods: {
> +    getWalker: {

I was actually planning, in the follow up work, to start tracking a11y-service-init-shutdown events to indicate when accessibility is enabled/disabled or reasons why it can't be disabled for example. This is also a potentially good actor for containing other a11y related children actors other than walker, related to a11y reporting?

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 24 • 7 years ago

Alexandre, is the approach regarding the use let {promise, resolve} = defer(); acceptable in head.js? or should I be doing something else?

	Alexandre Poirot [:ochameau]
	Comment 25 • 7 years ago

Comment on attachment 8902795 [details] [diff] [review]
1151468 Accessibility Spec/Actor/Front patch v4

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

::: devtools/server/tests/browser/head.js
@@ +264,5 @@
> +  let { resolve, promise } = defer();
> +  emitter.once(name, (...args) => {
> +    handler.apply(null, args);
> +    resolve();
> +  });

I think front.once already returns a promise?
So you may be able to do:
  let promise = emitter.once(name, (...args) => {
    handler(...args);
  });
If not, we are trying to avoid using defer and instead use plain promises, which would look like this:
  let promise = new Promise(done => {
    emitter.once(name, (...args) => {
      handler(...args);
      done();
    });
  });

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 26 • 7 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #25)
> Comment on attachment 8902795 [details] [diff] [review]
> 1151468 Accessibility Spec/Actor/Front patch v4
> 
> Review of attachment 8902795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/server/tests/browser/head.js
> @@ +264,5 @@
> > +  let { resolve, promise } = defer();
> > +  emitter.once(name, (...args) => {
> > +    handler.apply(null, args);
> > +    resolve();
> > +  });
> 
> I think front.once already returns a promise?
> So you may be able to do:
>   let promise = emitter.once(name, (...args) => {
>     handler(...args);
>   });
> If not, we are trying to avoid using defer and instead use plain promises,
> which would look like this:
>   let promise = new Promise(done => {
>     emitter.once(name, (...args) => {
>       handler(...args);
>       done();
>     });
>   });

Thanks, that worked!

	Patrick Brosset <:pbro>
	Comment 27 • 7 years ago

Comment on attachment 8903009 [details] [diff] [review]
1151468 Accessibility Spec/Actor/Front patch v5

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

Bugzilla's interdiff is failing for me, so I'm entirely sure of the amount of things that changed in the past 2 patches, but I see my comments have been addressed or answered in comment 23. So this looks good to me now.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 28 • 7 years ago

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

	Pulsebot
	Comment 29 • 7 years ago

Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f75865f1986
add accessibility spec/actor/front to devtools. r=pbro

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 30 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0f75865f1986

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 31 • 7 years ago

Hi Victoria,

Blake mentioned I could flag you for a ui-review of this accessibility tree inspector tool I've been working on. Here's the link to the try build with it. You would need to enable accessibility checkbox in the DevTools settings before being able to see the accessiblity tab.

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

Thanks

	Victoria Wang [:victoria]
	Comment 32 • 7 years ago

Thanks Yura, I'm very excited to look at this! For the rest of this week and much of next week, I'm busy with designer tools projects, but I hope I can get to this soon. Do you have any specific timeline for this project?

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 33 • 7 years ago

(In reply to Victoria Wang [:victoria] from comment #32)
> Thanks Yura, I'm very excited to look at this! For the rest of this week and
> much of next week, I'm busy with designer tools projects, but I hope I can
> get to this soon. Do you have any specific timeline for this project?

I was hoping to land it in 58 cycle, but possibly 59 to have enough time to get support from devrel, marketing etc.

	Victoria Wang [:victoria]
	Comment 34 • 7 years ago

(In reply to Yura Zenevich [:yzen] from comment #33)
> I was hoping to land it in 58 cycle, but possibly 59 to have enough time to
> get support from devrel, marketing etc.

Thanks for the info Yura! So sounds like it would be nice to get a first version of this into the beta merge on 2017-11-13, but it may need more time depending on other departments. I've noted this in my docs and will see what I can do. Will let you know when I start looking at this.

	Jen Simmons [:jensimmons]
	Comment 35 • 7 years ago

This is very exciting.

I would like to help promote this once it's dropped in Nightly. Could someone give me a tour? Is it in Nightly behind a flag at the moment? Or not yet?

Also, if you'd like feedback from some of the best speakers who promote & teach accessibility, let me know. I can connect you with them, or reach out to them myself and get feedback. (I'm especially thinking of Derek Featherstone.)

	Patrick Brosset <:pbro>
	Comment 36 • 7 years ago

Yura is probably the best person who can give you a demo of this. Needinfo'ing him here.

	Victoria Wang [:victoria]
	Comment 37 • 7 years ago

Oh, if Yura is able to give Jen a demo, I'd love to be in that call!

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 38 • 7 years ago

(In reply to Jen Simmons [:jensimmons] from comment #35)
> This is very exciting.
> 
> I would like to help promote this once it's dropped in Nightly. Could
> someone give me a tour? Is it in Nightly behind a flag at the moment? Or not
> yet?
> 
> Also, if you'd like feedback from some of the best speakers who promote &
> teach accessibility, let me know. I can connect you with them, or reach out
> to them myself and get feedback. (I'm especially thinking of Derek
> Featherstone.)

(In reply to Victoria Wang [:victoria] from comment #37)
> Oh, if Yura is able to give Jen a demo, I'd love to be in that call!

I could set up a quick call next week to show you what the tools does. I got some feedback from Gabriel a couple of weeks ago and working on a small improvement for the panel, but it should be in a good shape at this point.

	:Harald Kirschner :digitarald
	Comment 39 • 7 years ago

Lets plan to have this in by 59 to gather feedback. This will give us enough time for polish on Nightly.

There is a follow up discussion on if we should move this to github for long term maintenance and capturing more community. This is also a great target to allow integration with extensions, like aXe.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 40 • 7 years ago

(In reply to :Harald Kirschner :digitarald from comment #39)
> Lets plan to have this in by 59 to gather feedback. This will give us enough
> time for polish on Nightly.

Agreed, 59 is the likely target.

	Victoria Wang [:victoria]
	Comment 41 • 7 years ago

Just looked at this - thanks Yura for the build! (here's the new link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=017d99fed5c0ec1535165cb852bc878488897d7c&selectedJob=144599234)

I have some UI suggestions for the intro panel, toggle button, and some consistency fixes for the tree views. I will check with Harald/Blake on my priorities before writing those up. 

For deeper UX concerns, I'll need to understand this panel better. Is there a website that I should use with this that would show more accessibility properties? Are there existing accessibility inspectors I can look at to see what the current alternatives are? A demo to understand the common workflow would be helpful. Do you all have a doc for this project, or other info that isn't in this bug?

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 42 • 7 years ago

(In reply to Victoria Wang [:victoria] from comment #41)
> Just looked at this - thanks Yura for the build! (here's the new link:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=017d99fed5c0ec1535165cb852bc878488897d7c&selectedJob=1
> 44599234)
> 
> I have some UI suggestions for the intro panel, toggle button, and some
> consistency fixes for the tree views. I will check with Harald/Blake on my
> priorities before writing those up. 

Hi Victoria, thanks for looking at it. Looking forward to your suggestions.

> 
> For deeper UX concerns, I'll need to understand this panel better. Is there
> a website that I should use with this that would show more accessibility
> properties? Are there existing accessibility inspectors I can look at to see
> what the current alternatives are? A demo to understand the common workflow
> would be helpful. Do you all have a doc for this project, or other info that
> isn't in this bug?

In terms of websites you should be able to use it with any website you like. Chances are, some websites are presented in accessibility tree very sparsely (at least in a way Firefox build its accessible tree; our team is going to be looking into pruning it where possible but that would be an ongoing initiative). I would suggest something relatively simple, otherwise you would have to dig deeper into the tree.

Some helpful information about what accessibility tree is here:
https://developers.google.com/web/fundamentals/accessibility/semantics-builtin/the-accessibility-tree
https://developer.paciellogroup.com/blog/2015/01/the-browser-accessibility-tree/

In terms of existing tools and alternatives: we used to have accessibility tree inspector as part of DOM Inspector addon that was popular around the time Firebug was big. It is now (especially with web-extensions) obsolete, but here is a good write up by one of our engineers from a couple of years back: http://asurkov.blogspot.ca/2012/02/dom-inspector-as-accessibility-tool.html

Chrome currently has a very basic tree that is not visually friendly: go to chrome://accessibility/ were you can show accessibility tree for tabs that you have open.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 43 • 7 years ago

Adding Jen's comment from bug 1249839 to keep it within this bug. It might later be spun into its own bug as an improvement to accessibility panel:

I just ran across this:  
The Visual ARIA Bookmarklet
http://whatsock.com/training/matrices/visual-aria.htm

It's a good idea — create some kind of visual tool that highlights ARIA roles in use. Make what is typically experienced by a screenreader or assistive technology visual, so developers can easily scan visually and see if they are doing things right. 

I don't know if there's discussion on how to create a visual helper tool for better accessibility on another Bugzilla ticket, but perhaps this suggestion is helpful.

	Victoria Wang [:victoria]
	Comment 44 • 7 years ago

(In reply to Yura Zenevich [:yzen] from comment #42)
> Some helpful information about what accessibility tree is here:
> https://developers.google.com/web/fundamentals/accessibility/semantics-
> builtin/the-accessibility-tree
> https://developer.paciellogroup.com/blog/2015/01/the-browser-accessibility-
> tree/
> 
> In terms of existing tools and alternatives: we used to have accessibility
> tree inspector as part of DOM Inspector addon that was popular around the
> time Firebug was big. It is now (especially with web-extensions) obsolete,
> but here is a good write up by one of our engineers from a couple of years
> back:
> http://asurkov.blogspot.ca/2012/02/dom-inspector-as-accessibility-tool.html
> 
> Chrome currently has a very basic tree that is not visually friendly: go to
> chrome://accessibility/ were you can show accessibility tree for tabs that
> you have open.

Yura, thanks for all the info! Will take a look at these. I'll also send out a meeting invite to demo/discuss this next week (will invite you and Jen).

	Eitan Isaacson [:eeejay]
	Comment 45 • 7 years ago

I am already using this tool and I love it. Some feedback:
 * Tree consistency with the DOM inspector, like Victoria said. The cursor changes when hovering over nodes, an underline appears, selecting an item with children expands it. All those things don't happen in the inspector.
 * It would be really helpful to have a way to go from a dom node in the inspector to an equivalent accessible. Does that feature exist? Couldn't find it.
 * The states (and attributes) subtrees collapse very easily. It makes it hard to compare the state of one node with another, or to see a state change from an interaction on the page. I would like to see this update and highlight in real time, kind of like how CSS rules update in the inspector.
 * When an item goes away the tree flickers and the cursor goes to the root node. In contrast, it goes to a neighbouring node in the DOM inspector.

	Patrick Brosset <:pbro>
	Comment 46 • 7 years ago

(In reply to Victoria Wang [:victoria] from comment #44)
> Yura, thanks for all the info! Will take a look at these. I'll also send out
> a meeting invite to demo/discuss this next week (will invite you and Jen).
I wouldn't mind getting an invite for this too!

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 47 • 7 years ago

Comment on attachment 8888899 [details] [diff] [review]
1151468 Accessibility Inspector WIP

Latest revision: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cbb49aef070e690038a9511ac1d09a71802df86

(In reply to Eitan Isaacson [:eeejay] from comment #45)
> I am already using this tool and I love it. Some feedback:
>  * Tree consistency with the DOM inspector, like Victoria said. The cursor
> changes when hovering over nodes, an underline appears, selecting an item
> with children expands it. All those things don't happen in the inspector.

Should be addressed.

>  * It would be really helpful to have a way to go from a dom node in the
> inspector to an equivalent accessible. Does that feature exist? Couldn't
> find it.

Yes, it's definitely going to happen. I was hoping to do it as follow up.

>  * The states (and attributes) subtrees collapse very easily. It makes it
> hard to compare the state of one node with another, or to see a state change
> from an interaction on the page. I would like to see this update and
> highlight in real time, kind of like how CSS rules update in the inspector.

Expanded state should be addressed. Animation is going to have to be a follow up as it's not supported by the tree component I'm reusing.

>  * When an item goes away the tree flickers and the cursor goes to the root
> node. In contrast, it goes to a neighbouring node in the DOM inspector.

The tree is likely to be re-rendered. I think we can do something smart with update animation. The part regarding the active descendant (e.g. selected item) should now be fixed.

	Victoria Wang [:victoria]
	Comment 49 • 7 years ago

I installed the updated build. Attached is a mockup with some ideas for the initial install view.

- Put the enable button front-and-center (I still need to figure out a good solution for the deactivate button, but I was thinking we could borrow some behavior from the Debugger/Network monitor's "active" modes)
- Don't initially show sidebar/toolbar
- Made some copy change suggestions and added icon and more centering to the intro message

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 50 • 7 years ago

(In reply to Victoria Wang [:victoria] from comment #49)
> Created attachment 8932283 [details]
> initial mockup for accessibility start up panel
> 
> I installed the updated build. Attached is a mockup with some ideas for the
> initial install view.
> 
> - Put the enable button front-and-center (I still need to figure out a good
> solution for the deactivate button, but I was thinking we could borrow some
> behavior from the Debugger/Network monitor's "active" modes)
> - Don't initially show sidebar/toolbar
> - Made some copy change suggestions and added icon and more centering to the
> intro message

Thanks!

	Victoria Wang [:victoria]
	Comment 51 • 7 years ago

And here's an initial idea for the main view. Maybe the [ON] badge is too loud, but I was thinking it could work as a button to deactivate A11y features if one is on a different panel and noticing performance problems. It could just go inside the panel somewhere as well. I mainly just wanted to move away from the button having its own extra row.

https://mozilla.invisionapp.com/share/2XEEY0RYA#/266514538_Accessibility_panel

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 52 • 7 years ago

(In reply to Victoria Wang [:victoria] from comment #51)
> And here's an initial idea for the main view. Maybe the [ON] badge is too
> loud, but I was thinking it could work as a button to deactivate A11y
> features if one is on a different panel and noticing performance problems.
> It could just go inside the panel somewhere as well. I mainly just wanted to
> move away from the button having its own extra row.
> 
> https://mozilla.invisionapp.com/share/2XEEY0RYA#/
> 266514538_Accessibility_panel

I like this idea. I think it's also worth bringing to your attention bug 1383051. It is currently going into a shield study and if it is successful, could land in the same release cycle for all users. Indicator shows up similarly to a private browsing badge when accessibility is on.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 53 • 7 years ago

Last night build https://treeherder.mozilla.org/#/jobs?repo=try&revision=2273d94e70ccf6ec829e66c3e3b8e3b6089f238a with feedback from Eitan. Haven't had a chance yet to add Victoria's suggestions.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 54 • 7 years ago

(In reply to Victoria Wang [:victoria] from comment #51)
> And here's an initial idea for the main view. Maybe the [ON] badge is too
> loud, but I was thinking it could work as a button to deactivate A11y
> features if one is on a different panel and noticing performance problems.
> It could just go inside the panel somewhere as well. I mainly just wanted to
> move away from the button having its own extra row.
> 
> https://mozilla.invisionapp.com/share/2XEEY0RYA#/
> 266514538_Accessibility_panel

In light of today's conversation, if we are planning to add a picker functionality (and potentially more features, search for example) do you still think Toolbar is not a good option, Victoria?

	Victoria Wang [:victoria]
	Comment 55 • 7 years ago

(In reply to Yura Zenevich [:yzen] from comment #52)
> I like this idea. I think it's also worth bringing to your attention bug
> 1383051. It is currently going into a shield study and if it is successful,
> could land in the same release cycle for all users. Indicator shows up
> similarly to a private browsing badge when accessibility is on.

Oh interesting! I like that it opens a doorhanger/about page as well. This is good to know. Sounds like it can be a little less obvious for the DevTools panel then.

(I remember in the meeting that you and Patrick said you could freeze the a11y panel when switching to other panels - I'm assuming this just halts activity in the DevTools a11y panel, but browser a11y features would still be on, so it would still affect performance?)

We could copy the Debugger/Network behavior of turning the icon green if there's something active happening that can affect other panels, just to remind the user. It would be easier/more subtle than having an on/off switch in the tab. I'd then need to put a Deactivate button elsewhere in the panel. 

(In reply to Yura Zenevich [:yzen] from comment #54)
> In light of today's conversation, if we are planning to add a picker
> functionality (and potentially more features, search for example) do you
> still think Toolbar is not a good option, Victoria?

Hm, I've been thinking this over, and I think ideally, we overload the current picker button for this. It could work like this:
- User is viewing a11y panel: click picker to inspect in a11y tree
- User is in any other panel: click picker to switch to Inspector and inspect in inspector

	Victoria Wang [:victoria]
	Comment 56 • 7 years ago

The idea in my last comment (subtler a11y on/off) could look like this:

https://mozilla.invisionapp.com/share/2XEEY0RYA#/267021156_a11y_3

	Victoria Wang [:victoria]
	Comment 57 • 7 years ago

I got some great feedback from the UX peeps. They commented that the initial paragraph sounds a little discouraging. We could consider starting with something more descriptive like this:

"Inspect your website's accessibility attributes with the new Accessibility Inspector. Use the element picker to explore a specific element, or browse the whole tree."

We could display the part about performance in a second line as a kind of side-note:

"Note: Enabling accessibility features will negatively affect Firefox performance. Consider turning off accessibility features before using other Developer Tools panels."

My original mock up had a mismatch of a11y icons - the Photon one should be used next to the welcome message.

Also, re: the off-button: We could use a bottom bar, and put a couple things in it, including both the off button and a help button. Something like this:

https://mozilla.invisionapp.com/share/2XEEY0RYA#/screens/267021156_a11y_3 (oops, overrode the other version that was here)

	Gabriel [:gl] (ΦωΦ)
	Comment 59 • 7 years ago

Comment on attachment 8935846 [details] [diff] [review]
1151468.latest.patch

Flagging myself so I can remember to check out the code.

	Victoria Wang [:victoria]
	Comment 60 • 7 years ago

Here's a mockup with new ideas from a discussion I had with Yura today, showing changes to toolbar and picker:

https://mozilla.invisionapp.com/share/2XEEY0RYA#/269603614_a11y_5

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 61 • 7 years ago

Latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58a091d78cb41975c7365a0e5c55c10061fc2bdd

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 62 • 7 years ago

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

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 64 • 7 years ago

Latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1014e91750da98c4d281e6fb4bd1b46bf150bb74

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 65 • 7 years ago

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

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 66 • 7 years ago

Fixed latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dd8cd61e6e55ccb527e01ff06df48d58fac334f

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 67 • 7 years ago

Latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f726d857f979a9b88937c460caeff69d1a795f9

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 69 • 7 years ago

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

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 70 • 6 years ago

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

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 71 • 6 years ago

Latest try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=119233fe6127488143092b9698d38e29dbaa49c7

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 72 • 6 years ago

Latest try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54073536fcc5703aa71e9c7433592cd387a88c0c

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 73 • 6 years ago

Latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55523803881cb60675d1144da46d9e8170ce20b1

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 74 • 6 years ago

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

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 75 • 6 years ago

Latest try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=499d93733484e632899a1f11d15189e5131a3594

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 76 • 6 years ago

Now that the panel is riding the trains, Chris, I was wondering if there are any resources available for dev-docs for it?

	Chris Mills [:cmills]
	Comment 77 • 6 years ago

(In reply to Yura Zenevich [:yzen] from comment #76)
> Now that the panel is riding the trains, Chris, I was wondering if there are
> any resources available for dev-docs for it?

Yes!

I'll reach out to you over mail about this — we've got a thread going there too, and I don't want to spam this bug with all that conversation.