Closed Bug 1430777 Opened 5 years ago Closed 5 years ago

Move more XBL accessibility roles into XULMap.h

Categories

(Toolkit :: XUL Widgets, task)

Product:
Toolkit
Component:
XUL Widgets
Type:
task
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: Paolo, Unassigned)

References

Details

Here is a list of the bindings from bug 1428930 that I've determined may need conditional constructors or more investigation:

toolkit/content/widgets/autocomplete.xml#autocomplete role="xul:combobox"
  textbox[type="autocomplete"]
  browser/base/content/urlbarBindings.xml#urlbar
    #urlbar
  browser/components/search/content/search.xml#searchbar-textbox
    .searchbar-textbox

toolkit/content/widgets/autocomplete.xml#autocomplete-base-popup role="none"
  toolkit/content/widgets/autocomplete.xml#autocomplete-result-popup
    panel[type="autocomplete"]
    browser/components/search/content/search.xml#browser-search-autocomplete-result-popup
      #PopupSearchAutoComplete
  toolkit/content/widgets/autocomplete.xml#autocomplete-rich-result-popup
    panel[type="autocomplete-richlistbox"]
    browser/base/content/urlbarBindings.xml#urlbar-rich-result-popup
      #PopupAutoCompleteRichResult

toolkit/content/widgets/button.xml#button-base role="xul:button"
  browser/base/content/browser-tabPreviews.xml#ctrlTab-preview
    .ctrlTab-preview
  toolkit/content/widgets/button.xml#button
    button
    toolkit/content/widgets/button.xml#button-repeat
      button[type="repeat"]
    toolkit/content/widgets/button.xml#menu
      button[type="menu"]
      button[type="panel"]
  toolkit/content/widgets/button.xml#menu-button-base
    browser/components/downloads/content/download.xml#download-subview-toolbarbutton
      .subviewbutton.download
    toolkit/content/widgets/button.xml#menu-button
      button[type="menu-button"]
    toolkit/content/widgets/toolbarbutton.xml#menu-button
      toolbarbutton[type="menu-button"]

toolkit/content/widgets/colorpicker.xml#colorpickertile role="xul:colorpickertile"
  .colorpickertile

toolkit/content/widgets/colorpicker.xml#colorpicker-button role="xul:colorpicker"
  colorpicker[type="button"]

toolkit/content/widgets/popup.xml#panel role="xul:panel"
  panel
  toolkit/content/widgets/popup.xml#arrowpanel
    panel[type="arrow"]
    toolkit/content/widgets/datetimepopup.xml#datetime-popup
      #DateTimePickerPanel[active="true"]

toolkit/content/widgets/scale.xml#scale role="xul:scale"
  scale
  toolkit/content/widgets/videocontrols.xml#suppressChangeEvent
    .scrubber
    .volumeControl

toolkit/content/widgets/scale.xml#scalethumb role="xul:thumb"
  .scale-thumb

toolkit/content/widgets/text.xml#text-base role="xul:text"
  description
  toolkit/content/widgets/text.xml#text-label
    label
    toolkit/content/widgets/text.xml#label-control
      label[control]
      label.radio-label
      label.checkbox-label
      label.toolbarbutton-multiline-text

toolkit/content/widgets/text.xml#text-link role="xul:link"
  label.text-link
  label[onclick]

toolkit/content/widgets/textbox.xml#textbox role="xul:textbox"
  textbox
  toolkit/content/widgets/findbar.xml#findbar-textbox
    .findbar-textbox
  toolkit/content/widgets/numberbox.xml#numberbox
    textbox[type="number"]
  toolkit/content/widgets/textbox.xml#search-textbox
    textbox[type="search"]
  toolkit/content/widgets/textbox.xml#textarea
    textbox[multiline="true"]

toolkit/content/widgets/toolbarbutton.xml#toolbarbutton role="xul:toolbarbutton"
  toolbarbutton
  toolkit/content/widgets/toolbarbutton.xml#menu
    toolbarbutton[type="menu"]
    toolbarbutton[type="panel"]
  toolkit/content/widgets/toolbarbutton.xml#toolbarbutton-image
    browser/base/content/tabbrowser.xml#tabbrowser-close-tab-button
      .tab-close-button
  toolkit/content/widgets/toolbarbutton.xml#toolbarbutton-badged
    toolbarbutton.badged-button
    toolbarbutton.badged-button > toolbarbutton
  toolkit/content/widgets/toolbarbutton.xml#toolbarbutton-badged-menu
    toolbarbutton.badged-button[type="menu"]
    toolbarbutton.badged-button[type="panel"]
Summary: Move as more XBL accessibility roles into XULMap.h → Move more XBL accessibility roles into XULMap.h
Also this isn't part of bug 1428930 because of a reference in XULButtonAccessible::IsAcceptableChild:

