1473030 - Show accessible object's name and role information with info-bar highlight.

Assignee

Description

•

6 years ago

Attached image InfobarExample.png — Details

We want to display an accessible object's name and role information in the info-bar when highlighted. This will serve as a starting point for what sorts of information can be shown as part the accessible highlighter. 

See attached screenshot for an example.

Micah [:mtigley] (she/her)

Assignee

Updated

•

6 years ago

Assignee: nobody → mtigley

Micah [:mtigley] (she/her)

Assignee

Updated

•

6 years ago

Status: NEW → ASSIGNED

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

Updated

•

6 years ago

Priority: -- → P3

Comment hidden (mozreview-request)

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

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

Comment 2

•

6 years ago

mozreview-review

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review263402

Good work! See comments, mostly small improvements.

One issue I found: when using Browser Toolbox and hovering over accessible objects in the tree that do not have valid bounds the infobar still shows in the top left corner of the window.

::: devtools/server/actors/accessibility.js:394
(Diff revision 1)
> +    this.onHighlighterEvent = this.onHighlighterEvent.bind(this);
>  
>      this.highlighter = CustomHighlighterActor(this, isXUL(this.rootWin) ?
>        "XULWindowAccessibleHighlighter" : "AccessibleHighlighter");
> +
> +    this.highlighter.on("highlighter-event", this.onHighlighterEvent);

OK, I think walker managing highlighter is the way to go since we are emitting events on an actor that is not transferred over the wire to the client.

after highlighter is initalized, you should call
```
this.manage(this.highlighter); 
```
which adds the highglihger actor to its pool of actors and assigns the actorID

