Remove the CUI toolbarpaletteitem binding

RESOLVED FIXED in Firefox 65

Status

()

P3
normal
RESOLVED FIXED
4 months ago
22 days ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(3 attachments)

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.
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).
Flags: needinfo?(mzehe)
(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"
Attachment #9025776 - Attachment description: Bug 1507875 - WIP - Remove the CUI toolbarpaletteitem binding → Bug 1507875 - Remove the customizable ui toolbarpaletteitem binding;r=Gijs
(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.
Flags: needinfo?(mzehe)
Priority: -- → P3
(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

4 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f7476054232
Remove the customizable ui toolbarpaletteitem binding;r=Gijs

Comment 9

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f7476054232
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Assignee: nobody → bgrinstead

Updated

22 days ago
Depends on: 1530288
You need to log in before you can comment on or make changes to this bug.