1507875 - Remove the CUI toolbarpaletteitem binding

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, task, P3)

Description

[Brian Grinstead [:bgrins]]

After Bug 1473311 blockers are fixed, the only remaining binding for Customizable UI will be "toolbarpaletteitem": https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/browser/components/customizableui/content/toolbar.xml#33-40.

This binding only creates <content>, and I believe they are only programatically created via CustomizeMode.jsm, so we may be able to construct the DOM directly there with normal DOM instead of anonymous content.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1507875 - Remove the customizable ui toolbarpaletteitem binding;r=Gijs

Comment 2

[Brian Grinstead [:bgrins]]

Marco, do you have an opinion on if the individual items in the Customize UI should show up in the accessible tree as:

- pushbutton: "Find"
- label:
-- text leaf: "Find"

versus:

- pushbutton: "Find"
- text leaf: "Find"

That is, I have a patch that removes the label element and uses the ::after selector to set the `content` to the same string (which removes a level from the tree putting the text leaf directly beneath the button).

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: current-cui-accessible-tree.png

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: updated-cui-accessible-tree.png.png

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Marco, do you have an opinion on if the individual items in the Customize UI
> should show up in the accessible tree as:
> 
> - pushbutton: "Find"
> - label:
> -- text leaf: "Find"
> 
> versus:
> 
> - pushbutton: "Find"
> - text leaf: "Find"
> 
> That is, I have a patch that removes the label element and uses the ::after
> selector to set the `content` to the same string (which removes a level from
> the tree putting the text leaf directly beneath the button).

Correction: I had accidentally listed the updated version as "text leaf" but it should have been "statictext". The version with the patch is:

- pushbutton: "Find"
- statictext: "Find"

Comment 6

[Marco Zehe (:MarcoZ)]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > Marco, do you have an opinion on if the individual items in the Customize UI
> > should show up in the accessible tree as:
> > 
> > - pushbutton: "Find"
> > - label:
> > -- text leaf: "Find"
> > 
> > versus:
> > 
> > - pushbutton: "Find"
> > - text leaf: "Find"
> > 
> > That is, I have a patch that removes the label element and uses the ::after
> > selector to set the `content` to the same string (which removes a level from
> > the tree putting the text leaf directly beneath the button).
> 
> Correction: I had accidentally listed the updated version as "text leaf" but
> it should have been "statictext". The version with the patch is:
> 
> - pushbutton: "Find"
> - statictext: "Find"

I am confused. Why is the text separate from the button in the first place? I mean with and without the <label> element? Does the pushbutton not provide the text already?

Anyway, from a screen reader perspective, it would only make a difference if the <label> element was labelling something which its removal now breaks. Other than that, for static text, these are almost equivalent. If the focusable item, the pushbutton, has a proper name, this should be OK.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Marco Zehe (:MarcoZ) from comment #6)
> (In reply to Brian Grinstead [:bgrins] from comment #5)
> > (In reply to Brian Grinstead [:bgrins] from comment #2)
> > > Marco, do you have an opinion on if the individual items in the Customize UI
> > > should show up in the accessible tree as:
> > > 
> > > - pushbutton: "Find"
> > > - label:
> > > -- text leaf: "Find"
> > > 
> > > versus:
> > > 
> > > - pushbutton: "Find"
> > > - text leaf: "Find"
> > > 
> > > That is, I have a patch that removes the label element and uses the ::after
> > > selector to set the `content` to the same string (which removes a level from
> > > the tree putting the text leaf directly beneath the button).
> > 
> > Correction: I had accidentally listed the updated version as "text leaf" but
> > it should have been "statictext". The version with the patch is:
> > 
> > - pushbutton: "Find"
> > - statictext: "Find"
> 
> I am confused. Why is the text separate from the button in the first place?
> I mean with and without the <label> element? Does the pushbutton not provide
> the text already?
> 

The DOM currently looks like:

<toolbarpalettitem>
  <hbox>
    <toolbarbutton />
  </hbox>
  <label />
</toolbarpaletteitem>

Where <toolbarbutton /> could also be swapped out with arbitrary content depending on the widget (i.e. a text box for the search box, or multiple toolbarbuttons for cut/copy/paste).

I think the reason the extra DOM (currently a <label> but proposed to be ::after content) is there is so the label can appear visually beneath the button.

> Anyway, from a screen reader perspective, it would only make a difference if
> the <label> element was labelling something which its removal now breaks.
> Other than that, for static text, these are almost equivalent. If the
> focusable item, the pushbutton, has a proper name, this should be OK.

OK thanks - the label isn't labelling anything in particular AFAICT, and the pushbutton does have the same name. So sounds like removing the label won't be an issue here.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f7476054232
Remove the customizable ui toolbarpaletteitem binding;r=Gijs

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8f7476054232