::: devtools/server/actors/accessibility.js:447
(Diff revision 1)
>    destroy() {
>      Actor.prototype.destroy.call(this);
>  
>      this.reset();
>  
> +    this.highlighter.removeEventListener("highlighter-event", this.onHighlighterEvent);

removeEventListener -> off

::: devtools/server/actors/accessibility.js:448
(Diff revision 1)
>      Actor.prototype.destroy.call(this);
>  
>      this.reset();
>  
> +    this.highlighter.removeEventListener("highlighter-event", this.onHighlighterEvent);
>      this.highlighter.destroy();

Since we now manage the highlighter (it's in walker's pool of actors), it will be destroyed when walker actor is destroyed. We do not need to call this.highlighter.destroy() any more here.

Also, double check if we successfully remove the event listener even after the the highligher's destroy method was called.

::: devtools/server/actors/highlighters/accessible.js:41
(Diff revision 1)
>   *           height of the the accessible object
>   *         - {Number} duration
>   *           Duration of time that the highlighter should be shown.
> + *         - {String | null} name
> + *           name of the the accessible object
> + *         - {String | null} role

Can't be null.

::: devtools/server/actors/highlighters/accessible.js:45
(Diff revision 1)
> + *           name of the the accessible object
> + *         - {String | null} role
> + *           role of the the accessible object
>   *
>   * Structure:
>   * <div class="highlighter-container">

It makes sense for now to stick aria-hidden on the top level highlighter-container and remove role presentation, and aria-hidden from anywhere else

::: devtools/server/actors/highlighters/accessible.js:129
(Diff revision 1)
>        prefix: this.ID_CLASS_PREFIX
>      });
>  
> +    // Build the accessible's infobar markup
> +    const accessibleInfobarContainer = createNode(this.win, {
> +      parent: container,

In the function doc, the parent of the infobar container is root, instead of highlighter-container.

::: devtools/server/actors/highlighters/accessible.js:246
(Diff revision 1)
>  
>      if (this._updateAccessibleBounds()) {
>        this._showAccessibleBounds();
> +
> +      // Show accessible's infobar
> +      this._showAccessibleInfobar();

Wouldn't we want to first update the inforbar and then show it?

::: devtools/server/actors/highlighters/accessible.js:297
(Diff revision 1)
> +
> +  /**
> +   * Hide the accessible infobar information content.
> +   */
> +  _hideAccessibleContent() {
> +    this.getElement("infobar-name").setAttribute("hidden", "true");

Not entirely sure why this is necessary. I'd expect we can get away with just having _hideAccessibleContent and _showAccessibleInfobar

::: devtools/server/actors/highlighters/accessible.js:349
(Diff revision 1)
> +   * Update accessible information displayed for current accessible's infobar.
> +   *
> +   */
> +  _updateAccessibleInfobar() {
> +    // Refresh infobar content.
> +    this._hideAccessibleContent();

If update happens before show, this might not be necessary?

::: devtools/server/actors/highlighters/accessible.js:353
(Diff revision 1)
> +    // Refresh infobar content.
> +    this._hideAccessibleContent();
> +
> +    // Initially, show only the accessible's role and name. In future patches,
> +    // we will be able to show messages for contrast ratio and more.
> +    this.showAccessibleRole();

I would rename these two methods to update... instead of show

::: devtools/server/actors/highlighters/accessible.js:356
(Diff revision 1)
> +    // Initially, show only the accessible's role and name. In future patches,
> +    // we will be able to show messages for contrast ratio and more.
> +    this.showAccessibleRole();
> +    this.showAccessibleNameInfo();
> +
> +    const container = this.getElement("infobar-container");

I would move everything from this line to the end into a separate function (similar to box model). Also see box model how infobar is moved when scroll position changes. I think this is relevant in our case here too.

::: devtools/server/actors/highlighters/accessible.js:390
(Diff revision 1)
> +
> +  /**
> +   * Show the accessible's name message.
> +   */
> +  showAccessibleNameInfo() {
> +    const Check = new AccessibleChecker(this.options);

nit: Check should not be capitalised.

::: devtools/server/actors/highlighters/accessible.js:391
(Diff revision 1)
> +  /**
> +   * Show the accessible's name message.
> +   */
> +  showAccessibleNameInfo() {
> +    const Check = new AccessibleChecker(this.options);
> +    const { warning, message: nameMessage } = Check.evaluateAccessibleName();

Is there a reason to reassign message to nameMessage?
Also, as far as I can see, warning is actually named hasWarning in the checker.

::: devtools/server/actors/highlighters/accessible.js:394
(Diff revision 1)
> +  showAccessibleNameInfo() {
> +    const Check = new AccessibleChecker(this.options);
> +    const { warning, message: nameMessage } = Check.evaluateAccessibleName();
> +    let nameText =  nameMessage;
> +
> +    if (warning) {

I think it will be safer to not include the checker in this patch. Lets wait until we have something we actually implement in it.

::: devtools/server/actors/highlighters/accessible.js:395
(Diff revision 1)
> +    const Check = new AccessibleChecker(this.options);
> +    const { warning, message: nameMessage } = Check.evaluateAccessibleName();
> +    let nameText =  nameMessage;
> +
> +    if (warning) {
> +      this.getElement("infobar-name").setAttribute("style", "color: #DCDDA4");

This should be in a class instead of a style attribute.

::: devtools/server/actors/highlighters/accessible.js:402
(Diff revision 1)
> +    } else {
> +      this.getElement("infobar-name").removeAttribute("style");
> +    }
> +
> +    this.getElement("infobar-name").setTextContent(nameText);
> +    this.getElement("infobar-name").removeAttribute("hidden");

Hopefully this will not be necessary with suggestions about the order of update/visibility above.

::: devtools/server/actors/highlighters/accessible.js:410
(Diff revision 1)
> +  /**
> +   * Show the accessible's role.
> +   */
> +  showAccessibleRole() {
> +    const { role } = this.options;
> +    const roleText = role ? `${role}` : "";

I don't believe we can have an accesible with no role. So
```
this.getElement("infobar-role").setTextContent(`${this.options.role}`);
```
should suffice

::: devtools/server/actors/highlighters/utils/accessibility.js:74
(Diff revision 1)
> + *         The arrow tip element to indicate infobar direction.
> + * @param  {Object} bounds
> + *         The bounds information of accessible object.
> + *
> + */
> +function moveInfobar(win, container, arrowEl, bounds) {

We should probably keep the order of args as consistent with the moveInfobar in markup as possible.

::: devtools/server/actors/highlighters/utils/accessibility.js:83
(Diff revision 1)
> +  const {
> +    left: boundsLeft,
> +    right: boundsRight,
> +    top: boundsTop,
> +    bottom: boundsBottom} = bounds;
> +  const arrowSize = 8;

This would be more consistent if we took that out as a CSS variable similar to --highlighter-bubble-arrow-size. 8px is referenced in many places so reusing that value would be very helpful. (I think it's possible to do arithmetic with the value of "8px" if you use calc())

::: devtools/server/actors/highlighters/utils/accessibility.js:141
(Diff revision 1)
> + * @param  {Number} maxLength
> + *         The number of characters to truncate with. Default is 50 chars.
> + *
> + * @return {String} The truncated text.
> + */
> +function truncateText(str, maxLength = 50) {

Lets use truncateString from devtools/client/inspector/markup/utils.js which means you need to move this utility into shared folder and put it somewhere meaningful

::: devtools/server/actors/highlighters/utils/accessibility.js:155
(Diff revision 1)
> +
> +/**
> + * The AccessibleChecker generates information about an accessible's options for
> + * displaying.
> + */
> +class AccessibleChecker {

Lets not include this in this patch.

::: devtools/server/actors/highlighters/xul-accessible.js:34
(Diff revision 1)
> +  }
> +
> +  .accessible-infobar {
> +    position: relative;
> +    left: -50%;
> +    background-color: hsl(214, 13%, 24%);

Use css variable for this value

::: devtools/server/actors/highlighters/xul-accessible.js:43
(Diff revision 1)
> +    padding: 5px;
> +  }
> +
> +  .arrow {
> +    position: absolute;
> +    width: 0; 

nit: here and below, remove extra trailing white space

::: devtools/server/actors/highlighters/xul-accessible.js:45
(Diff revision 1)
> +
> +  .arrow {
> +    position: absolute;
> +    width: 0; 
> +    height: 0; 
> +    border-left: 8px solid transparent;

8px seems to be a special number everywhere, lets take it out as a variable, (also seen in moveInforbar)

::: devtools/server/actors/highlighters/xul-accessible.js:137
(Diff revision 1)
> +
> +    this.infobarContainer = createNode(this.win, {
> +      parent: this.container,
> +      attributes: {
> +        "class": "accessible-infobar-container",
> +        "aria-hidden": "true"

Same here, lets put aria-hidden true on container, this way we wont need any presentation or aria-hidden attributes anywhere.

::: devtools/server/actors/highlighters/xul-accessible.js:215
(Diff revision 1)
>     *           height of the the accessible object
>     *         - duration {Number}
>     *                    Duration of time that the highlighter should be shown.
> +   *         - {String | null} name
> +   *           name of the the accessible object
> +   *         - {String | null} role

Can't be null

::: devtools/server/actors/highlighters/xul-accessible.js:324
(Diff revision 1)
>    }
>  
>    /**
> +   * Hide infobar's info
> +   */
> +  _hideAccessibleContent() {

Same comments here regarding special handling of the name part. I'd expect we can get away with just showing higing the infobar at the appropriate time.

::: devtools/server/actors/highlighters/xul-accessible.js:349
(Diff revision 1)
> +    const boundsMidPoint = (left + right) / 2;
> +
> +    infobarEl.style.left = `${boundsMidPoint}px`;
> +
> +    // Set the text content of the infobar.
> +    this.showAccessibleRole();

same , lets rename to update..

::: devtools/server/actors/highlighters/xul-accessible.js:378
(Diff revision 1)
>    }
> +
> +  /**
> +   * Show the accessible's name message.
> +   */
> +  showAccessibleNameInfo() {

See comments from regular accessible highlighter

::: devtools/server/actors/highlighters/xul-accessible.js:397
(Diff revision 1)
> +  }
> +
> +  /**
> +   * Show the accessible's role.
> +   */
> +  showAccessibleRole() {

See comments from regular accessible highlighter

::: devtools/server/tests/browser/browser_accessibility_highlighter_infobar.js:19
(Diff revision 1)
> +
> +   const a11yWalker = await accessibility.getWalker();
> +   await accessibility.enable();
> +
> +   info("Button front checks");
> +   const buttonNode = await walker.querySelector(walker.rootNode, "#button");

Part from here to the end of role and name checks could be taken out as a function since these bits are identical for both nodes.

::: devtools/server/tests/browser/browser_accessibility_highlighter_infobar.js:25
(Diff revision 1)
> +   const accessibleButtonFront = await a11yWalker.getAccessibleFor(buttonNode);
> +   let { name, role } = accessibleButtonFront;
> +   let onHighlightEvent = a11yWalker.once("highlighter-event");
> +
> +   await a11yWalker.highlightAccessible(accessibleButtonFront);
> +   let response = await onHighlightEvent;

nit: just let {options} =...

::: devtools/server/tests/browser/browser_accessibility_highlighter_infobar.js:30
(Diff revision 1)
> +   let response = await onHighlightEvent;
> +   let options = response.options;
> +   is(options.name, name, "Button highlight has correct name option");
> +   is(options.role, role, "Button highlight has correct role option");
> +
> +   const ButtonChecker = new AccessibleChecker(options);

Lets skip all the checker stuff.

::: devtools/server/tests/browser/browser_accessibility_highlighter_infobar.js:39
(Diff revision 1)
> +   is(buttonMessage, "\"Accessible Button\"", "Button accessible has correct name.");
> +   is(buttonWarning, false, "Button name's message is not a warning.");
> +
> +   info("Front with long name checks");
> +   const headerNode =
> +    await walker.querySelector(walker.rootNode, "#h1");

nit: keep declaration on the same line.

::: devtools/server/tests/browser/browser_accessibility_highlighter_infobar.js:52
(Diff revision 1)
> +   response = await onHighlightEvent;
> +   options = response.options;
> +   is(options.name, name, "Header highlight has correct name option");
> +   is(options.role, role, "Header highlight has correct role option");
> +
> +   const HeaderChecker = new AccessibleChecker(options);

lets skip the checker.

::: devtools/server/tests/browser/browser_accessibility_highlighter_infobar.js:56
(Diff revision 1)
> +
> +   const HeaderChecker = new AccessibleChecker(options);
> +   const { hasWarning: h1Warning, message: h1Message } =
> +   HeaderChecker.evaluateAccessibleName();
> +
> +   is(h1Message, "\"Lorem ipsum dolor sit ame...e et dolore magna aliqua.\"",

To test truncation you can just import utils, I believe.

Attachment #8991082 - Flags: review?(yzenevich)

Micah [:mtigley] (she/her)

Assignee

Comment 3

•

6 years ago

mozreview-review-reply

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review263402

Thanks for the review!

I think we discussed some of these accessible objects probably not having a "aria-hidden: true" attribute on them and only the "hidden" property, which can be an accessibility concern depending on the role of the object. If this is the case, should we flag this with the info-bar? Or just hide the info-bar for now until we address it in another patch?

> OK, I think walker managing highlighter is the way to go since we are emitting events on an actor that is not transferred over the wire to the client.
> 
> after highlighter is initalized, you should call
> ```
> this.manage(this.highlighter); 
> ```
> which adds the highglihger actor to its pool of actors and assigns the actorID

Sounds good, I will make sure to mention this in Bug 1474391 - Accessible highlighter does not emit "highlighter-event".

> Wouldn't we want to first update the inforbar and then show it?

`this._showAccessibleInfobar` removes the "hidden" attribute to the info-bar node, which makes its computed style properties available as a result. When `this._updateAccessibleInfobar` is called the `moveInfobar` needs these computed style properties to properly position the info-bar.  Trying to update first causes the container to be positioned incorrectly

> Not entirely sure why this is necessary. I'd expect we can get away with just having _hideAccessibleContent and _showAccessibleInfobar

We would need `_hideAccessibleInfobar` since it hides the infobar container when the accessible is unhighlighted. 

I think for now we can remove `_hideAccessibleContent` since the infobar name would be already hidden by `_hideAccessibleInfobar`

Comment hidden (mozreview-request)

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256082/diff/1-2/

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

Comment 5

•

6 years ago

mozreview-review

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review264396

Mostly complete, some nits below. I did find another issue were (in browser toolbox) if you highlight something valid and then after highlight a no-bounds accessible (via a11y tree) the infobar will still show up, but with data related to the last valid accessible.

::: devtools/server/actors/accessibility.js:393
(Diff revisions 1 - 2)
>      this.onKey = this.onKey.bind(this);
>      this.onHighlighterEvent = this.onHighlighterEvent.bind(this);
>  
>      this.highlighter = CustomHighlighterActor(this, isXUL(this.rootWin) ?
>        "XULWindowAccessibleHighlighter" : "AccessibleHighlighter");
>  

nit: no need for empty line here

::: devtools/server/actors/accessibility.js:395
(Diff revisions 1 - 2)
>  
>      this.highlighter = CustomHighlighterActor(this, isXUL(this.rootWin) ?
>        "XULWindowAccessibleHighlighter" : "AccessibleHighlighter");
>  
> +    this.manage(this.highlighter);
> +

nit: no need for empty line here

::: devtools/server/actors/highlighters.css:661
(Diff revisions 1 - 2)
>    border-inline-start: 1px solid #5a6169;
>    margin-inline-start: 6px;
>    padding-inline-start: 6px;
>  }
>  
> +:-moz-native-anonymous .accessible-infobar-name-warning {

this isn't used in this patch, lets remove.

::: devtools/server/actors/highlighters/accessible.js:51
(Diff revisions 1 - 2)
> - * <div class="highlighter-container">
> + * <div class="highlighter-container" aria-hidden="true">
>   *   <div class="accessible-root">
>   *     <svg class="accessible-elements" hidden="true">
>   *       <path class="accessible-bounds" points="..." />
>   *     </svg>
>   *     <div class="accessible-infobar-container">

nit: if you see the actual implementation, infobar container is appended to highligher container, instead of root. So either change the comment or the implementation to be consistent.

::: devtools/server/actors/highlighters/xul-accessible.js:58
(Diff revisions 1 - 2)
> -    border-right: 8px solid transparent;
> -    left: calc(50% - 8px);
> +    border-right: var(--highlighter-bubble-arrow-size) solid transparent;
> +    left: calc(50% - var(--highlighter-bubble-arrow-size));
>    }
>  
>    .top {
> -    border-bottom: 8px solid hsl(214, 13%, 24%);
> +    border-bottom: var(--highlighter-bubble-arrow-size) solid 

nit: still trailing whitespace

::: devtools/server/actors/highlighters/xul-accessible.js:64
(Diff revisions 1 - 2)
> -    top: -8px;
> +      var(--highlighter-bubble-background-color);
> +    top: calc(-1 * var(--highlighter-bubble-arrow-size));
>    }
>  
>    .bottom {
> -    border-top: 8px solid hsl(214, 13%, 24%);
> +    border-top: var(--highlighter-bubble-arrow-size) solid 

nit: still trailing whitespace

::: devtools/server/actors/highlighters/accessible.js:125
(Diff revision 2)
> -        "role": "presentation"
>        },
>        prefix: this.ID_CLASS_PREFIX
>      });
>  
> +    // Build the accessible's infobar markup

Since infobar is the same for both highlighters, lets make an infobar component (in utils) that will be responsible for rendering itself in the markup, and would contain, update methods that are infobar specific (e.g. for role and name), maybe even move.

::: devtools/server/actors/highlighters/accessible.js:244
(Diff revision 2)
>  
>      if (this._updateAccessibleBounds()) {
>        this._showAccessibleBounds();
> +
> +      // Show accessible's infobar
> +      this._showAccessibleInfobar();

Lets comment that we show first to get displayed styles.

::: devtools/server/actors/highlighters/utils/accessibility.js:77
(Diff revision 2)
> + *
> + */
> +function isValidBounds(bounds) {
> +  const { left, right, top, bottom } = bounds;
> +
> +  return left < right && top < bottom;

as we talked, lets check if left and right both equal to 0 or if top and bottom are both equal 0.

::: devtools/server/actors/highlighters/utils/accessibility.js:135
(Diff revision 2)
> +  }
> +
> +  // Check if infobar is offscreen on either top/bottom of viewport and position.
> +  if (isOffScreenOnTop) {
> +    if (boundsTop < 0) {
> +      container.style.top = "var(--highlighter-bubble-arrow-size)";

"var(--highlighter-bubble-arrow-size)" is used in plenty of places to declare it as a variable.

::: devtools/server/actors/highlighters/xul-accessible.js:138
(Diff revision 2)
>  
>      this.bounds = createNode(this.win, {
>        parent: this.container,
>        attributes: {
>          "class": "accessible-bounds",
> -        "role": "presentation"
> +        "aria-hidden": "true"

This aria hidden should go on top level container. The one defined on line 126

::: devtools/server/actors/highlighters/xul-accessible.js:142
(Diff revision 2)
>          "class": "accessible-bounds",
> -        "role": "presentation"
> +        "aria-hidden": "true"
> +      }
> +    });
> +
> +    this.infobarContainer = createNode(this.win, {

as mentioned in the other highlighter, lets move it out as a separate component.

::: devtools/server/actors/highlighters/xul-accessible.js:339
(Diff revision 2)
> +    if (!bounds || !isValidBounds(bounds)) {
> +      return false;
> +    }
> +
> +    this.arrow.removeAttribute("hidden");
> +    const { left, right } = bounds;

shouldn't this position code belong to the moveinfobar method?

::: devtools/server/tests/browser/browser_accessibility_highlighter_infobar.js:41
(Diff revision 2)
> +   is(`"${truncateString(name, MAX_STRING_LENGTH)}"`, `"${expectedName}"`,
> +    "Accessible has correct displayed name.");
> + }
> +
> + // Checks for the accessible highlighter's infobar content.
> +

nit: no emoty line on line 41

::: devtools/server/tests/browser/browser_accessibility_highlighter_infobar.js:41
(Diff revision 2)
> +   is(`"${truncateString(name, MAX_STRING_LENGTH)}"`, `"${expectedName}"`,
> +    "Accessible has correct displayed name.");
> + }
> +
> + // Checks for the accessible highlighter's infobar content.
> +

nit: no emoty line on line 41

Attachment #8991082 - Flags: review?(yzenevich)

Comment hidden (mozreview-request)

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256082/diff/2-3/

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

Comment 7

•

6 years ago

mozreview-review

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review264834

Looks good, I think we can simplify the API between the highlighter object and an infobar. I'll add comments here instead of inline so they are all in one place:

* Nit: lets just name it Inforbar and the other one XULWindowInfobar.
* From what I can tell a common api between the two infobars and the two highlighters are the following methods of the inforbar: buildMarkup, show, hide, update (move should really be done in update).
* To be consistent lets:
  * Create infobar component at the same time we create highlighter.
  * Destroy(set to null) the infobar component when we destroy highlighter.
  * Only pass the things we need to the infobar in its constructor (root can just be passed to buildMarkup method, win and bounds can be passed to move). I think infobar doesn't really need neither highlighter nor root in the contstructor. buildMarkup method can recieve both root and createNode method which highlighter passes to it and is a wrapper around the original createNode that already sets the prefix, and has the window.
  * I don't think you need setContentOn (it doesn't seem to change when you override it in xul infobar either). You can just do it all in updateName and updateRole directly
  * XUL does not need to keep track of any of the infobar elements.
  * _updateAccessibleInfobar methods are not needed. You can just call infobar.update in their place.
  * _updateAccessibleInfobar should move as show into infobar component
  * _hideAccessibleInfobar should move as hide into infobar component.

I think that's mostly it.

::: devtools/server/actors/highlighters.css:650
(Diff revision 3)
>    opacity: 0.6;
>    fill: #6a5acd;
>  }
> +
> +:-moz-native-anonymous .accessible-infobar-name {
> +  color:hsl(210, 30%, 85%);

move color:hsl(210, 30%, 85%) into a variable in the beginning of the file (along with other ones) since this value is used in more places than just our inforbar.

::: devtools/server/tests/browser/browser.ini:33
(Diff revision 3)
>    storage-helpers.js
>    !/devtools/client/shared/test/shared-head.js
>    !/devtools/client/shared/test/telemetry-test-helpers.js
>    !/devtools/server/tests/mochitest/hello-actor.js
>  
> +[browser_accessibility_highlighter_infobar.js]

This is a good "integration" test. That we get things as expected on walker event. Now that we have a component on its own (infobar) we should also add a test suite for it as well (sorry to pile more work on :) ).

Attachment #8991082 - Flags: review?(yzenevich)

Comment hidden (mozreview-request)

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256082/diff/3-4/

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

Comment 9

•

6 years ago

mozreview-review

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review266178

Sorry haven't gotten to the tests just yet, but the rest is reviewed. Final polish and final re-review and I think we'd be good to go.

::: devtools/server/actors/highlighters/accessible.js:196
(Diff revisions 3 - 4)
>      setIgnoreLayoutChanges(true);
>  
>      if (this._updateAccessibleBounds()) {
>        this._showAccessibleBounds();
>  
> -      // Show accessible's infobar. We do this first to get the computed styles of the
> +      this.accessibleInfobar.show(this);

doesn't look like 'this' argument is used anywhere. Lets remove.

::: devtools/server/actors/highlighters/accessible.js:216
(Diff revisions 3 - 4)
>     * Hide the highlighter.
>     */
>    _hide() {
>      setIgnoreLayoutChanges(true);
>      this._hideAccessibleBounds();
> -    this._hideAccessibleInfobar();
> +    this.accessibleInfobar.hide(this.getElement("infobar-container"));

Similarly, doesn't look like this.getElement("infobar-container") is used as an arument, lets remove.

::: devtools/server/actors/highlighters/utils/accessibility.js:40
(Diff revisions 3 - 4)
> +
> +  get win() {
> +    return this.highlighter.win;
> +  }
> +
> +  _createNode(nodeName, options = {}) {

was thinking about this a little more, do you mind removing this method after all. Lets use createNode (from markup) instead to be consistent with attributes, prefixes, etc.

::: devtools/server/actors/highlighters/utils/accessibility.js:68
(Diff revisions 3 - 4)
> +   * Build markup for infobar.
> +   *
> +   *  @param  {Element} root
> +   *          Root element to build infobar with.
> +   *
> +   *  @param  {Function} createNode

This @param is no longer there.

::: devtools/server/actors/highlighters/utils/accessibility.js:81
(Diff revisions 3 - 4)
>          "aria-hidden": "true",
>          "hidden": "true"
>        },
>      });
>  
> -    this.infoBar = createNode(this.win, {
> +    this.infoBar = this._createNode("infobar", {

To be able to have a more elegant getElement, lets have these property names match the ids of the created nodes like this:

this.infobar = ...
this["infobar-text"] = ...
this["infobar-role"] = ...

::: devtools/server/actors/highlighters/utils/accessibility.js:105
(Diff revisions 3 - 4)
> -   */
> +  }
> -  _moveInfobar() {
> -    const container = this.highlighter.getElement("infobar-container");
>  
> -    // Position the infobar using accessible's bounds
> -    const bounds = this.highlighter._bounds;
> +  getElement(el) {
> +    const id = el.id.split("-").slice(-2).join("-");

This can be simplified, see comment below and build markup:

in regular infobar (e.g. here) this method can just look like:
```
getElement(id) {
  return this.highlighter.getElement(id);
}
```
XULWindowInfobar should override it like this:
```
getElement(id) {
  return this[id];
}
```

wherever you use getElement you will just call this.getElement({some-id}) where some-id is the element id without the prefix. This should work because you can put created nodes directly into infobar (in buildMarkup) like this, for example, this["infobar-text"] = createNode(...)

::: devtools/server/actors/highlighters/utils/accessibility.js:113
(Diff revisions 3 - 4)
>  
> -    moveInfobar(container, infobarBounds, this.win);
> +  /**
> +   * Hide the accessible infobar.
> +   */
> +  hide() {
> +    const container = this.getElement(this.container);

So as mentioned above, getElement will be called (here and everywhere else with the string id):

this.getElement("infobar-container")

::: devtools/server/actors/highlighters/utils/accessibility.js:191
(Diff revisions 3 - 4)
> -   *  @param  {Function} setText
> -   *          A function that sets the role text on the infobar element.
> +   *  @param  {Element} el
> +   *          Element to set text content on.
>     *
>     */
> -  updateRole(role, setText) {
> +  updateRole(role, el) {
>      const roleText = role ? `${role}` : "";

Accesible objects will always have a role. role ? ... check is not needed.

::: devtools/server/actors/highlighters/utils/accessibility.js:203
(Diff revisions 3 - 4)
>   * possible with the regular accessible highlighter.
>   */
> -class XULAccessibleInfobar extends AccessibleInfobar {
> -  constructor(highlighter, root) {
> -    super(highlighter, root);
> -
> +class XULWindowInfobar extends Infobar {
> +  constructor(highlighter) {
> +    super(highlighter);
> +    this.highlighter = highlighter;

This is already done in the super constructor, lets remove this.

::: devtools/server/actors/highlighters/utils/accessibility.js:292
(Diff revisions 3 - 4)
> -   *          Options to update infobar content with.
> +   *          Root element to build infobar with.
>     *
>     */
> -  update(options) {
> -    const { name, role } = options;
> +  buildMarkup(root) {
> +    super.buildMarkup(root, createNode);
> +    this.container.removeAttribute("hidden");

This should not be necessarry?

::: devtools/server/actors/highlighters/utils/accessibility.js:294
(Diff revisions 3 - 4)
>     */
> -  update(options) {
> -    const { name, role } = options;
> +  buildMarkup(root) {
> +    super.buildMarkup(root, createNode);
> +    this.container.removeAttribute("hidden");
>  
> -    super.updateRole(role, (roleText) => {
> +    this.arrow = super._createNode("arrow", {

super._createNode -> this._createNode, or since we are removing _createNode - just createNode(...)

::: devtools/server/actors/highlighters/xul-accessible.js:280
(Diff revisions 3 - 4)
>    }
>  
>    /**
>     * Hide accessible bounds highlighter.
>     */
>    _hideAccessibleBounds() {

We never seem to call infobar hide in XUL highlighter, should we ?

::: devtools/server/tests/browser/browser_accessibility_infobar_markup.js:12
(Diff revision 4)
> +// Checks for the accessible Infobar component.
> + add_task(async function() {
> +   const browser = await addTab(MAIN_DOMAIN + "doc_accessibility_infobar.html");
> +
> +   await ContentTask.spawn(browser, null, async function() {
> +     const {require} = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});

nit spaces around require

Attachment #8991082 - Flags: review?(yzenevich)

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

Comment 10

•

6 years ago

mozreview-review

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review266236

I think the markup tests can be removed/folded into show. We don't really test much by checking markup structure. We should really just have tests for visibibility (default, after show, after hide) and text contnet of role and name. Also I would test show/hide when sequence after options and bounds are updated and we re-rednder infobar correctly.

::: devtools/server/tests/browser/browser_accessibility_infobar_markup.js:19
(Diff revision 4)
> +     const { createNode } = require("devtools/server/actors/highlighters/utils/markup");
> +
> +     const doc = content.document;
> +
> +      // Mock highlighter environment.
> +     const mockHighlighterEnv = {

Same as devtools/server/tests/browser/browser_accessibility_infobar_show.js

::: devtools/server/tests/browser/browser_accessibility_infobar_markup.js:49
(Diff revision 4)
> +             "class": "root",
> +         }
> +     });
> +     await accessibleInfobar.buildMarkup(root);
> +
> +     const container = accessibleInfobar.container;

These are probably overkill (I don't think we test anything relevant when we check markup structure.) Lets drop there and just have ones that check correct textContent for name and role.

It's probably only worth checking that before show we are hidden and after show we are visible. And i think it's done in show test file already.

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:9
(Diff revision 4)
> +
> + "use strict";
> +
> +// Checks for the accessible Infobar's show method.
> + add_task(async function() {
> +   const browser = await addTab(MAIN_DOMAIN + "doc_accessibility_infobar.html");

I don't think you need to spawn a new tab here. You can just run the tests in the current tab.

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:13
(Diff revision 4)
> + add_task(async function() {
> +   const browser = await addTab(MAIN_DOMAIN + "doc_accessibility_infobar.html");
> +
> +   info("Test the show method with valid bounds");
> +   await ContentTask.spawn(browser, null, async function() {
> +     const {require} = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});

require seems to be used in head.js without being imported, are you sure it needs to be explicitely imported?

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:20
(Diff revision 4)
> +     const { createNode } = require("devtools/server/actors/highlighters/utils/markup");
> +
> +     const doc = content.document;
> +
> +      // Mock highlighter environment with valid bounds.
> +     const mockHighlighterEnv = {

The bits of code from here until the end of initial buildMarkup are similar enough in all 3 tests that they should belong to a separate function e.g. setupAccessibleInfobar that should be in head.js

Maybe it can take bounds and options as arugments?

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:26
(Diff revision 4)
> +        _bounds: { top: 1, bottom: 2, left: 1, right: 2},
> +        options: { name: "Accessible Button", role: "button" },
> +        ID_CLASS_PREFIX: "accessible-",
> +     };
> +
> +     const accessibleInfobar = new XULWindowInfobar({

I think you need to do the have tests with regular Infobar, since we override a bunch of methods. I think it should be possible to parametrize these based on the Infobar passed.

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:45
(Diff revision 4)
> +     const infobarRole = accessibleInfobar.getElement(accessibleInfobar.infobarRole);
> +     const infobarName = accessibleInfobar.getElement(accessibleInfobar.infobarName);
> +
> +     info("Checks for infobar's show method");
> +     accessibleInfobar.show();
> +     is(container.getAttribute("hidden"), null,

ok(!container.hasAttribute("hidden"), ...)

::: devtools/server/tests/browser/browser_accessibility_xul_infobar_markup.js:11
(Diff revision 4)
> +
> +// Checks for the XUL Window Infobar component.
> + add_task(async function() {
> +   const browser = await addTab(MAIN_DOMAIN + "doc_accessibility_infobar.html");
> +
> +   await ContentTask.spawn(browser, null, async function() {

Same as in devtools/server/tests/browser/browser_accessibility_infobar_show.js I don't think you need a new tab for that.

::: devtools/server/tests/browser/browser_accessibility_xul_infobar_markup.js:19
(Diff revision 4)
> +     const { createNode } = require("devtools/server/actors/highlighters/utils/markup");
> +
> +     const doc = content.document;
> +
> +      // Mock highlighter environment.
> +     const mockHighlighterEnv = {

See comment in devtools/server/tests/browser/browser_accessibility_infobar_show.js about taking this out as a function in head.js

::: devtools/server/tests/browser/browser_accessibility_xul_infobar_markup.js:39
(Diff revision 4)
> +             "class": "root",
> +         }
> +     });
> +     await accessibleInfobar.buildMarkup(root);
> +
> +     const arrow = accessibleInfobar.getElement(accessibleInfobar.arrow);

Same as the other markup test. No need for tests that check markup structure.

::: devtools/server/tests/browser/browser_accessibility_xul_infobar_markup.js:44
(Diff revision 4)
> +     const arrow = accessibleInfobar.getElement(accessibleInfobar.arrow);
> +     ok(arrow, "Infobar has an arrow element.");
> +     is(arrow.parentNode, accessibleInfobar.infoBar, "arrow is a child of infoBar");
> +
> +     info("Checks for infobar's XUL show method");
> +     accessibleInfobar.show();

It's probably only worth checking that before show we are hidden and after show we are visible. And i think it's done in show test file already.

::: devtools/server/tests/browser/browser_accessibility_xul_infobar_markup.js:46
(Diff revision 4)
> +     is(arrow.parentNode, accessibleInfobar.infoBar, "arrow is a child of infoBar");
> +
> +     info("Checks for infobar's XUL show method");
> +     accessibleInfobar.show();
> +     const container = accessibleInfobar.container;
> +     is(container.getAttribute("hidden"), null, "XUL Infobar container is shown.");

here and below ok(!container.hasAttribute("hidden"),...

Comment hidden (mozreview-request)

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256082/diff/4-5/

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

Comment 12

•

6 years ago

mozreview-review

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review268080

This looks good with final set of nits.

::: devtools/server/actors/highlighters/utils/accessibility.js:221
(Diff revisions 4 - 5)
>  /**
>   * The XULAccessibleInfobar handles building the XUL infobar markup where it isn't
>   * possible with the regular accessible highlighter.
>   */
>  class XULWindowInfobar extends Infobar {
>    constructor(highlighter) {

I don't think you need to override constructor if you're only calling super() in it.

::: devtools/server/actors/highlighters/utils/accessibility.js:382
(Diff revisions 4 - 5)
>   *           zoom level of the accessible object's parent window
>   * @return {Object|null} Returns, if available, positioning and bounds information for
>   *                 the accessible object.
>   */
>  function getBounds(win, { x, y, w, h, zoom }) {
> +  // Ensure that bounds values are valid.

I don't think we need this.

::: devtools/server/actors/highlighters/xul-accessible.js:192
(Diff revisions 4 - 5)
>      const hasBounds = options && typeof options.x == "number" &&
>                                 typeof options.y == "number" &&
>                                 typeof options.w == "number" &&
>                                 typeof options.h == "number";
>      if (!hasBounds || !isNodeValid(node) || isSameNode) {
> +      this.hide();

Same here, we instead need to call highlighter.hide in tests between show's.

::: devtools/server/actors/highlighters/xul-accessible.js:234
(Diff revisions 4 - 5)
>     */
>    _update() {
>      this._hideAccessibleBounds();
>      const bounds = this._bounds;
>      if (!bounds) {
> +      this.hide();

I don't think we need it. Instead, please call highlighter.hide between highlighter.show's in your tests.

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:8
(Diff revision 5)
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +/**
> + * Checks for the accessible Infobar and XULWindowInfobar components.

Update this comment.

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:31
(Diff revision 5)
> +       *
> +       * @return  {Boolean} Returns if the infobar container is hidden.
> +       */
> +      function isContainerHidden(infobar) {
> +        const container = infobar.getElement("infobar-container");
> +        const hiddenAttr = container.getAttribute("hidden");

we can just write:
```
return container.getAttribute("hidden") === "true";
```

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:82
(Diff revision 5)
> +       *
> +       */
> +      function checkInfobar(infobar, expected) {
> +        const { shouldBeHidden, role, name } = expected;
> +
> +        if (shouldBeHidden) {

From here to line 91 lets just write:

```
is(isContainerHidden(infobar), shouldBeHidden, ...);
if (shouldBeHidden) {
  return;
}
```

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:122
(Diff revision 5)
> +          ...bounds,
> +          role: "button",
> +          name: "Accessible Button"
> +        });
> +
> +        info("Check that infobar is shown with valid bounds.");

nit: move this info right before you call show.

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:129
(Diff revision 5)
> +          role: "button",
> +          name: "Accessible Button",
> +          shouldBeHidden: false
> +        });
> +
> +        info("Checks for showing accessible with invalid bounds.");

make sure you call highlighter.hide() after you're done with checkInforbar.

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:133
(Diff revision 5)
> +
> +        info("Checks for showing accessible with invalid bounds.");
> +        highlighter.show(node);
> +        checkInfobar(infobar, { shouldBeHidden: true });
> +
> +        info("Check to make sure content is updated with new options.");

same here, you need to hide before the next show.

Attachment #8991082 - Flags: review?(yzenevich) → review+

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

Updated

•

6 years ago

Attachment #8991082 - Flags: review?(gl)

Micah [:mtigley] (she/her)

Assignee

Comment 13

•

6 years ago

mozreview-review-reply

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review266178

> We never seem to call infobar hide in XUL highlighter, should we ?

We don't need to since `_hideAccessibleBounds` hides the entire markup container for the highlighter.

Micah [:mtigley] (she/her)

Assignee

Comment 14

•

6 years ago

mozreview-review-reply

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review268080

> make sure you call highlighter.hide() after you're done with checkInforbar.

As discussed, we don't need to check for invalid bounds, since show() will never be called if bounds are invalid.

Comment hidden (mozreview-request)

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256082/diff/5-6/

Gabriel Luong [:gl] (ΦωΦ)

Comment 16

•

6 years ago

mozreview-review

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review268902

::: devtools/server/actors/accessibility.js:352
(Diff revision 6)
>      } catch (e) {
>        return null;
>      }
>  
> +    // Check if accessible bounds are invalid.
> +    const left = x, right = x + w, top = y, bottom = y + h;

I would move these constants into their own individual lines to make it easier to read. This would make more sense when you are initializing simple values like {}, but otherwise, this wouldn't be easy to read.

::: devtools/server/actors/accessibility.js:455
(Diff revision 6)
>      Actor.prototype.destroy.call(this);
>  
>      this.reset();
>  
> -    this.highlighter.destroy();
> +    this.highlighter.off("highlighter-event", this.onHighlighterEvent);
>      this.highlighter = null;

We should also destroy the highlighter.

::: devtools/server/actors/highlighters/utils/accessibility.js:191
(Diff revision 6)
> +  /**
> +   * Show the accessible's name message.
> +   *
> +   *  @param  {String} name
> +   *          Accessible's name value.
> +   *

You are introducing a different kind of JSDoc style with the spacing and extra new lines and we should follow the convention used. See getBounds() below for an example.

::: devtools/server/actors/highlighters/utils/accessibility.js:228
(Diff revision 6)
> +   *
> +   * @param  {Object} container
> +   *         The infobar container.
> +   *
> +   */
> +  _moveInfobar(container) {

We also have a moveInfobar in https://searchfox.org/mozilla-central/source/devtools/server/actors/highlighters/utils/markup.js#586. I was wondering if we have considered using that or improving it.

::: devtools/server/actors/highlighters/xul-accessible.js:9
(Diff revision 6)
>  
>  "use strict";
>  
> -const { getBounds } = require("./utils/accessibility");
> +const {
> +  getBounds,
> +  XULWindowInfobar } = require("./utils/accessibility");

I think we should just keep both of these on a single line like below.

::: devtools/server/actors/highlighters/xul-accessible.js:178
(Diff revision 6)
>     *           width of the the accessible object
>     *         - {Number} h
>     *           height of the the accessible object
>     *         - duration {Number}
>     *                    Duration of time that the highlighter should be shown.
> +   *         - {String | null} name

String|null

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:26
(Diff revision 6)
> +      /**
> +       * Get whether or not infobar container is hidden.
> +       *
> +       * @param   {Object}  infobar
> +       *          Accessible highlighter's infobar component.
> +       *

Same NIT about these JSDoc

Attachment #8991082 - Flags: review?(gl)

Micah [:mtigley] (she/her)

Assignee

Comment 17

•

6 years ago

mozreview-review-reply

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review268902

> We should also destroy the highlighter.

We don't need to destroy since the highlighter actor is added to the walker actor's pool of actors, so when the walker is destroyed the highlighter will be too.

> We also have a moveInfobar in https://searchfox.org/mozilla-central/source/devtools/server/actors/highlighters/utils/markup.js#586. I was wondering if we have considered using that or improving it.

There is no guarantee bounds will be the same between XUL and the regular highlighter, as well as the moveInfobar from markup.js expecting special properties that are only present on anonymous content, which is why we are unable to use the existing one.

Comment hidden (mozreview-request)

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256082/diff/6-7/

Gabriel Luong [:gl] (ΦωΦ)

Comment 19

•

6 years ago

mozreview-review

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review269490

::: devtools/server/actors/highlighters/accessible.js:123
(Diff revision 7)
> -        "role": "presentation"
>        },
>        prefix: this.ID_CLASS_PREFIX
>      });
>  
> +    // Build the accessible's infobar markup

Add a period at the end.

::: devtools/server/actors/highlighters/utils/accessibility.js:64
(Diff revision 7)
> +   */
> +  buildMarkup(root) {
> +    this["infobar-container"] = createNode(this.win, {
> +      parent: root,
> +      attributes: {
> +        "class": `${this.prefix}infobar-container`,

Couldn't we avoid doing this by specifying prefix: this.prefix.

Ex:
    createNode(this.win, {
      nodeType: "span",
      parent: areaTextbox,
      attributes: {
        "class": "area-infobar-dimensions",
        "id": "area-infobar-dimensions"
      },
      prefix: this.ID_CLASS_PREFIX
    });

::: devtools/server/actors/highlighters/utils/accessibility.js:96
(Diff revision 7)
> +        "class": `${this.prefix}infobar-role`,
> +        "id": `${this.prefix}infobar-role`,
> +      },
> +    });
> +
> +    this["infobar-name"] = createNode(this.win, {

Since we already fetching the elements using the getElement method, I don't think we should set the node into the class' this prototype.

This is related to all the this["infobar-X"].

::: devtools/server/actors/highlighters/utils/accessibility.js:110
(Diff revision 7)
> +
> +  /**
> +   * Destroy the Infobar's highlighter.
> +   */
> +  destroy() {
> +    this.highlighter = null;

We should clear 

this.infobar = null;

::: devtools/server/actors/highlighters/utils/accessibility.js:118
(Diff revision 7)
> +  /**
> +   * Gets the element with the specified ID.
> +   *
> +   * @param  {String} id
> +   *         Element ID.
> +   * @return {String} Returns the specified element.

We can actually remove "Returns" since the @return should write like a sentence.

Also, I think you meant Element here.

@return {Element} the element with the specified ID.

::: devtools/server/actors/highlighters/utils/accessibility.js:127
(Diff revision 7)
> +  }
> +
> +  /**
> +   * Gets the text content of element.
> +   *
> +   * @param   {String} id

Just 2 spaces after @param

::: devtools/server/actors/highlighters/utils/accessibility.js:127
(Diff revision 7)
> +  }
> +
> +  /**
> +   * Gets the text content of element.
> +   *
> +   * @param   {String} id

Just 2 spaces after @param

::: devtools/server/actors/highlighters/utils/accessibility.js:129
(Diff revision 7)
> +  /**
> +   * Gets the text content of element.
> +   *
> +   * @param   {String} id
> +   *          Element ID to retrieve text content from.
> +   * @return  {String} Returns the text content of the element.

Can remove "Returns"

::: devtools/server/actors/highlighters/utils/accessibility.js:129
(Diff revision 7)
> +  /**
> +   * Gets the text content of element.
> +   *
> +   * @param   {String} id
> +   *          Element ID to retrieve text content from.
> +   * @return  {String} Returns the text content of the element.

just a single space after @return

Basically the { should line up.

::: devtools/server/actors/highlighters/utils/accessibility.js:238
(Diff revision 7)
> +    container.style.left = `${boundsMidPoint}px`;
> +
> +    const zoom = getCurrentZoom(this.win);
> +    let {
> +      width: viewportWidth,
> +      height: viewportHeight } = getViewportDimensions(this.win);

Move "} = getViewportDimensions(this.win);" to the next line.

::: devtools/server/actors/highlighters/utils/accessibility.js:300
(Diff revision 7)
> +   *         Root element to build infobar with.
> +   */
> +  buildMarkup(root) {
> +    super.buildMarkup(root, createNode);
> +
> +    this.arrow = createNode(this.win, {

We need to remove this reference to the node in a destroy(). Otherwise, I would do this.getElement("arrow") everywhere.

::: devtools/server/actors/highlighters/xul-accessible.js:262
(Diff revision 7)
>      if (!this.currentNode || !this.highlighterEnv.window) {
>        return;
>      }
>  
>      this._hideAccessibleBounds();
> +    this.accessibleInfobar.hide();

Add a new line after this.

::: devtools/server/actors/highlighters/xul-accessible.js:300
(Diff revision 7)
>      if (this.container) {
>        this.container.remove();
>      }
>  
>      this.win = null;
> +    this.accessibleInfobar.destroy();

Let's reorder these to be

this.accessibleInfobar.destroy();

this.accessibleInfobar = null;
this.win = null;

That way we have a clear section to destroy and remove things and then the end is just to clean up references by setting them to null.

::: devtools/server/tests/browser/browser_accessibility_highlighter_infobar.js:23
(Diff revision 7)
> + *         The accessibility walker.
> + * @param  {String} expectedName
> + *         Expected string content for displaying the accessible's name.
> + *         We are testing this in particular because name can be truncated.
> + */
> + async function checkNameAndRole(walker, querySelector, a11yWalker, expectedName) {

I like to move helper functions to unit tests to the bottom of the file so that we can read the test tasks up first.

::: devtools/server/tests/browser/browser_accessibility_highlighter_infobar.js:39
(Diff revision 7)
> +
> +   is(`"${truncateString(name, MAX_STRING_LENGTH)}"`, `"${expectedName}"`,
> +    "Accessible has correct displayed name.");
> + }
> +
> + // Checks for the accessible highlighter's infobar content.

This is the unit test description so it should be moved to the top of the file so that it is the first thing people will read when they are looking to see what this unit test is doing.

Move it under "use strict";:

"use strict";

// Test the accessible highlighter's infobar content.

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:8
(Diff revision 7)
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +/**
> + * Checks for the AccessibleHighlighter's and XULWindowHighlighter's infobar components.

Change the comment style to be //

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:10
(Diff revision 7)
> +"use strict";
> +
> +/**
> + * Checks for the AccessibleHighlighter's and XULWindowHighlighter's infobar components.
> + */
> +add_task(async function() {

Add a new line between the test comment and add_task

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:28
(Diff revision 7)
> +       *
> +       * @param  {Object} infobar
> +       *         Accessible highlighter's infobar component.
> +       * @return {Boolean} Returns if the infobar container is hidden.
> +       */
> +      function isContainerHidden(infobar) {

I think we can move these helper functions outside this giant ContentTask.spawn

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:31
(Diff revision 7)
> +       * @return {Boolean} Returns if the infobar container is hidden.
> +       */
> +      function isContainerHidden(infobar) {
> +        const container = infobar.getElement("infobar-container");
> +
> +        return container.getAttribute("hidden") === "true";

I think this whole function can be simplified to be 

infobar.getElement("infobar-container").getAttribute("hidden")

::: devtools/server/tests/browser/browser_accessibility_infobar_show.js:71
(Diff revision 7)
> +       *         - {String} name
> +       *           Name value of the accessible.
> +       *         - {Boolean} shouldBeHidden
> +       *           If the infobar component should be hidden.
> +       */
> +      function checkInfobar(infobar, expected) {

We can simplify this:

function checkInfobar(infobar, { shouldBeHidden, role, name }) {

Attachment #8991082 - Flags: review?(gl)

Micah [:mtigley] (she/her)

Assignee

Comment 20

•

6 years ago

mozreview-review-reply

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review269490

> Since we already fetching the elements using the getElement method, I don't think we should set the node into the class' this prototype.
> 
> This is related to all the this["infobar-X"].

We store the node into the class since the highlighter's getElement method only works for anonymous content markup, so we are doing this primarily for the XUL infobar markup. 

However, I changed it so we use getElementById for the XULWindowInfobar's getElement override .

> I think we can move these helper functions outside this giant ContentTask.spawn

Since the tests run on a separate content process for this browser tab, they won't be able to access functions defined outside of it.

Comment hidden (mozreview-request)

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256082/diff/7-8/

Gabriel Luong [:gl] (ΦωΦ)

Comment 22

•

6 years ago

mozreview-review

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

https://reviewboard.mozilla.org/r/256082/#review269558

::: devtools/server/actors/highlighters/utils/accessibility.js:242
(Diff revision 8)
> +    // highlighter's bounds position.
> +    const {
> +      left: boundsLeft,
> +      right: boundsRight,
> +      top: boundsTop,
> +      bottom: boundsBottom} = this.bounds;

Move "} = this.bounds;" to the next line.

::: devtools/server/tests/browser/browser_accessibility_highlighter_infobar.js:32
(Diff revision 8)
> +  await waitForA11yShutdown();
> +  await client.close();
> +  gBrowser.removeCurrentTab();
> +});
> +
> +  /**

This JSDoc is misindented.

Attachment #8991082 - Flags: review?(gl) → review+

Comment hidden (mozreview-request)

Comment on attachment 8991082 [details]
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256082/diff/8-9/

Micah [:mtigley] (she/her)

Assignee

Comment 24

•

6 years ago

Try server results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcf24236fa078f3cea5d7682643a079077b5179f

Timed-out test unrelated to this patch.

Micah [:mtigley] (she/her)

Assignee

Updated

•

6 years ago

Keywords: checkin-needed

Pulsebot

Comment 25

•

6 years ago

Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0ac567c436c
Show accessible object's name and role information with the info-bar highlighter. r=gl,yzen

Keywords: checkin-needed

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

Updated

•

6 years ago

Blocks: 1471933

Tiberius Oros[:tiberius_oros]

Comment 26

•

6 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

status-firefox63: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 63

Micah [:mtigley] (she/her)

Assignee

Updated

•

6 years ago

Keywords: dev-doc-needed

Irene Smith

Comment 27

•

6 years ago

Added the following information to the page: https://developer.mozilla.org/en-US/docs/Tools/Accessibility_inspector#Highlighting_of_UI_items

"In the following example, you can see that the image has been highlighted and its role, graphic, and name, "Road, Asphalt, Sky, Clouds, Fall" appears in the information bar above it." along with an example image showing the info-bar.

Flags: needinfo?(yzenevich)

Keywords: dev-doc-needed → dev-doc-complete

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

Comment 28

•

6 years ago

(In reply to Irene Smith from comment #27)
> Added the following information to the page:
> https://developer.mozilla.org/en-US/docs/Tools/
> Accessibility_inspector#Highlighting_of_UI_items
> 
> "In the following example, you can see that the image has been highlighted
> and its role, graphic, and name, "Road, Asphalt, Sky, Clouds, Fall" appears
> in the information bar above it." along with an example image showing the
> info-bar.

Looks good, thanks Irene!

Flags: needinfo?(yzenevich)

InfobarExample.png 6 years ago Micah [:mtigley] (she/her) 181.12 KB, image/png		Details
Bug 1473030 - Show accessible object's name and role information with the info-bar highlighter. 6 years ago Micah [:mtigley] (she/her) 59 bytes, text/x-review-board-request	gl : review+	Details