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?

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.

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

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.

(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?

(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.

(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)

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?

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

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.

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

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.

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

(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.

(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.

(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.

Hopefully all comments addressed.

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.

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?

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

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(); }); });

(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!

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.

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

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

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?

(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.

(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.

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.)

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

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

(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.

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.

(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.

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?

(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.

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.

(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).

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.

(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!

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.

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

(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!

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 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.

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.

(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?

(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

The idea in my last comment (subtler a11y on/off) could look like this: https://mozilla.invisionapp.com/share/2XEEY0RYA#/267021156_a11y_3

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)

Comment on attachment 8935846 [details] [diff] [review] 1151468.latest.patch Flagging myself so I can remember to check out the code.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

(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.