1441778 - [Accessibility] Get rid of the "New Attribute" text when speaking the node info for an item.

Status: NEW

(DevTools :: Inspector, enhancement, P3)

Description

[Marco Zehe (:MarcoZ)]

Bug 1439920 labeled the New Attribute button for assistive technologies. An unwanted side effect is that now, the words New Attribute appear with every tree node a screen reader, for example, speaks when navigating the inspector. For example, the item for this Bugzilla entry reads like this:

'<textarea id="comment" name="comment" rows="25" cols="80" onfocus="this.rows=25" style="width: 648px;" New attribute ></textarea>ev'

The "New Attribute " before the closing > of the opening tag is what should not be there. The item should read like this:

'<textarea id="comment" name="comment" rows="25" cols="80" onfocus="this.rows=25" style="width: 648px;" ></textarea>ev'

Side note: That "ev" text is also weird at the end.

The reason is that for the element-editor.tagline, which in its base class markup-container gets a role of "treeitem", the spoken label for assistive technologies is built using the accessible sub tree. Since the button now has textual info where it didn't use to have it, this is now included.

The solution is to give the tree item an explicit label via aria-label, overriding the logic to get the spoken label (AKA AccessibleName) from the children.

gl, I believe I need some help with this. I know that I have to add something to the tagline element for the element-container. But what is the best way to construct the label in element-editor.js? I see that attributes, for example, are manipulated in multiple places, so it would be easiest to have somewhere to update the label after each mutation that is guaranteed to be called. I will probably need to collect the bits and pieces from properties. But I must admit I am a bit overwhelmed by the various bits and pieces in that element-editor.js file... So some pointers would be really helpful. :)

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Passing this to jdescottes

Comment 2

[Julian Descottes [:jdescottes]]

From the summary, I feel like we only need to define aria-label when creating the tagline element? 
In this case we should be able to do so directly by adding the attribute in https://searchfox.org/mozilla-central/rev/908e3662388a96673a0fc00b84c47f832a5bea7c/devtools/client/inspector/markup/views/markup-container.js#89

If you already tried that and it is not enough here, can you share some more details about the scenarios where the attribute needs to be updated?

Comment 3

[Marco Zehe (:MarcoZ)]

No, the problem is that right now, the text is fetched from the children, including all elements with aria-labels. The problem is that you cannot just assign it once. What happens if you add or remove attributes, or change an attribute's value? Those mutations must be reflected as well.
So if you just assign the aria-label once when building the tag line, it will never get updated if mutations happen within that line.

Comment 4

[Julian Descottes [:jdescottes]]

Oh I see, aria-label should contain the whole string that is going to be read for this node. 

Two questions. First, what is the logic to differentiate what should be read from what should not be read? Second, could we try to modify the markup in order to wrap the element labeled with `aria-label="New attribute"` with a an element with an empty aria-label.

If we need absolutely to handle this with a custom aria-label on the main container element, then we should create a method that can generate such a label and call it in two methods of markup-container: _buildMarkup and update. The update method is called every time a mutation for a given node changes the attributes or the inline text content. See the implementation at https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/devtools/client/inspector/markup/markup.js#1002-1039.

Comment 5

[Marco Zehe (:MarcoZ)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #4)
> Oh I see, aria-label should contain the whole string that is going to be
> read for this node. 

Exactly.

> Two questions. First, what is the logic to differentiate what should be read
> from what should not be read? Second, could we try to modify the markup in
> order to wrap the element labeled with `aria-label="New attribute"` with a
> an element with an empty aria-label.

No, that wouldn't work, since the current logic, that kicks in if no aria-label is present, will just grab all the names from all accessibles of the children. That's why the button is now included. Like I wrote in the initial description, everything else that was read before is fine. Like I want to hear the tag, existing attributes etc. The one thing that is not clear to me is what "ev" at the end of that string actually symbolizes.

> If we need absolutely to handle this with a custom aria-label on the main
> container element, then we should create a method that can generate such a
> label and call it in two methods of markup-container: _buildMarkup and
> update. The update method is called every time a mutation for a given node
> changes the attributes or the inline text content. See the implementation at
> https://searchfox.org/mozilla-central/rev/
> efce4ceddfbe5fe4e2d74f1aed93894bada9b273/devtools/client/inspector/markup/
> markup.js#1002-1039.

So, if we just grabbed all the textContent from the node, that would give us everything we need, since an aria-label doesn't count as textContent in the JS sense.

My problem is that I am totally ovewhelmed with all the dependencies and what goes where for which of the pieces. :(

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Marco Zehe (:MarcoZ) from comment #5)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #4)
> > Oh I see, aria-label should contain the whole string that is going to be
> > read for this node. 
> 
> Exactly.
> 
> > Two questions. First, what is the logic to differentiate what should be read
> > from what should not be read? Second, could we try to modify the markup in
> > order to wrap the element labeled with `aria-label="New attribute"` with a
> > an element with an empty aria-label.
> 
> No, that wouldn't work, since the current logic, that kicks in if no
> aria-label is present, will just grab all the names from all accessibles of
> the children. That's why the button is now included. Like I wrote in the
> initial description, everything else that was read before is fine. 

I assumed that when the children are grabbed, the algorithm would still respect a "hierarchy" of aria-labels. On a simplified example that would be:

  <div>
    <span>child 1</span>
    <span>child 2</span>
    <div aria-label="child 3">
      <span>should not be read</span>
    </div>
  </div>

I would expect this to be read as "child 1 child 2 child 3". Not sure we can use an empty aria-label to silence children though, the attribute might be ignored.

> Like I
> want to hear the tag, existing attributes etc. The one thing that is not
> clear to me is what "ev" at the end of that string actually symbolizes.
> 

"ev" represents what we call the event bubble. It indicates that event listeners are attached to this DOM node. Clicking on this event bubble shows a popup that lists all the listeners. It would be interesting to have a more description aria-label here I think. 

> > If we need absolutely to handle this with a custom aria-label on the main
> > container element, then we should create a method that can generate such a
> > label and call it in two methods of markup-container: _buildMarkup and
> > update. The update method is called every time a mutation for a given node
> > changes the attributes or the inline text content. See the implementation at
> > https://searchfox.org/mozilla-central/rev/
> > efce4ceddfbe5fe4e2d74f1aed93894bada9b273/devtools/client/inspector/markup/
> > markup.js#1002-1039.
> 
> So, if we just grabbed all the textContent from the node, that would give us
> everything we need, since an aria-label doesn't count as textContent in the
> JS sense.
> 

I think it should work for now, as long as the textContent is good enough. If some children start using aria-label to be more descriptive, this logic will have to be complexified.  

> My problem is that I am totally ovewhelmed with all the dependencies and
> what goes where for which of the pieces. :(

I think we should recreate this aria-label in the two methods I mentioned:
1. buildMarkup at https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/devtools/client/inspector/markup/views/markup-container.js#80
2. update at https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/devtools/client/inspector/markup/views/markup-container.js#714

I will give it a shot and see how it goes.

Comment 7

[Julian Descottes [:jdescottes]]

I didn't manage to make my suggestion work correctly, sorry.