Remove the CUI toolbarpaletteitem binding

RESOLVED FIXED in Firefox 65

Status

()

task
P3
normal
RESOLVED FIXED
7 months ago
15 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)

Assignee

Description

7 months ago
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.
Assignee

Comment 2

7 months ago
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)
Assignee

Comment 5

7 months ago
(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
Assignee

Comment 7

7 months ago
(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

7 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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f7476054232
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Assignee

Updated

7 months ago
Assignee: nobody → bgrinstead

Updated

4 months ago
Depends on: 1530288

Updated

15 days ago
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.