1412369 - Remove the 'panebutton' binding

Status: RESOLVED DUPLICATE of bug 1379338

(Toolkit :: Themes, task, P3)

Description

[Brian Grinstead [:bgrins]]

This binding is similar to 'viewbutton' - it appears to be used to more easily control the display of radio buttons cross-platform: https://dxr.mozilla.org/mozilla-central/rev/aa958b29c149a67fce772f8473e9586e71fbdb46/toolkit/content/widgets/preferences.xml#1382.

Once we consolidate the radio bindings (Bug 1411640) and have an example to follow for 'viewbutton' (Bug 1410540), we should be able to remove this one as well.

Comment 1

[Brian Grinstead [:bgrins]]

It looks like the one potential complication may be that it has role="xul:listitem" even though it extends radio (which has role="xul:radiobutton").

The style is attached here: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#1144 and the node is created in _makePaneButton: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#752.

This gets called and added into the UI when opening a dialog in the preferences. For example, if you open about:preferences, scroll down to "Fonts & Colors" then click the "Advanced" button, that opens a modal dialog. Inside of that dialog before any content is this markup (which isn't visible on screen):

<xul:windowdragbox xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" orient="vertical">
  <xul:radiogroup anonid="selector" orient="horizontal" class="paneSelector chromeclass-toolbar" role="listbox" value="" collapsed="true">
    <xul:radio pane="FontsDialogPane" label="" focused="false" selected="true"/>
</xul:radiogroup><!-- Expose to accessibility APIs as a listbox -->
</xul:windowdragbox>

In this case, the radio[pane=FontsDialogPane] gets the panebutton binding (which also has the listitem role). :yzen, two questions:

1) Should this radiogroup / radio actually have the listbox role in this case? I'm not sure exactly what the purpose of these elements are so I don't know what the role should be, but there's a comment that indicates it's on purpose.
2) If so, can we remove the panebutton binding and instead set the role attribute directly on the xul:radio?  So instead of specifying the role in the binding definition it would be read directly from the XUL element in the DOM.

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> It looks like the one potential complication may be that it has
> role="xul:listitem" even though it extends radio (which has
> role="xul:radiobutton").
> 
> The style is attached here:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#1144
> and the node is created in _makePaneButton:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> preferences.xml#752.
> 
> This gets called and added into the UI when opening a dialog in the
> preferences. For example, if you open about:preferences, scroll down to
> "Fonts & Colors" then click the "Advanced" button, that opens a modal
> dialog. Inside of that dialog before any content is this markup (which isn't
> visible on screen):
> 
> <xul:windowdragbox
> xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> orient="vertical">
>   <xul:radiogroup anonid="selector" orient="horizontal" class="paneSelector
> chromeclass-toolbar" role="listbox" value="" collapsed="true">
>     <xul:radio pane="FontsDialogPane" label="" focused="false"
> selected="true"/>
> </xul:radiogroup><!-- Expose to accessibility APIs as a listbox -->
> </xul:windowdragbox>
> 
> In this case, the radio[pane=FontsDialogPane] gets the panebutton binding
> (which also has the listitem role). :yzen, two questions:
> 
> 1) Should this radiogroup / radio actually have the listbox role in this
> case? I'm not sure exactly what the purpose of these elements are so I don't
> know what the role should be, but there's a comment that indicates it's on
> purpose.

I am not sure about the chosen semantics here either. I would think that if there is only 1 selection possible, radio group would be fine. I'm cc'ing Marco and Alex, in case they can remember the context for this implementation.

> 2) If so, can we remove the panebutton binding and instead set the role
> attribute directly on the xul:radio?  So instead of specifying the role in
> the binding definition it would be read directly from the XUL element in the
> DOM.

I believe setting the role directly should work just fine.

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Marco, Alex see comment 2

Comment 4

[Marco Zehe (:MarcoZ)]

Sheesh, I don't even remember seeing this markup when I last looked at XUL files to fix accessibility bugs. Is this a color picker or something?

In any case: xul:radio already has a role definition, if it is a radio button, it doesn't need an extra role set on the xul:radio element.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Marco Zehe (:MarcoZ) from comment #4)
> Sheesh, I don't even remember seeing this markup when I last looked at XUL
> files to fix accessibility bugs. Is this a color picker or something?

No, this is a hidden element inside of dialogs that gets opened in about:preferences. I'm not actually sure what the purpose of this element is.

Maybe there used to be multiple options for categories inside of the dialog but now there is only one. Myk, is this something you ran into when looking into preferences lately? I remember you mentioned something along these lines - I'm not sure if this is it.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

<prefwindow> windows used to contain multiple <prefpane> panes, each of which showed a different set of preferences, with "tabs" across the top of the window for selecting the pane to view.  The element in question represents such a tab.

Firefox no longer contains any prefwindows with multiple prefpanes.  All such windows (colors.xul, connections.xul, donottrack.xul, fonts.xul, languages.xul, and both instances of sanitize.xul) contain a single prefpane each.  So it's no longer necessary to display any UI for switching between prefpanes, and I've removed the code from my work-in-progress branch to remove XBL from preferences in bug 1379338.

Thus I think this bug can depend on that one, and the binding in question can be simply removed once that bug lands.

Comment 7

[Brian Grinstead [:bgrins]]

Thanks Myk, that makes things easier. Clearing remaining needinfo since it sounds like we won't need to worry about the accessibility role in this case and we can

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> and we can

... use this bug to simply remove the unused binding once that bug lands

Comment 9

[Brian Grinstead [:bgrins]]

This binding will be removed in Bug 1379338