Closed Bug 1447112 Opened Last year Closed Last year

Add front end parts for accessibility inspector functionality.

Categories

(DevTools :: Accessibility Tools, enhancement)

57 Branch
enhancement
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: yzen, Assigned: yzen)

References

Details

(Keywords: access)

Attachments

(2 files, 4 obsolete files)

No description provided.
Blocks: 1151468
Attached patch 1445251 patch (obsolete) — Splinter Review
This is a first standalone pass for front end bits of the a11y inspector that is built on top of all other patches that have landed already.
Attachment #8960337 - Flags: review?(pbrosset)
Attachment #8960337 - Flags: review?(pbrosset) → review+
Attached patch 1447112 patch (obsolete) — Splinter Review
Sorry for the confusion, here's the correct patch.
Attachment #8960337 - Attachment is obsolete: true
Attachment #8960573 - Flags: review?(pbrosset)
Comment on attachment 8960573 [details] [diff] [review]
1447112 patch

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

::: devtools/client/accessibility/accessibility.css
@@ +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/. */
> +
> + :root {

nit: remove leading space

@@ +5,5 @@
> + :root {
> +   --accessibility-font-size: 12px;
> +   --accessibility-header-font: message-box;
> +   --accessibility-toolbar-heigh: 24px;
> +   --accessibility-toolbar-heigh-tall: 35px;

nit: height

@@ +89,5 @@
> +}
> +
> +@media (max-width: 800px) {
> +  .description {
> +    width: 65vw;

Can you specify a min-width rather than using 3 media queries ?

@@ +128,5 @@
> +  height: 20px;
> +  font: message-box;
> +  padding: 2px;
> +  background: var(--theme-toolbar-background);
> +  border-bottom: 1px solid var(--theme-splitter-color);

Please use the .devtools-toolbar class, all of these styles are provided for free.

@@ +132,5 @@
> +  border-bottom: 1px solid var(--theme-splitter-color);
> +}
> +
> +.toolbar .btn {
> +  background: unset !important;

Please use an empty .devtools-button, this provides the classic icon-button styles (including hover/active states) that are used by the other tools.

@@ +174,5 @@
> +    display: block;
> +    /* 1px is for the splitter between the table head and the body. */
> +    height: calc(100vh - (var(--accessibility-toolbar-heigh) + 1px) * 2);
> +    overflow-y: auto;
> +}

nit: 2 spaces indentation
Thanks, ill take care of these
Comment on attachment 8960573 [details] [diff] [review]
1447112 patch

I'll give this a quick look but I don't feel comfortable reviewing it alone. Yulia has offered to help.
Yulia: feel free to ask someone else if there are areas you would prefer someone else to look at too.
Attachment #8960573 - Flags: review?(ystartsev)
Comment on attachment 8960573 [details] [diff] [review]
1447112 patch

Also, Julian is working on a new panel too at the moment, it would make sense for him to look at this new panel too.
Attachment #8960573 - Flags: review?(jdescottes)
Comment on attachment 8960573 [details] [diff] [review]
1447112 patch

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

Really neat! I had a few nits and a few questions but overall looks really good

::: devtools/client/accessibility/AccessibilityView.js
@@ +57,5 @@
> +    }
> +
> +    let mainFrame = MainFrame({ accessibility, walker });
> +    // Render top level component
> +    let provider = createElement(Provider, { store: this.store }, mainFrame);

nit: let should be const here

@@ +67,5 @@
> +      window.emit(EVENTS.NEW_ACCESSIBLE_FRONT_INSPECTED));
> +  },
> +
> +  highlightAccessible(walker, accessible) {
> +    this.store.dispatch(highlight(walker, accessible)).then(() =>

I am wondering why we might want to use then rather than using async/await? Usually when I see something like this in the code base im expecting purposefully asynchronous behavior

@@ +72,5 @@
> +      window.emit(EVENTS.NEW_ACCESSIBLE_FRONT_HIGHLIGHTED));
> +  },
> +
> +  async selectNodeAccessible(walker, node) {
> +    let accessible = await walker.getAccessibleFor(node);

nit: let should be const here

@@ +73,5 @@
> +  },
> +
> +  async selectNodeAccessible(walker, node) {
> +    let accessible = await walker.getAccessibleFor(node);
> +    this.store.dispatch(select(walker, accessible)).then(() =>

this can use await as well, since the function is already async

@@ +83,5 @@
> +   *
> +   * @param {Object} event  message type and data.
> +   */
> +  onMessage(event) {
> +    let data = event.data;

nit: neither of these is reassigned so they should be `const`

::: devtools/client/accessibility/accessibility-panel.js
@@ +154,5 @@
> +    return this.walker.getAccessibleFor(nodeFront);
> +  },
> +
> +  postContentMessage(type, ...args) {
> +    let event = new this.panelWin.MessageEvent("devtools/chrome/message", {

nit: let should be const

::: devtools/client/accessibility/accessibility.css
@@ +87,5 @@
> +    width: 60vw;
> +  }
> +}
> +
> +@media (max-width: 800px) {

just seconding Tim Nguyen, since anything that matches a media query will be parsed and applied. In this case, max-width 1200 will also be applied to max-width 1000 and max-width 800. These three rules will all be applied overwrite each other. Whereas if you use min-width, rules for larger screens will not be applied to smaller screens. It might save us from bugs later on, which is an added benefit

@@ +162,5 @@
> +}
> +
> +.split-box.horz .treeTable {
> +  /* To compenstate for 1px splitter between the tree and sidebar. */
> +  height: calc(100% - 1px);

looks like this 1px indentation should be a var, since it is used in a few places and has specific meaning

@@ +287,5 @@
> +.accessible .tree button {
> +  display: block;
> +}
> +
> +/* NOTE: total height of the node (height + padding + border + margin) must

this looks like maybe something that should be saved in a variable? 5 px seems to be used a lot, so its looking like a magic number that should have some proper definition somewhere

@@ +332,5 @@
> +  text-overflow: ellipsis;
> +}
> +
> +.accessible .tree .object-delimiter {
> +  padding-inline-end: 6px;

6 px is often used for padding -- is there some specific meaning to this width? maybe it should be made into a var as well?

::: devtools/client/accessibility/components/AccessibilityRow.js
@@ +23,5 @@
> +const { unhighlight } = require("../actions/accessibles");
> +
> +class HighlightableTreeRowClass extends TreeRow {
> +  shouldComponentUpdate(nextProps) {
> +    let props = ["name", "open", "value", "loading", "selected", "hasChildren"];

nit, should be const

@@ +25,5 @@
> +class HighlightableTreeRowClass extends TreeRow {
> +  shouldComponentUpdate(nextProps) {
> +    let props = ["name", "open", "value", "loading", "selected", "hasChildren"];
> +
> +    for (let p in props) {

why not use 

```
for (const p of props) {
      if (nextProps.member[p] !== this.props.member[p]) {
        return true;
      }
}
```

@@ +26,5 @@
> +  shouldComponentUpdate(nextProps) {
> +    let props = ["name", "open", "value", "loading", "selected", "hasChildren"];
> +
> +    for (let p in props) {
> +      if (nextProps.member[props[p]] != this.props.member[props[p]]) {

looks like this should be a !==?

@@ +53,5 @@
> +    };
> +  }
> +
> +  componentDidMount() {
> +    let { selected, object } = this.props.member;

nit: should be const

@@ +69,5 @@
> +   * Update accessible object details that are going to be rendered inside the
> +   * accessible panel sidebar.
> +   */
> +  componentDidUpdate(prevProps) {
> +    let { selected, object } = this.props.member;

nit, should be const (this happens quite a bit in this file, maybe im missing why let is being used?)

::: devtools/client/accessibility/components/AccessibilityTree.js
@@ +66,5 @@
> +   */
> +  componentWillUnmount() {
> +    let { walker } = this.props;
> +    walker.off("reorder", this.onReorder);
> +    walker.off("name-change", this.onNameChanage);

spelling issue: onNameChanage should be onNameChange, same with onTextChanage should be onTextChange

@@ +135,5 @@
> +      accessibles,
> +      dispatch,
> +      expanded,
> +      selected,
> +      highlighted: highlightedObj,

this naming is a bit ambiguous, it isn't clear if it is a bool or an object when reading a subsegment of the code (like line 158). its not as pretty but maybe something like `activeItem`? or highlightedItem

::: devtools/client/accessibility/components/Toolbar.js
@@ +74,5 @@
> +      }, Button({
> +        className: "disable",
> +        id: "accessibility-disable-button",
> +        onClick: this.onDisable,
> +        disabled: disabling || disableButton,

its not clear to me what disableButton is-- it looks like a boolean? should it be isDisabled?

::: devtools/client/accessibility/reducers/ui.js
@@ +26,5 @@
> +  return {
> +    enabled: false,
> +    canBeDisabled: true,
> +    canBeEnabled: true,
> +

what is the significance of this space?

::: devtools/client/accessibility/test/head.js
@@ +104,5 @@
> +
> +function disableAccessibilityInspector(env) {
> +  let { doc, win } = env;
> +  EventUtils.synthesizeMouseAtCenter(
> +    doc.getElementById("accessibility-disable-button"), {}, win);

I'm not really familiar with EventUtils, but i thought the second argument needed a type? mousedown or similar? I am probably wrong about this
Attachment #8960573 - Flags: review?(ystartsev) → review+
Comment on attachment 8960573 [details] [diff] [review]
1447112 patch

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

This new panel works really well, great job! 

I mainly focused on the framework plumbing code for the panel and consistency issues, since I am also adding a new panel at the moment
in Bug 1445197. Note that we discussed a bit about consistency and guidelines with Netmonitor and Console folks and started
drafting documentation at https://docs.google.com/document/d/11CRu0F6GvviATF1FK5IwrJH0KtepLtJdNNFWPhYg7lo/edit#. Feel free to have a 
look. Some of the naming and folder structure we decided to follow will differ from yours but I can't force you to switch to this as it 
wasn't discussed with a bigger group :)

Overall this looks pretty solid. I would like to see a new version with clarifications about the bits that seem copied from 
the DOM panel (and that don't seem relevant to me here).

(note, I haven't really focused on React, the UX/UI or the tests, I assume we can use follow ups if needed)

::: devtools/client/accessibility/AccessibilityView.js
@@ +1,4 @@
> +/* 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/. */
> +"use strict";

We are using CamelCase to name react components (https://github.com/devtools-html/rfcs/issues/17)
but other files are lowercase+hyphen. Can we rename this to accessibility-view.js?

::: devtools/client/accessibility/accessibility-panel.js
@@ +63,5 @@
> +
> +    this.target.on("navigate", this.onTabNavigated);
> +    this._toolbox.on("select", this.onPanelVisibilityChange);
> +
> +    XPCOMUtils.defineConstant(this.panelWin, "EVENTS", EVENTS);

Same comment as for the other call site of defineConstant.
Maybe we can simply this.panelWin.EVENTS = EVENTS; ?

@@ +112,5 @@
> +  /**
> +   * Make sure the panel is refreshed (if needed) when it's selected.
> +   */
> +  onPanelVisibilityChange() {
> +    this._opening.then(() => this.refresh());

The method name is misleading, I assumed this was called when selecting _this_ panel and when leaving it 
(e.g. something similar to what devtools/client/shared/components/VisibilityHandler.js does)

- we should update the name to onPanelSelected
- we should also check that the selected panel is the accessibility panel, otherwise this is called for any panel we select
  (for instance with `this._toolbox.currentToolId === "accessibility"`)

::: devtools/client/accessibility/components/AccessibilityRow.js
@@ +87,5 @@
> +  }
> +
> +  scrollIntoViewIfNeeded() {
> +    let row = findDOMNode(this);
> +    row.scrollIntoView({ block: "center" });

This is unconditionally scrolling the element, not just "if needed", 
maybe we should change the method's name?

::: devtools/client/accessibility/components/Accessible.js
@@ +22,5 @@
> +const Tree = createFactory(require("devtools/client/shared/components/VirtualizedTree"));
> +// Reps
> +const { REPS, MODE } = require("devtools/client/shared/components/reps/reps");
> +const { Rep } = REPS;
> +const ElementNode = REPS.ElementNode;

nit: can be folded in previous line?

@@ +27,5 @@
> +
> +class AccessiblePropertyClass extends Component {
> +  static get propTypes() {
> +    return {
> +      acessible: PropTypes.string,

acessible -> accessible

@@ +67,5 @@
> +
> +class Accessible extends Component {
> +  static get propTypes() {
> +    return {
> +      ...Tree.propTypes,

Just as a comment, combining props like this prevents eslint from checking for missing propsTypes.
This is why it didn't spot accessible and dispatch which are missing here.

Not sure what we should do here.

(actually do we really pass VirtualizedTree props to Accessible?)

@@ +90,5 @@
> +    window.on(EVENTS.NEW_ACCESSIBLE_FRONT_INSPECTED, this.onAccessibleInspected);
> +  }
> +
> +  componentWillReceiveProps({ accessible }) {
> +    let oldAccessible = this.props.accessible;

accessible not declared in propTypes

@@ +121,5 @@
> +    }
> +  }
> +
> +  update() {
> +    let { dispatch, accessible } = this.props;

dispatch not declared in proptypes

@@ +170,5 @@
> +    let valueProps = { object, mode: MODE.TINY, title: "Object" };
> +    if (isNode(object)) {
> +      valueProps.defaultRep = ElementNode;
> +      valueProps.onDOMNodeMouseOut = () => this.hideHighlighter();
> +      valueProps.onDOMNodeMouseOver = () => this.showHighlighter(this.props.DOMNode);

DOMNode not defined in propTypes

@@ +183,5 @@
> +    return AccessibleProperty(
> +      { object, focused, acessible: this.props.accessible.actorID },
> +      () => div({
> +        className: classList.join(" "),
> +        style: { paddingLeft: depth * 15 },

This will not work in RTL, can we log a follow up to add RTL support for this panel?

@@ +200,5 @@
> +  }
> +
> +  render() {
> +    const { expanded, focused } = this.state;
> +    const { items, parents, accessible, labelledby } = this.props;

items, parents not defined in propTypes

::: devtools/client/accessibility/components/Button.js
@@ +2,5 @@
> + * 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/. */
> +"use strict";
> +
> +// Make this available to both AMD and CJS environments

We should probably drop that for a component which is specific to this panel? 
Unless you plan to move it to the shared components folder? (even so I'd prefer
 to change the file when it moves to /shared and not before)

::: devtools/client/accessibility/components/Toolbar.js
@@ +15,5 @@
> +
> +class Toolbar extends Component {
> +  static get propTypes() {
> +    return {
> +      dispatch: PropTypes.func,

dispatch isRequired as well (commenting here since isRequired have been added for the two other props
but other components are also missing isRequired for properties that are required)

@@ +57,5 @@
> +
> +  render() {
> +    let { canBeDisabled } = this.props;
> +    let { disabling } = this.state;
> +    let disableButtonStr = `accessibility.${disabling ? "disabling" : "disable"}`;

Can we avoid concatenating l10n keys? in the long run it makes the maintenance harder

disabling ? "accessibility.disabling" : "accessibility.disable";

We don't have a lot of tools to lint and check the quality of our l10n keys, so we rely
a lot on string search.

@@ +58,5 @@
> +  render() {
> +    let { canBeDisabled } = this.props;
> +    let { disabling } = this.state;
> +    let disableButtonStr = `accessibility.${disabling ? "disabling" : "disable"}`;
> +    let title = null;

I think there should be a title even when it is not disabled. 
The current label "ON" + the green color is also a bit misleading.
That can be handled in a follow up

::: devtools/client/accessibility/main.js
@@ +13,5 @@
> +  baseURI: "resource://devtools/client/accessibility/",
> +  window
> +}).require;
> +
> +XPCOMUtils.defineConstant(this, "require", require);

Is this needed? This `require` instance should already be available as a global in all files loaded using require().
If it can't be removed, can we add a comment explaining why it is needed here?

@@ +17,5 @@
> +XPCOMUtils.defineConstant(this, "require", require);
> +
> +// Localization
> +const { LocalizationHelper } = require("devtools/shared/l10n");
> +this.l10n = new LocalizationHelper("devtools/client/locales/accessibility.properties");

This seems to have been copied from the DOM panel which has very specific constraints. 
See my comment on utils.js

::: devtools/client/accessibility/provider.js
@@ +11,5 @@
> + * @param {Map}      accessibles accessibles object cache
> + * @param {Function} dispatch    react dispatch function that triggers a redux
> + *                               action.
> + */
> +function Provider(accessibles, dispatch) {

Any special reason to use function + prototype rather than a class here?

::: devtools/client/accessibility/test/head.js
@@ +48,5 @@
> +  return new Promise(resolve => {
> +    let observe = (subject, topic, data) => {
> +      Services.obs.removeObserver(observe, "a11y-init-or-shutdown");
> +
> +      if (data === "0") {

If data != "0" we remove the observer and never resolve? 
Looks like a timeout? Either move the removeObserver in the if or add comments (and logs) to clarify.

@@ +52,5 @@
> +      if (data === "0") {
> +        resolve();
> +      }
> +    };
> +    Services.obs.addObserver(observe, "a11y-init-or-shutdown");

Where is the shutdown coming from? The method calls GC/CC and then adds the observer, 
but never explicitly requests the shutdown of the accessibility service. 
Is this a side effect of GC/CC? Is this coming from somewhere else? 

Can you add a comment to clarify? Note, if it is linked to the "disableAccessibilityInspector" 
pushed at the end of every a11y test, it might make sense to call it as part of the cleanup instead.

@@ +104,5 @@
> +
> +function disableAccessibilityInspector(env) {
> +  let { doc, win } = env;
> +  EventUtils.synthesizeMouseAtCenter(
> +    doc.getElementById("accessibility-disable-button"), {}, win);

The type is not mandatory, when not specified simulates mousdown then mouseup:
https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/testing/mochitest/tests/SimpleTest/EventUtils.js#421-422

@@ +129,5 @@
> + */
> +async function checkTreeState(doc, expected) {
> +  info("Checking tree state.");
> +  await BrowserTestUtils.waitForCondition(() => {
> +    let notEqual = !![...doc.querySelectorAll(".treeRow")].find((row, i) =>

nit: Maybe it would be clearer by reducing the number of (!) here?
let hasExpectedStructure = [...doc.querySelectorAll(".treeRow")].every((row, i) => ...)
return hasExpectedStructure;

The ok(true, msg) might be easier to read as an info(msg) after the await waitForCondition as well.

@@ +289,5 @@
> +  executeSoon(() => target.activeTab.navigateTo(url));
> +  return once(target, waitForTargetEvent);
> +}
> +
> +async function selectNodeAndWaitForAnimations(data, inspector, reason = "test") {

Doesn't seem used in any test, can we remove it?

@@ +302,5 @@
> + * Open the inspector menu and return all of it's items in a flat array
> + * @param {InspectorPanel} inspector
> + * @return An array of MenuItems
> + */
> +async function openContextMenuAndGetAllItems(inspector) {

ditto

::: devtools/client/accessibility/utils.js
@@ +21,5 @@
> + * Note that Accessibility panel content can also run within a scope with no
> + * chrome privileges, e.g. in an iframe with type 'content' or in a browser tab,
> + * which allows using our own tools for development.
> + */
> +exports.l10n = window.l10n || DefaultL10N;

This file seems copied from the DOM panel, which used a special setup for enabling local development in a tab.
I don't think we want to invest too much in those alternative workflows anymore. We already have Launchpad for some 
tools, which we are obsoleting thanks to the progress made to reload quickly changes in Firefox. 

If you are not using the workflow, we should import LocalizationHelper regularly here (see devtools/client/netmonitor/src/utils/l10n.js for instance).
If you are using the workflow, we should probably discuss how we could improve the regular workflow to match your needs.

::: devtools/client/definitions.js
@@ +441,5 @@
>  
> +Tools.accessibility = {
> +  id: "accessibility",
> +  accesskey: l10n("accessibility.accesskey"),
> +  key: l10n("accessibility.commandkey"),

is this new `key` property used anywhere beyond `get tooltip()` ? None of the other tools define it so if it's only used locally I would prefer to skip it and keep things consistent.

@@ +446,5 @@
> +  ordinal: 14,
> +  modifiers: osString == "Darwin" ? "accel,alt" : "accel,shift",
> +  visibilityswitch: "devtools.accessibility.enabled",
> +  icon: "chrome://devtools/skin/images/tool-accessibility.svg",
> +  invertIconForDarkTheme: true,

invertIconForDarkTheme is no longer used/supported

::: devtools/client/locales/en-US/accessibility.properties
@@ +1,1 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public

Let's flag :flod for l10n review here.

@@ +44,5 @@
> +accessibility.enabling=Enabling Accessibility Features
> +
> +# LOCALIZATION NOTE (accessibility.disable): A title text for Disable
> +# accessibility button used to disable accessibility service.
> +accessibility.disable=ON

This label is very different from the action of the button, we should find something more explicit (can be done in a UX review follow up)

@@ +55,5 @@
> +# LOCALIZATION NOTE (accessibility.pick): A title text for Picker button
> +# button used to pick accessible objects from the page.
> +accessibility.pick=Pick accessible from the page
> +
> +# LOCALIZATION NOTE (accessibility.disableButtonTitle.enabled): A title

No longer matches the key accessibility.disableButtonTitle

@@ +60,5 @@
> +# text used for a tooltip for Disable accessibility button when accessibility
> +# service can not be disabled.
> +accessibility.disableButtonTitle=Accessibility service can not be turned off. It was started outside Developer Tools.
> +
> +# LOCALIZATION NOTE (accessibility.enableButtonTitle.disabled): A title

ditto

::: devtools/shim/devtools-startup.js
@@ +168,5 @@
>        modifiers
>      },
> +    // Key for opening the Accessibility Panel
> +    {
> +      toolId: "accessibility",

Can you log a follow up to add "accessibility" in the openedkey histogram for devtools at: https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/components/telemetry/Histograms.json#8350
Attachment #8960573 - Flags: review?(jdescottes)
Comment on attachment 8960573 [details] [diff] [review]
1447112 patch

Francesco, can you or someone from your team have a look at the new properties file?
Attachment #8960573 - Flags: review?(francesco.lodolo)
Comment on attachment 8960573 [details] [diff] [review]
1447112 patch

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

Leaving the flag open for now, I have a few questions (not many). The most important one is about "ON".

::: devtools/client/locales/en-US/accessibility.properties
@@ +40,5 @@
> +accessibility.enable=Enable Accessibility Features
> +
> +# LOCALIZATION NOTE (accessibility.enabling): A title text for Enable
> +# accessibility button used when accessibility service is being enabled.
> +accessibility.enabling=Enabling Accessibility Features

Isn't it weird for such a message to use title case? Also, not ellipsis at the end?

@@ +44,5 @@
> +accessibility.enabling=Enabling Accessibility Features
> +
> +# LOCALIZATION NOTE (accessibility.disable): A title text for Disable
> +# accessibility button used to disable accessibility service.
> +accessibility.disable=ON

This is definitely confusing (both ID and comment), and it's going to produce poor translations. We should fix it before exposing for localization.

If I understand it correctly: ON means that the accessibility tools are disable?

If needed to unblock, I'd suggest to hard-code this and expose the string once it's ready.

@@ +49,5 @@
> +
> +# LOCALIZATION NOTE (accessibility.disabling): A title text for Disable
> +# accessibility button used when accessibility service is being
> +# disabled.
> +accessibility.disabling=Turning Off

Same question about case and ellipsis

@@ +53,5 @@
> +accessibility.disabling=Turning Off
> +
> +# LOCALIZATION NOTE (accessibility.pick): A title text for Picker button
> +# button used to pick accessible objects from the page.
> +accessibility.pick=Pick accessible from the page

Should this string have a noun (object)?

@@ +58,5 @@
> +
> +# LOCALIZATION NOTE (accessibility.disableButtonTitle.enabled): A title
> +# text used for a tooltip for Disable accessibility button when accessibility
> +# service can not be disabled.
> +accessibility.disableButtonTitle=Accessibility service can not be turned off. It was started outside Developer Tools.

What does "started" means in this context? That information should be added to the comment.

@@ +68,5 @@
> +
> +# LOCALIZATION NOTE (accessibility.description.general): A title text used when
> +# accessibility service description is provided before accessibility inspector
> +# is enabled.
> +accessibility.description.general=Accessibility features are deactivated by default because they negatively impact Firefox performance. Consider turning off accessibility features before using other Developer Tools panels.

You don't want Firefox hard-coded in string. Use a placeholder (%S), and replace it at run-time with the brand name (also explain in the comment what replaces the placeholder).

@@ +73,5 @@
> +
> +# LOCALIZATION NOTE (accessibility.description.oldVersion): A title text used
> +# when accessibility service description is provided when a client is connected
> +# to an older version of accessibility actor.
> +accessibility.description.oldVersion=You are connected to debugger server that is too old. To use Accessibility panel, please connect to the latest debugger server version.

Not l10n, but should this say "…to A debugger server"?

::: devtools/client/locales/en-US/startup.properties
@@ +257,5 @@
> +accessibility.panelLabel=Accessibility Panel
> +
> +# LOCALIZATION NOTE (accessibility.accesskey)
> +# Used for the menuitem in the tool menu
> +accessibility.accesskey=0

Is this access key used for accessibility.label? If that's the case, it should be moved near the label, and you can drop the comment.

If it's used for more than one label, it's a problem.
(In reply to Yulia Startsev [:yulia] from comment #7)

Thanks for the review, all comments will be addressed, one question answered below:

> Comment on attachment 8960573 [details] [diff] [review]
> 1447112 patch
> 
> Review of attachment 8960573 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: devtools/client/accessibility/test/head.js
> @@ +104,5 @@
> > +
> > +function disableAccessibilityInspector(env) {
> > +  let { doc, win } = env;
> > +  EventUtils.synthesizeMouseAtCenter(
> > +    doc.getElementById("accessibility-disable-button"), {}, win);
> 
> I'm not really familiar with EventUtils, but i thought the second argument
> needed a type? mousedown or similar? I am probably wrong about this

From the EventUtils docs:
...
* If the type is specified, an mouse event of that type is fired. Otherwise,
* a mousedown followed by a mouse up is performed.
...
Here we want to simulate the events that otherwise happen on click (so the whole sequence).
(In reply to Julian Descottes [:jdescottes][:julian] from comment #8)
> Comment on attachment 8960573 [details] [diff] [review]
> 1447112 patch
> 
> Review of attachment 8960573 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: devtools/client/accessibility/accessibility-panel.js
> @@ +112,5 @@
> > +  /**
> > +   * Make sure the panel is refreshed (if needed) when it's selected.
> > +   */
> > +  onPanelVisibilityChange() {
> > +    this._opening.then(() => this.refresh());

This refresh checks if current panel is "accessibility" in isVisible method and does different things depending on what the answer is. Hope that's alright?

> 
> The method name is misleading, I assumed this was called when selecting
> _this_ panel and when leaving it 
> (e.g. something similar to what
> devtools/client/shared/components/VisibilityHandler.js does)
> 
> - we should update the name to onPanelSelected
> - we should also check that the selected panel is the accessibility panel,
> otherwise this is called for any panel we select
>   (for instance with `this._toolbox.currentToolId === "accessibility"`)
> 


> ::: devtools/shim/devtools-startup.js
> @@ +168,5 @@
> >        modifiers
> >      },
> > +    // Key for opening the Accessibility Panel
> > +    {
> > +      toolId: "accessibility",
> 
> Can you log a follow up to add "accessibility" in the openedkey histogram
> for devtools at:
> https://searchfox.org/mozilla-central/rev/
> 49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/components/telemetry/
> Histograms.json#8350

Yes this is going to be part of bug 1447302, and will land after this one.
(In reply to Francesco Lodolo [:flod] (workweek until Mar 30, slower than usual) from comment #10)
> Comment on attachment 8960573 [details] [diff] [review]
> 1447112 patch
> 
> Review of attachment 8960573 [details] [diff] [review]:
> -----------------------------------------------------------------

All should be fixed + see below:
> ::: devtools/client/locales/en-US/startup.properties
> @@ +257,5 @@
> > +accessibility.panelLabel=Accessibility Panel
> > +
> > +# LOCALIZATION NOTE (accessibility.accesskey)
> > +# Used for the menuitem in the tool menu
> > +accessibility.accesskey=0
> 
> Is this access key used for accessibility.label? If that's the case, it
> should be moved near the label, and you can drop the comment.
> 
> If it's used for more than one label, it's a problem.

It is similar to all other panels in that file: *.accesskey is used to populate the accesskey bit for the panel list item in the Tools -> Web Developer menu (top level).
Attached patch 1447112 patch v2 (obsolete) — Splinter Review
Addressed everyone's comments, thanks! Apologies if I missed any. Julian, hope it's alright to follow up with dir restructuring? Thanks.
Attachment #8960573 - Attachment is obsolete: true
Attachment #8960573 - Flags: review?(pbrosset)
Attachment #8960573 - Flags: review?(francesco.lodolo)
Attachment #8963171 - Flags: review?(pbrosset)
Attachment #8963171 - Flags: review?(jdescottes)
Attachment #8963171 - Flags: review?(francesco.lodolo)
(In reply to Yura Zenevich [:yzen] from comment #14)
> Created attachment 8963171 [details] [diff] [review]
> 1447112 patch v2
> 
> Addressed everyone's comments, thanks! Apologies if I missed any. Julian,
> hope it's alright to follow up with dir restructuring? Thanks.

Yes follow up is fine. If we want to go through with it, we can have a refactor bug to align all react panels. I don't think it's something you necessarily have to tackle right after landing this.
Comment on attachment 8963171 [details] [diff] [review]
1447112 patch v2

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

::: devtools/client/locales/en-US/accessibility.properties
@@ +40,5 @@
> +accessibility.enable=Turn On Accessibility Features
> +
> +# LOCALIZATION NOTE (accessibility.enabling): A title text for Enable
> +# accessibility button used when accessibility service is being enabled.
> +accessibility.enabling=Turning on accessibility features...

Use a single unicode character for ellipsis (…)

@@ +49,5 @@
> +
> +# LOCALIZATION NOTE (accessibility.disabling): A title text for Disable
> +# accessibility button used when accessibility service is being
> +# disabled.
> +accessibility.disabling=Turning off accessibility features...

Same here (… vs ...)

::: devtools/client/locales/en-US/startup.properties
@@ +257,5 @@
> +accessibility.panelLabel=Accessibility Panel
> +
> +# LOCALIZATION NOTE (accessibility.accesskey)
> +# Used for the menuitem in the tool menu
> +accessibility.accesskey=0

Still open questions here: is this an accesskey or a shortcut (commandkey)? If it's an actual access key, it should be a character available in the label.
Comment on attachment 8963171 [details] [diff] [review]
1447112 patch v2

Here's a try build https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6c481f99bf9e65c2165629754d69645b2f21764 or you can just apply a patch if that's easier.
Attachment #8963171 - Flags: a11y-review?(victoria)
Attached image Screen Shot (obsolete) —
(In reply to Francesco Lodolo [:flod] (workweek until Mar 30, slower than usual) from comment #16)
> Comment on attachment 8963171 [details] [diff] [review]
> 1447112 patch v2
> 
> Review of attachment 8963171 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: devtools/client/locales/en-US/startup.properties
> @@ +257,5 @@
> > +accessibility.panelLabel=Accessibility Panel
> > +
> > +# LOCALIZATION NOTE (accessibility.accesskey)
> > +# Used for the menuitem in the tool menu
> > +accessibility.accesskey=0
> 
> Still open questions here: is this an accesskey or a shortcut (commandkey)?
> If it's an actual access key, it should be a character available in the
> label.

It is the actual accesskey but is not related to accessibility.label property. It is used to populate a browser menu (see screenshot for accessibility menu item) that indeed displays the accesskey on the right hand side. 

accessibility.label is used for the text of the DevTools tab. Hopefully, I understood your concern correctly.
Attached patch 1447112 patch v3Splinter Review
Updated to the right ellipsis.
Attachment #8963171 - Attachment is obsolete: true
Attachment #8963171 - Flags: review?(pbrosset)
Attachment #8963171 - Flags: review?(jdescottes)
Attachment #8963171 - Flags: review?(francesco.lodolo)
Attachment #8963171 - Flags: a11y-review?(victoria)
Attachment #8963445 - Flags: ui-review?(victoria)
Attachment #8963445 - Flags: review?(pbrosset)
Attachment #8963445 - Flags: review?(jdescottes)
Attachment #8963445 - Flags: review?(francesco.lodolo)
(In reply to Yura Zenevich [:yzen] from comment #18)
> Created attachment 8963293 [details]
> Screen Shot
> 
> (In reply to Francesco Lodolo [:flod] (workweek until Mar 30, slower than
> usual) from comment #16)
> > Comment on attachment 8963171 [details] [diff] [review]
> > 1447112 patch v2
> > 
> > Review of attachment 8963171 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: devtools/client/locales/en-US/startup.properties
> > @@ +257,5 @@
> > > +accessibility.panelLabel=Accessibility Panel
> > > +
> > > +# LOCALIZATION NOTE (accessibility.accesskey)
> > > +# Used for the menuitem in the tool menu
> > > +accessibility.accesskey=0
> > 
> > Still open questions here: is this an accesskey or a shortcut (commandkey)?
> > If it's an actual access key, it should be a character available in the
> > label.
> 
> It is the actual accesskey but is not related to accessibility.label
> property. It is used to populate a browser menu (see screenshot for
> accessibility menu item) that indeed displays the accesskey on the right
> hand side. 
> 
> accessibility.label is used for the text of the DevTools tab. Hopefully, I
> understood your concern correctly.

I believe what Francesco meant is that the access key is usually a letter from the menu item's label itself, which you then can press while the menu is open to quickly activate the item. In Windows, these once used to be underlined. For example the F in File, E in Edit etc.

So for the accessibility inspector, one expects the access key to be either one of a, c, e, s, i b l, t, or y. These are also localizable, so whichever the translators choose will then become the access key in the respective other language.

This is in contrast to the shortcut, like ctrl+shift+0, which can be pressed from anywhere even when menus are closed.
(In reply to Marco Zehe (:MarcoZ) from comment #20)
> I believe what Francesco meant is that the access key is usually a letter
> from the menu item's label itself, which you then can press while the menu
> is open to quickly activate the item. In Windows, these once used to be
> underlined. For example the F in File, E in Edit etc.
> 
> So for the accessibility inspector, one expects the access key to be either
> one of a, c, e, s, i b l, t, or y. These are also localizable, so whichever
> the translators choose will then become the access key in the respective
> other language.
> 
> This is in contrast to the shortcut, like ctrl+shift+0, which can be pressed
> from anywhere even when menus are closed.

Exactly. Also, on Windows and Linux, this will show up as "Accessibility (0)"
Comment on attachment 8963445 [details] [diff] [review]
1447112 patch v3

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

This new version looks good to me! Thanks for addressing the comments. Also had a look at the mochitests this time and they look good.

::: devtools/client/accessibility/test/browser_accessibility_reload.js
@@ +78,5 @@
> +  }
> +}];
> +
> +/**
> + * Simple test that checks content of the Accessibility panel tree.

nit: comment is copied from browser_accessibility_tree.js
Attachment #8963445 - Flags: review?(jdescottes) → review+
Attached patch 1447112 patch v4Splinter Review
Addressed Julian's comment. "y" does not seem to be used to I am updating it to that character.
Attachment #8963293 - Attachment is obsolete: true
Attachment #8963445 - Attachment is obsolete: true
Attachment #8963445 - Flags: ui-review?(victoria)
Attachment #8963445 - Flags: review?(pbrosset)
Attachment #8963445 - Flags: review?(francesco.lodolo)
Attachment #8963584 - Flags: ui-review?(victoria)
Attachment #8963584 - Flags: review?(pbrosset)
Attachment #8963584 - Flags: review?(francesco.lodolo)
Comment on attachment 8963445 [details] [diff] [review]
1447112 patch v3

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

As discussed on Slack, I did not review the code itself, because I trust others to do a better job than me on this particular patch, but I did take the UI for a spin. Here are some comments:

- The list of properties in the sidebar could use a higher line-height and better vertical alignment. The line height should be the same as in the tree panel (and consistent with other tools, like the sidebar of the debugger, or the storage inspector).
- In the sidebar again, when a row with an expander icon is selected (blue), then the expander icon should turn white, so it's more easily visible.
- Also, the expander icon isn't vertically aligned with the text.
- At some stage, I managed to get 2 horizontal scrollbars in the sidebar. Note that I'm on Windows 10 and using the light theme (so, got big scrollbars that are always visible). In fact the vertical scrollbar to scroll through the properties is hidden for me, I have to scroll right to see it and grab it. I think there shouldn't be a horizontal scrollbar, but text should wrap when necessary instead.
- It feels a bit odd having the text-selection cursor style when hovering over property names in the sidebar. If we want them to be selectable/copyable, I guess that's fine, but otherwise, having the normal cursor would look better.
- If you pause the debugger, and return to the accessibility panel, you can't pick accessible objects from the page anymore. That's expected because pausing the debugger suppresses all content events, so we can't detect mouse events on the page. We usually have a warning banner for this (at least we do when you switch to the inspector). We should do the same here.
- As size gets reduced in the tree panel, the 2 columns get squished, and at some stage, the name column starts overlapping the role column. If you make them really small, you can see the roles start to become hidden (like you'd only see the beginning of listitem for instance), but the name remains fully visible. I tend to think we should do the opposite. The role part is, to me, the more important part because it's what gives you the sense for this thing being a tree at all. Indeed, that's the part that looks indented. While the name is just a flat column. So when there isn't enough space, you lose the ability to see the tree structure. Perhaps we should add an ellipsis to the name column instead, and introduce a horizontal scrollbar after some point.
- The tree jumps up and down when you expand an item. It looks like it does so to make the selected item be at the center of the tree. I think this can be seen as a bit jarring from users. It may be enough for someone to lose their context.
- We might need to work a little bit on making keyboard navigation better. I see a couple issues there: holding arrowDown pressed seems to have bad performance. If you try it on a long list of items, like the items inside a combobox for a list of countries. After a second or so, the selection doesn't go down at all, until you stop pressing down on the key. And, second, I would tend to think we want exactly the same keys to work in the same way as in the inspector. So for instance, pressing arrowLeft should go back up and collapse when needed. And arrowRight should expand (which it does) and then go down if it can. Muscle memory is important to take into account here.

Ok that's it for now. Other than those minor UX things, the panel works really well and I can see it being very useful for people who know (or want to know) what the accessibility tree is.
I think these can be handled in other bugs later. I don't really see a big blocker here. But Victoria will do a more thorough UI review I guess.

I'm clearing the review flag because this isn't a review. I see both Yulia and Julian have already R+'d the code, so we should be good there. Just wanted to provide some feedback on the UI.
Attachment #8963445 - Attachment is obsolete: false
Oh, there was a mid-air collision. This comment was about the v3 patch, I haven't tested v4 locally. Let me know if you'd like me to.
(In reply to Patrick Brosset <:pbro> from comment #25)
> Oh, there was a mid-air collision. This comment was about the v3 patch, I
> haven't tested v4 locally. Let me know if you'd like me to.

New changes only included comment updates and .properties file.
Comment on attachment 8963584 [details] [diff] [review]
1447112 patch v4

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

Thanks, l10n bits look good now. I'm not sure you need to use the same letter for accesskey and shortcut, but in case that's a DevTools decision.
Attachment #8963584 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8963584 [details] [diff] [review]
1447112 patch v4

Clearing Patrick's r? from v4 since he gave feedback on v3.
Attachment #8963584 - Flags: review?(pbrosset)
Comment on attachment 8963584 [details] [diff] [review]
1447112 patch v4

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

::: devtools/client/accessibility/accessibility.css
@@ +23,5 @@
> +  padding: 0;
> +  width: 100%;
> +}
> +
> +:root:not(.no-animate) .flash-out {

This looks copied from the storage inspector, are you using the `no-animate` class ?

@@ +59,5 @@
> +}
> +
> +.mainFrame .devtools-button > .btn-content,
> +.description .devtools-button > .btn-content {
> +  padding: 2px var(--accessibility-horizontal-padding);

Why is this needed ?

@@ +69,5 @@
> +  border: 1px solid transparent;
> +  outline: none;
> +}
> +
> +.devtools-button:focus > .btn-content:not(.devtools-throbber) {

These kind of focus styles should be global to devtools, not specific to the a11y inspector
Comment on attachment 8963584 [details] [diff] [review]
1447112 patch v4

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

Hi Yura, this is looking great. Exciting to know that this will land soon! Here are my comments:

Welcome panel:
The text/buttons look too big - I know bigger text is generally better a11y, but in this case it should match the rest of DevTools, as the user can use Cmd-+ to make the text bigger. The button should match the button in Performance and Memory panels with smaller text and a 1px gray border.
For the img, I'd recommend a smaller size of flex-basis: 42px;
For .description .general, I would request the equivalent of font-size: 13px; and margin-bottom 1.7em for more spacing between the p and the button.

Activated panel:
- the subtle gray background for the deactivate button seems like a good solution - I was suggesting adding an icon instead but this way is fine for now
- Patrick's comments sound good to me - matching line-height between the two panes would be especially nice, and would be great to have the selected row arrow match the Inspector's whitish expander arrows. I like the idea of not hiding the role column. The way the name column currently wraps onto another row seems fine, though I would be on board with the idea for truncation by scrollbar if there will often be a lot of text in the name column.

Problems for later, probably :) -
- Soon it would be fantastic to remove the dotted outline focus ring around the panels and try the "gray for unfocused selected row, blue for focused selected row for focused-panel indication" idea.
- I associate the square/reticule icon with "select this element in the page" rather than 'jump to inspector'  (e.g. how it works in Grid Inspector - it might make sense to use the 'jump to debugger' icon or a variant thereof

Bugzilla's UI seems different - hopefully I posted this review in the right place :)
Attachment #8963584 - Flags: ui-review?(victoria) → ui-review+
(In reply to Tim Nguyen :ntim from comment #29)
> Comment on attachment 8963584 [details] [diff] [review]
> 1447112 patch v4
> 
> Review of attachment 8963584 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/accessibility/accessibility.css
> @@ +23,5 @@
> > +  padding: 0;
> > +  width: 100%;
> > +}
> > +
> > +:root:not(.no-animate) .flash-out {
> 
> This looks copied from the storage inspector, are you using the `no-animate`
> class ?

Good catch.

> 
> @@ +59,5 @@
> > +}
> > +
> > +.mainFrame .devtools-button > .btn-content,
> > +.description .devtools-button > .btn-content {
> > +  padding: 2px var(--accessibility-horizontal-padding);
> 
> Why is this needed ?

This was to ensure that the inner element of the button takes as much space as there's available.

> 
> @@ +69,5 @@
> > +  border: 1px solid transparent;
> > +  outline: none;
> > +}
> > +
> > +.devtools-button:focus > .btn-content:not(.devtools-throbber) {
> 
> These kind of focus styles should be global to devtools, not specific to the
> a11y inspector

This sounds like a follow up.
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4589600b540b
added front end parts of accessibility inspector tool. r=pbro, yulia, jdescottes, flod
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fac38e508661
Fix for browser/base/content/test/static/browser_parsable_css.js | custom property --accessibility-header-font is not referenced. CLOSED TREE
(In reply to Yura Zenevich [:yzen] from comment #31)
> > @@ +69,5 @@
> > > +  border: 1px solid transparent;
> > > +  outline: none;
> > > +}
> > > +
> > > +.devtools-button:focus > .btn-content:not(.devtools-throbber) {
> > 
> > These kind of focus styles should be global to devtools, not specific to the
> > a11y inspector
> 
> This sounds like a follow up.


Could you please file a bug in this case ?
https://hg.mozilla.org/mozilla-central/rev/4589600b540b
https://hg.mozilla.org/mozilla-central/rev/fac38e508661
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.