toolkit/content/widgets/popup.xml#popup role="xul:menupopup"
  popup
  menupopup
  browser/base/content/tabbrowser.xml#tabbrowser-alltabs-popup
    #alltabs-popup
    .new-tab-popup
  browser/components/places/content/menu.xml#places-popup-base
    menupopup[placespopup="true"]
    browser/components/places/content/menu.xml#places-popup-arrow
      #BMB_bookmarksPopup
  toolkit/content/widgets/popup.xml#popup-scrollbars
    menulist > menupopup
This one should be simple enough to be re-included in bug 1428930.

(In reply to :Paolo Amadini from comment #0)
> toolkit/content/widgets/toolbarbutton.xml#toolbarbutton
> role="xul:toolbarbutton"
>   toolbarbutton
>   toolkit/content/widgets/toolbarbutton.xml#menu
>     toolbarbutton[type="menu"]
>     toolbarbutton[type="panel"]
>   toolkit/content/widgets/toolbarbutton.xml#toolbarbutton-image
Inherence was removed back in bug 1422934.
>     browser/base/content/tabbrowser.xml#tabbrowser-close-tab-button
This binding was removed (bug ?)
>       .tab-close-button
>   toolkit/content/widgets/toolbarbutton.xml#toolbarbutton-badged
>     toolbarbutton.badged-button
>     toolbarbutton.badged-button > toolbarbutton
>   toolkit/content/widgets/toolbarbutton.xml#toolbarbutton-badged-menu
>     toolbarbutton.badged-button[type="menu"]
>     toolbarbutton.badged-button[type="panel"]

(I am still looking at other ones)
And this one too.

(In reply to :Paolo Amadini from comment #0)
> toolkit/content/widgets/textbox.xml#textbox role="xul:textbox"
>   textbox
>   toolkit/content/widgets/findbar.xml#findbar-textbox
>     .findbar-textbox
There is only one instance of class="findbar-textbox" and it is set on <xul:textbox> too.
>   toolkit/content/widgets/numberbox.xml#numberbox
>     textbox[type="number"]
>   toolkit/content/widgets/textbox.xml#search-textbox
>     textbox[type="search"]
>   toolkit/content/widgets/textbox.xml#textarea
>     textbox[multiline="true"]
(In reply to :Paolo Amadini from comment #0)
> toolkit/content/widgets/colorpicker.xml#colorpickertile
> role="xul:colorpickertile"
>   .colorpickertile
> 
> toolkit/content/widgets/colorpicker.xml#colorpicker-button
> role="xul:colorpicker"
>   colorpicker[type="button"]

These will be gone with bug 1416363.

> toolkit/content/widgets/text.xml#text-link role="xul:link"
>   label.text-link
>   label[onclick]

I am not sure about this one. First of all, "label[onclick]" looks like a redundant selector because looks like every <label> with onclick= have class=text-link set. However, I am not sure checking the class name as a condition in CPP is a good idea. Unless we want to do that ...
(In reply to :Paolo Amadini from comment #0)
> toolkit/content/widgets/button.xml#button-base role="xul:button"
>   toolkit/content/widgets/button.xml#menu-button-base
>     toolkit/content/widgets/toolbarbutton.xml#menu-button
>       toolbarbutton[type="menu-button"]

This is the only "toolbarbutton" with a "xul:button" role, we should understand if this is intentional or just an unwanted side-effect of the current inheritance chain. Apart from this one, button and toolbarbutton can likely be ported straightforwardly.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> > toolkit/content/widgets/text.xml#text-link role="xul:link"
> >   label.text-link
> >   label[onclick]
> 
> I am not sure about this one. First of all, "label[onclick]" looks like a
> redundant selector because looks like every <label> with onclick= have
> class=text-link set. However, I am not sure checking the class name as a
> condition in CPP is a good idea. Unless we want to do that ...

I think checking the class name is fine, unless it's computationally expensive and shows up in Talos tests, which I doubt...

If you manage to audit the consumers and remove the "onclick" attribute check, it would be great!

I suggest filing a new bug for each binding or group of related bindings, like button and toolbarbutton, so we can better track potential regressions.
(In reply to :Paolo Amadini from comment #0)
> toolkit/content/widgets/button.xml#button-base role="xul:button"
>   browser/base/content/browser-tabPreviews.xml#ctrlTab-preview
>     .ctrlTab-preview
They are all <button>s.
>   toolkit/content/widgets/button.xml#button
>     button
>     toolkit/content/widgets/button.xml#button-repeat
>       button[type="repeat"]
>     toolkit/content/widgets/button.xml#menu
>       button[type="menu"]
>       button[type="panel"]
>   toolkit/content/widgets/button.xml#menu-button-base
>    
> browser/components/downloads/content/download.xml#download-subview-toolbarbutton
>       .subviewbutton.download
This was implemented in bug 1354532 and is bound to <toolbarbutton>s.
I would wonder they should be map to XULToolbarButtonAccessible anyway?
>     toolkit/content/widgets/button.xml#menu-button
>       button[type="menu-button"]
>     toolkit/content/widgets/toolbarbutton.xml#menu-button
>       toolbarbutton[type="menu-button"]
This is only used in webconsole.xul, which bug 1347127 is going to get rid of.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #6)
> > browser/components/downloads/content/download.xml#download-subview-toolbarbutton
> >       .subviewbutton.download
> This was implemented in bug 1354532 and is bound to <toolbarbutton>s.
> I would wonder they should be map to XULToolbarButtonAccessible anyway?
> >     toolkit/content/widgets/toolbarbutton.xml#menu-button
> >       toolbarbutton[type="menu-button"]
> This is only used in webconsole.xul, which bug 1347127 is going to get rid
> of.

Paolo, do you know who can answer the question above, i.e. is it ok to declare them as XULToolbarButtonAccessible instead? That will include role=xul:button in bug 1437873 and unblock it. Thanks!
Flags: needinfo?(paolo.mozmail)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #7)
> Paolo, do you know who can answer the question above, i.e. is it ok to
> declare them as XULToolbarButtonAccessible instead? That will include
> role=xul:button in bug 1437873 and unblock it. Thanks!

Which is basically the question from comment 5. Maybe Marco knows?
Flags: needinfo?(paolo.mozmail) → needinfo?(mzehe)
(In reply to :Paolo Amadini from comment #8)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #7)
> > Paolo, do you know who can answer the question above, i.e. is it ok to
> > declare them as XULToolbarButtonAccessible instead? That will include
> > role=xul:button in bug 1437873 and unblock it. Thanks!
> 
> Which is basically the question from comment 5. Maybe Marco knows?

Sorry, no, only that it probably had something to do with the tool bars in the front-end. AFAIK, surkov and gijs worked on these together.
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(mzehe)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Paolo Amadini from comment #5)
> (In reply to :Paolo Amadini from comment #0)
> > toolkit/content/widgets/button.xml#button-base role="xul:button"
> >   toolkit/content/widgets/button.xml#menu-button-base
> >     toolkit/content/widgets/toolbarbutton.xml#menu-button
> >       toolbarbutton[type="menu-button"]
> 
> This is the only "toolbarbutton" with a "xul:button" role, we should
> understand if this is intentional or just an unwanted side-effect of the
> current inheritance chain. Apart from this one, button and toolbarbutton can
> likely be ported straightforwardly.

(In reply to :Paolo Amadini from comment #8)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #7)
> > Paolo, do you know who can answer the question above, i.e. is it ok to
> > declare them as XULToolbarButtonAccessible instead? That will include
> > role=xul:button in bug 1437873 and unblock it. Thanks!
> 
> Which is basically the question from comment 5. Maybe Marco knows?

I'd say it's likely unwanted effect of something. Technically there's no big difference between XULButton or XULToolbarbutton, expect toolbarbutton provides extra group info for a button, which sounds like the right thing for menu items.

It'd be good to make a try build with the change, and ask Marco for feedback, pointing out where those elements are encountered in Firefox/Thunderbird UI, so we can make sure there's no regressions.
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to alexander :surkov from comment #10)
> > Which is basically the question from comment 5. Maybe Marco knows?
> 
> I'd say it's likely unwanted effect of something. Technically there's no big
> difference between XULButton or XULToolbarbutton, expect toolbarbutton
> provides extra group info for a button, which sounds like the right thing
> for menu items.
> 
> It'd be good to make a try build with the change, and ask Marco for
> feedback, pointing out where those elements are encountered in
> Firefox/Thunderbird UI, so we can make sure there's no regressions.

Let's move to bug 1437873 for the discussion, in the mean time, this is the try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6122d5286a714d596a7238fe44a075148e03b4e2
Bugs remain to be filed are

* role="xul:scale"
* role="xul:thumb"
* role="xul:menupopup"
* role="xul:pane"
* role="none"

role="outerdoc" should be included in the list per offline discussion w/ :bgrins.

when everything is done we would be able to gut nsAccessibilityService::CreateAccessibleByType()
Thanks Tim for filing the bugs and the patches!

Mike mentioned it could be a good idea to share the load on the reviews as we go forward. Neil, would you be comfortable stealing some of the reviews on the dependent bugs?
Flags: needinfo?(enndeakin)
All bugs filed with patches in flight (either in try or ready for review.)

We should use this bug to remove nsAccessibilityService::CreateAccessibleByType() and maybe nsCoreUtils::XBLBindingRole() once everything has landed.
ok
Flags: needinfo?(enndeakin)
Neil, do you need me to explicitly set all reviews to you?
(ni for the question above.)
Flags: needinfo?(enndeakin)
Yep, please do!
Flags: needinfo?(enndeakin)
With bug 1442029, there are no more role attributes on XBL bindings in the tree.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.