1429940 - Use HTML headings to improve accessibility of groups in Preferences

Status: VERIFIED FIXED

(Firefox :: Settings UI, enhancement, P1)

Description

[Tim Nguyen :ntim]

The caption binding can be removed, since the icon feature isn't being used.

https://raw.githubusercontent.com/mozilla/gecko-dev/master/toolkit/content/widgets/groupbox.xml

Comment 1

[Brian Grinstead [:bgrins]]

Note - there is also an <html:caption> element that won't be affected by this. This is about the <xul:caption> element for example: https://dxr.mozilla.org/mozilla-central/search?q=path%3Axul+%22%3Ccaption%22&redirect=true

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Given that there is also an <xul:label> inside <children>, at the minimum we would need to identify and remove usage of <caption label=""> without child nodes here too.

      <children>
        <xul:image class="caption-icon" xbl:inherits="src=image"/>
        <xul:label class="caption-text" flex="1"
                   xbl:inherits="default,value=label,crop,accesskey"/>
      </children>

Thankfully, it looks manageable

https://dxr.mozilla.org/mozilla-central/search?q=caption+label%3D&redirect=false

Comment 3

[:Paolo Amadini]

The icon is used in pageInfo.xul to make the box collapsible, but it makes no sense there anymore, and the feature should definitely be removed.

https://dxr.mozilla.org/mozilla-central/search?q=collapsable&redirect=false

Comment 4

[:Paolo Amadini]

There is also <children includes="caption"/>:

https://dxr.mozilla.org/mozilla-central/rev/00bdc9451be6557ccce1492b9b966d4435615380/toolkit/content/widgets/groupbox.xml#15

So, whatever we do here should handle both groupbox and caption at the same time.

Comment 5

[:Paolo Amadini]

Actually, I don't really see a reason for extending "basetext" here:

https://dxr.mozilla.org/mozilla-central/rev/00bdc9451be6557ccce1492b9b966d4435615380/toolkit/content/widgets/groupbox.xml#23

As far as I know, the label inside the content should work just as well for accessibility, and I took a look at the other consumers and it doesn't look like we use the other properties of the base control.

Brian, Tim, do you think these could be candidates for conversion to Custom Elements, in particular with regard to how to handle selective <children/> tags, and preserving existing styles? Or do we need Shadow DOM here?

Comment 6

[Brian Grinstead [:bgrins]]

Could we remove the caption binding but continue to have the caption tag (so as to not change groupbox [includes])?

This would require manually creating <label> tags in the caption[label] cases Tim mentions in Comment 2. And then I guess the only other thing we'd be missing here is xbl:inherits on the caption (if calling code changed an attribute on the caption and expected it to propagate to the label, we'd instead need to set the attribute directly on the label).

Comment 7

[Brian Grinstead [:bgrins]]

The only thing I'd be unsure about are it no longer implementing nsIDOMXULLabeledControlElement, which is checked in a few places like:

- https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/accessible/base/nsTextEquivUtils.cpp#310
- https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/accessible/generic/Accessible.cpp#777
- https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/toolkit/content/widgets/text.xml#73

It's not obvious to me why the <caption> needs to be labeled in this way, but we should check with accessibility folks before making the change.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I would say stick to the original plan (i.e. converting them to <label>) is optimal. If that's too much work we can consider keeping a very small XBL (with <children>) and convert it to the custom element when we are ready.

Comment 9

[:Paolo Amadini]

That's feasible, and probably a similar amount of work, just in the consumers instead of the widgets layer.

We have to choose now whether we'll remove the <groupbox> element entirely or we'll turn it into a Custom Element, since it is basically used in the same places as <caption> and I wouldn't want us to do the work twice.

The main consumer is Preferences, so looping in Jared as well.

https://searchfox.org/mozilla-central/search?q=%3Cgroupbox&path=

If we're willing to lose some styling in other windows, or reimplement it with a few more tags, then we can get rid of the custom markup entirely, and probably for Preferences we'll have something like:

<vbox class="groupbox">
  <description class="caption" value="..."/>
  <...inner elements.../>
</vbox>

(Note that in this bug we talked about converting to "label" but the best element would still be "description" because it's not used to label a specific control.)

The wrapping box for the inner elements would likely be unneeded because their orientation is basically always vertical anyways. The outer box may even be unneeded in some cases.

With a Custom Element, this might look just slightly different:

<groupbox>
  <description class="caption" value="..."/>
  <...inner elements.../>
</groupbox>

While similar, the Custom Element could look up the first description with class="caption", leveraging the fact it wouldn't be limited to just <caption> elements like XBL, and place any other markup or wrapping box as needed.

If we don't anticipate more complex layout and we don't need the expressiveness of a custom tag, then I'd say to convert everything to plain "vbox" and "description". Jared, anything we're missing here?

Comment 10

[Brian Grinstead [:bgrins]]

We could also continue to use the groupbox tag (without any XBL / CE) but style it like a vbox. Something like:

<groupbox">
  <description class="caption" value="..."/>
  <...inner elements.../>
</groupbox>

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

No I don't think you're missing anything there. As long as we can keep the same visual/a11y output then I'm fine with switching to reimplementing the groupbox with vbox/etc.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> We could also continue to use the groupbox tag (without any XBL / CE) but
> style it like a vbox. Something like:
> 
> <groupbox">
>   <description class="caption" value="..."/>
>   <...inner elements.../>
> </groupbox>


(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> No I don't think you're missing anything there. As long as we can keep the
> same visual/a11y output then I'm fine with switching to reimplementing the
> groupbox with vbox/etc.

there's a bunch of cases where groupbox has vertical and horizontal orientations (https://dxr.mozilla.org/mozilla-central/search?q=%3Cgroupbox+orient&redirect=true), so inner elements have to be wrapped by v/hbox. It doesn't have to happen behind the scenes though as it's implemented now, i.e. the author can take care of it explicitly. There's no large amount of groupboxes in the code to convert all of them.

for a11y you either should put role="group" on a box element (groupbox) and aria-labelledby pointing to a caption, and role="label" on a description element (caption), which is bulky, or keep groupbox tagname to reuse its a11y c++ implementation and use <label class="caption"> (instead description) to pick up a proper a11y role.

Comment 13

[alexander :surkov (:asurkov)]

I filed bug 1493844 to get rid of anonymous .groupbox-body element

Comment 14

[alexander :surkov (:asurkov)]

Paolo, how do you think to remove groupboxes from Preferences? replace them on HTML fieldsets or something else?

Comment 15

[:Paolo Amadini]

As noted in bug 1493844, I have a work in progress patch implementing the first suggestion in comment 9.

(In reply to alexander :surkov (:asurkov) from bug 1493844 comment #19)
> vbox/description will break accessibility. You will either need to add ARIA
> role="group" and @aria-labelledby, or I would suggest to with
> groupbox/label, that should work for a11y.

The patch that I have now uses "vbox" without role="group". The resulting XHTML looks pretty much like an ordinary page with headers of the "caption" class delimiting each section. In fact, we already use the "header-name" class to delimit even higher level groups ("General", "Language and Appearance", "Files and Applications", ...) without using role="group".

Is this something that accessibility tools will recognize, and handle gracefully like when navigating an ordinary HTML document with headers? Is there a CSS property that allows the tools to recognize these two classes as headers of different levels? Maybe we should switch to HTML header elements?

I'll clean up and post the patch so you can test locally as well.

Comment 16

[:Paolo Amadini]

Attachment: Bug 1429940 - Part 1 - Remove uses of "caption" and "groupbox" from Preferences. r=jaws

Comment 17

[:Paolo Amadini]

Since Preferences is apparently the only consumer of "groupbox" in-content, this allows many styles to be removed, and the general structure to be simplified.

The main change is that there is no 4px shift on the main text inside the sections. It's a small change, but I think this better matches the current version of the Photon Design System, and looks better in general. Jared, feel free to ping the UX team about the change if you think it's necessary. Funnily enough, the vertical margins that I guessed for the CSS already match what is suggested in <https://design.firefox.com/photon/visuals/grid.html#spacing>.

There are currently glitches with the subdialog title, and the focus ring of radio buttons is cropped on Mac. I may address these in a separate patch that can land first, as we need to stop using "groupbox" for the sub dialogs as well.

Comment 18

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2465a33867ec7d350a0663979a4ffcf855da2ec

Comment 19

[:Paolo Amadini]

Attachment: Bug 1429940 - Part 2 - Remove the "prefpane" class and unneeded elements. r=jaws

Depends on D7581

Comment 20

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69e2bab87d804d7ec19aff00880f53644044e4a3

Comment 21

[:Paolo Amadini]

This is still a work in progress, but if you apply Part 2 and its dependencies (you can pull from the tryserver build), and open the "colors.xul" subdialog in Preferences, the elements in the main area of the dialog are intermittently not present. This happens with other dialogs as well. If I wrap these elements in a single "vbox" again, this problem disappears.

Is there a known bug with adding multiple elements as <children>, or is this something we should investigate?

Comment 22

[:Paolo Amadini]

Setting qe-verify+ since this ended up being a minor restyling as well as a simplification of Preferences code and styles.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #21)
> This is still a work in progress, but if you apply Part 2 and its
> dependencies (you can pull from the tryserver build), and open the
> "colors.xul" subdialog in Preferences, the elements in the main area of the
> dialog are intermittently not present. This happens with other dialogs as
> well. If I wrap these elements in a single "vbox" again, this problem
> disappears.
> 
> Is there a known bug with adding multiple elements as <children>, or is this
> something we should investigate?

I highly doubt it's related, but coincidentally we are looking into other XBL <children> insertion issues in Bug 1495946. Keeping ni? set so I can look more closely at this later.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to :Paolo Amadini from comment #15)
> As noted in bug 1493844, I have a work in progress patch implementing the
> first suggestion in comment 9.
> 
> (In reply to alexander :surkov (:asurkov) from bug 1493844 comment #19)
> > vbox/description will break accessibility. You will either need to add ARIA
> > role="group" and @aria-labelledby, or I would suggest to with
> > groupbox/label, that should work for a11y.
> 
> The patch that I have now uses "vbox" without role="group". The resulting
> XHTML looks pretty much like an ordinary page with headers of the "caption"
> class delimiting each section. In fact, we already use the "header-name"
> class to delimit even higher level groups ("General", "Language and
> Appearance", "Files and Applications", ...) without using role="group".

It could turn out that AT users are not just aware of the structure, i.e. they expect nothing and also see nothing. I'd say it makes sense to verbally describe the page structure to Marco and check with him, whether a screen reader gives same output.

Having said that I feel reluctant on replacing groupbox/caption on vbox/description pair, because the latter construction has no semantics. Of course ARIA attributes fix it, but in general ARIA should be used iff you can't use native markup for some reason. You may argue that there's no point to worry about XUL, but these vbox/description might go unnoticed during XUL to HTML transformation, and thus not converted to HTML fieldset elements.

> Is this something that accessibility tools will recognize, and handle
> gracefully like when navigating an ordinary HTML document with headers? Is
> there a CSS property that allows the tools to recognize these two classes as
> headers of different levels? Maybe we should switch to HTML header elements?

If you want headers, then either HTML:h elements or ARIA role="heading" + @aria-level for a heading number should be used (if HTML:h doesn't work for some reason). Otherwise documents structured by headings should work just fine.

Comment 25

[Brian Grinstead [:bgrins]]

Is there a downside to continuing to use the groupbox and caption tags without XBL (and simplifying CSS / markup where possible), rather than going to vbox / description?

Comment 26

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #25)
> Is there a downside to continuing to use the groupbox and caption tags
> without XBL (and simplifying CSS / markup where possible), rather than going
> to vbox / description?

The obvious downside would be that we’d have to maintain 2 custom elements and/or some code that handles a11y for those elements. Also, the less XUL elements there are now, the easier it is to convert them to HTML later on, so if we can go with vbox+description I believe that it would be a win.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #26)
> (In reply to Brian Grinstead [:bgrins] from comment #25)
> > Is there a downside to continuing to use the groupbox and caption tags
> > without XBL (and simplifying CSS / markup where possible), rather than going
> > to vbox / description?
> 
> The obvious downside would be that we’d have to maintain 2 custom elements
> and/or some code that handles a11y for those elements.

I suggested to go with <groupbox> and <label> tagnames, since it doesn't require changes on a11y side. It is also not necessary to have custom element for groupbox, just a tagname + styling should be fine. XUL a11y code is changed quite rare, so it's maintenance fee is rather minimal.

> Also, the less XUL
> elements there are now, the easier it is to convert them to HTML later on,
> so if we can go with vbox+description I believe that it would be a win.

XUL:groupbox is matched by HTML:fieldset/caption, so it should be a rather straightforward conversion.

The disadvantage of vbox+description approach is you will have to use ARIA to define role and also connect the elements by @aria-labelledby, which is a less optimal solution, because a11y has to process @aria-labelledby IDs (I don't think this is a problem though).

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #26)
> (In reply to Brian Grinstead [:bgrins] from comment #25)
> > Is there a downside to continuing to use the groupbox and caption tags
> > without XBL (and simplifying CSS / markup where possible), rather than going
> > to vbox / description?
> 
> The obvious downside would be that we’d have to maintain 2 custom elements
> and/or some code that handles a11y for those elements. Also, the less XUL
> elements there are now, the easier it is to convert them to HTML later on,
> so if we can go with vbox+description I believe that it would be a win.

I meant to do basically what Alex is suggesting in Comment 27. No XBL and no CE, but continue to use <groupbox> tag. Although I mentioned also continuing to use <caption>, sounds like there's a consensus to drop that and switch to <label> as Alex suggests. Is there a downside to this approach compared to dropping the groupbox tag entirely?

Comment 29

[:Paolo Amadini]

Tim has a good point, and I can add that the advantage of removing XUL elements entirely is not only about the code maintenance, but about the fact that developers need to know about all the elements we keep and understand their behavior, which is often undocumented, like the accessibility behavior of "groupbox" in this case.

For Preferences, I'm now trying the HTML headings approach. This seems to work well and even has a slightly simpler markup. I'd go for this option if this doesn't regress the ability to navigate Preferences.

Comment 30

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=005f40a1cd1874b97b4f5cbbf2ac4f391537f408

Comment 31

[:Paolo Amadini]

Marco, I'm trying to determine if the changes I'm making in Preferences to try and remove the "groupbox" XUL element would regress or improve the navigation of categories when using accessibility technologies.

The current version in Release uses the "groupbox" element, mixed with labels with a "heading" class. I made an earlier change in this bug that just removes "groupbox", and the latest version uses HTML elements to create headings of two different levels. In the three cases, the preference pages look the same visually, because we are using bold and varying font sizes for these headings. "General", "Language and Appearance", "Files and Applications" are examples of headings with larger font sizes, meant as first level groupings.

In comment 30 I posted links to tryserver builds where the "General" page is updated to use the HTML headings approach, and other pages like "Home" and "Search" use normal labels. Can you compare these to the current Release, and tell us if there are differences in the ease of navigation, and whether one works substantially better than the others?

Comment 32

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #29)
> Tim has a good point, and I can add that the advantage of removing XUL
> elements entirely is not only about the code maintenance, but about the fact
> that developers need to know about all the elements we keep and understand
> their behavior, which is often undocumented, like the accessibility behavior
> of "groupbox" in this case.

I agree the lack of documentation about this behavior is an issue, but if this is more-or-less a bridge to getting to html:fieldset then keeping the current tag name (with no XBL and no CE) seems like the easier/lower risk approach. If it turns out that the vbox implementation is OK for accessibility tools that's great, but if not and we need to tag it up with aria anyway it could be an easier first step to continue to use xul:groupbox.

By the way, you can use the Accessibility DevTool in the Browser Toolbox (or in the page's toolbox for about:preferences) to see what the accessibility tree looks like before/after a change.

Comment 33

[Tim Nguyen :ntim]

> I agree the lack of documentation about this behavior is an issue, but if this is more-or-less a bridge to getting to html:fieldset then keeping the current tag name (with no XBL and no CE) seems like the easier/lower risk approach

Have you considered using html:fieldset directly instead of groupbox? 

It allows removing the XULGroupboxAccessible and replacing XUL elements with HTML has been done before (bug 1416363). It's a bit more work needed with the styling, but it's the end goal anyway.

I also support replacing caption with h2 if it turns out not to regress anything, otherwise XUL label is perfectly fine as it's used in other spots too.

Comment 34

[Marco Zehe (:MarcoZ)]

The removal of the groupbox context causes screen reader users to no longer have proper context when tabbing through. Previously, or with proper fieldset/legend constructs, these group headings would speak automatically when entering them. Simply replacing them with headings doesn't give the same result. The reason is that we treat such options windows as an application and thus cause screen readers to disengage their reading modes. However, with these new types of options windows, it may make sense to stop doing that and cause screen readers to treat these like HTML pages with proper form controls rather than application-style dialogs. CC'ing Jamie for a second opinion.

Comment 35

[:Paolo Amadini]

Marco, that makes sense, and interestingly enough, I did a web search for the "application" role and it showed as the first result your article that warns about using it for cases that mostly look like documents, even with form controls:

https://www.marcozehe.de/2012/02/06/if-you-use-the-wai-aria-role-application-please-do-so-wisely/

Given that in Preferences we have many secondary labels that just follow the flow of the document, I would expect that the "document" role together with headings could make things easier to understand and navigate. I also imagine that in the current situation, what we considered top-level headings visually were actually lost in the accessible tree.

I just started a new tryserver build, link below, with headings and role="document". Is the "General" page easier to navigate this way?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=926b156172704e826c881f4084c55068ed256613

Comment 36

[Marco Zehe (:MarcoZ)]

(In reply to :Paolo Amadini from comment #35)
> Marco, that makes sense, and interestingly enough, I did a web search for
> the "application" role and it showed as the first result your article that
> warns about using it for cases that mostly look like documents, even with
> form controls:
> 
> https://www.marcozehe.de/2012/02/06/if-you-use-the-wai-aria-role-application-
> please-do-so-wisely/

Correct, and our options dialog is actually one where having it as a web document, makes a lot of sense.

BUT...

Users, especially beginners, tend to tab through things rather than exploring them, especially when going through something the first time. And since they expect this desktop application to behave like most other desktop applications they're learning to use, we MUST NOT regress the previous behavior of giving enough context when tabbing through this dialog.

Fortunately, web technologies and especially ARIA give us all the options we need to accomplish this.

> Given that in Preferences we have many secondary labels that just follow the
> flow of the document, I would expect that the "document" role together with
> headings could make things easier to understand and navigate. I also imagine
> that in the current situation, what we considered top-level headings
> visually were actually lost in the accessible tree.

Yes, they were. But now, the context when the screen reader speaks the focused element, is lost. For example, in a regular Nightly, when tabbing from the Search box into the General area, I hear:

"Start group", "Restore previous session checkbox checked".

In the try build, the information that I tabbed into the "Start group" is lost.

However, with a little ARIA spice, we can make it say:

"General group", "Start group", "Restore previous session checkbox checked".

That way, using the tab key, the user still gets all the context, without having to switch back and forth between browse mode and forms/focus mode, which normally gets engaged when tabbing to interactive controls.

There are two ways to accomplish this:

One is to use fieldset/legend elements like ntim suggested in comment #33. This is the more intrusive approach, since it might require re-styling some thing to make them still look pretty.

The other is to use WAI-ARIA group role and aria-labelledby, and sometimes in addition, aria-describedby, as is shown several times throughout these dialogs.

- The group role is put on a container element that puts related controls together.
- This same element gets aria-labelledby pointing to the ID of the H2 that labels this group. Yes, these H2 might have to receive IDs for that that weren't there before, but so...

- The overarching groups that have an H1 also get this treatment: role="group" and aria-labelledby pointing to the corresponding H1's ID.

- If there is additional text to either of these, like a sub text to the h1 or h2, that gets an ID, too, and the group element gets aria-describedby pointing to that ID, so the screen reader would speak this information *after* the label and group role, giving all the context necessary.

An example where all of this is really well impemented is the Security and Privacy Settings area, where for the different permissions like Camera etc., everything is labelled and described in a way that makes tabbing through incredibly efficient. Jamie put a lot of effort into making this complex set of controls really talk well and intuitively.

Doing this properly will also give us an edge over Chrome, since Chrome's Settings dialogs are all webby, but not well labelled otherwise. If we do this right, we will satisfy both those who like to use browse mode and navigate by headings, but also those who want to tab through and be efficient that way.

It's worth the effort, and it prevents a regression, because we already have a lot of this in place right now which would get lost by the implementation in the try builds. So, let's do it!

All of this is written after I talked to Jamie via voice chat, so removing the NI for him.

Comment 37

[Marco Zehe (:MarcoZ)]

Sort of pseudo code for the pattern would look like this:

<html><head><title>Testing new Settings dialog markup</title></head>
<body>
<h1 id="Group1Heading">General</h1>
<div role="group" aria-labelledby="Group1Heading">
  <h2 id="SubGrup1Heading">Startup</h2>
  <div role="group" aria-labelledby="SubGrup1Heading">
    <input type="checkbox" id="RestorePreviousSession"><label for="RestorePreviousSession">Restore previous session</label></input>
  </div>
</div>
</body>
</html>

Comment 38

[:Paolo Amadini]

That's great information, thanks! I agree it's worth improving accessibility here while we rethink the use of groupbox, and I may also be able to set the ARIA attributes automatically based on the page structure when the page loads, if this doesn't regress performance, so we keep the markup simpler and we reduce the chance of mistakes in the identifiers when adding new sections.

I have two questions. The first, does this work the same if the heading is inside the group, similarly to a fieldset structure?

<div role="group" aria-labelledby="Group1Heading">
  <h1 id="Group1Heading">General</h1>
  <div role="group" aria-labelledby="SubGrup1Heading">
    <h2 id="SubGrup1Heading">Startup</h2>
    <input type="checkbox" id="RestorePreviousSession"><label for="RestorePreviousSession">Restore previous session</label></input>
  </div>
</div>

This would make things much simpler, because we already have a wrapping element that we use to show and hide sections during search.

The second, should we keep role="document" on the page element, so that navigation by headers is allowed, or is it the same with the default "application" role?

Comment 39

[Marco Zehe (:MarcoZ)]

(In reply to :Paolo Amadini from comment #38)
> I have two questions. The first, does this work the same if the heading is
> inside the group, similarly to a fieldset structure?

Yes, this works the same. aria-labelledby always takes the exact ID of an element, and it can be a child element of the element being labelled.

> The second, should we keep role="document" on the page element, so that
> navigation by headers is allowed, or is it the same with the default
> "application" role?

Please keep role="document".

Comment 40

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #29)
> Tim has a good point, and I can add that the advantage of removing XUL
> elements entirely is not only about the code maintenance, but about the fact
> that developers need to know about all the elements we keep and understand
> their behavior, which is often undocumented, like the accessibility behavior
> of "groupbox" in this case.
> 
> For Preferences, I'm now trying the HTML headings approach. This seems to
> work well and even has a slightly simpler markup. I'd go for this option if
> this doesn't regress the ability to navigate Preferences.

<vbox> doesn't "remove XUL" any more than plain <groupbox> would, and it has the downside of being less semantic, so let's not do that.

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #26)
> Also, the less XUL
> elements there are now, the easier it is to convert them to HTML later on,
> so if we can go with vbox+description I believe that it would be a win.

I disagree. If anything, <groupbox> makes it easier to transition to <fieldset> later since the concepts are basically the same.

Comment 41

[:Paolo Amadini]

(In reply to Dão Gottwald [::dao] from comment #40)
> I disagree. If anything, <groupbox> makes it easier to transition to
> <fieldset> later since the concepts are basically the same.

According to Marco's suggestion, it's better not to transition to <fieldset> later, but to HTML elements like <div> and <h1> with appropriate ARIA attributes. As such, we can use <vbox> as a way of transitioning to <div>.

Comment 42

[Marco Zehe (:MarcoZ)]

(In reply to :Paolo Amadini from comment #41)
> (In reply to Dão Gottwald [::dao] from comment #40)
> > I disagree. If anything, <groupbox> makes it easier to transition to
> > <fieldset> later since the concepts are basically the same.
> 
> According to Marco's suggestion, it's better not to transition to <fieldset>
> later, but to HTML elements like <div> and <h1> with appropriate ARIA
> attributes. As such, we can use <vbox> as a way of transitioning to <div>.

Not exactly, it is just the easier route, since fieldset *always* requires a *legend* element as a child, which in turn can contain an h1 or h2 element, but it means some extra styling might be needed, and I hear from a lot of people that fieldset and legend aren't as flexible in styling as divs are, which is rooted in the long history of these elements. But a fieldset which contains a legend with an h1 child accomplishes the same as a div with aria-group and aria-labelledby pointing to the heading for screen readers.

Comment 43

[Marco Zehe (:MarcoZ)]

So, my previous example without any ARIA which accomplishes the same thing looks like this:

<html><head><title>Testing new Settings dialog markup</title></head>
<body>
<fieldset>
  <legend><h1 id="Group1Heading">General</h1></legend>
  <fieldset>
    <legend><h2 id="SubGrup1Heading">Startup</h2></legend>
    <input type="checkbox" id="RestorePreviousSession"><label for="RestorePreviousSession">Restore previous session</label></input>
  </fieldset>
</fieldset>
</body>
</html>

Comment 44

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #41)
> (In reply to Dão Gottwald [::dao] from comment #40)
> > I disagree. If anything, <groupbox> makes it easier to transition to
> > <fieldset> later since the concepts are basically the same.
> 
> According to Marco's suggestion, it's better not to transition to <fieldset>
> later, but to HTML elements like <div> and <h1> with appropriate ARIA
> attributes. As such, we can use <vbox> as a way of transitioning to <div>.

I'm having a hard time getting to the meat of Marco's comment. At the end of the day, fieldset is established web tech and should be accessible, and <div role="group" aria-labelledby="Group1Heading"><h1 id="Group1Heading">General</h1> is very awkward to write. What exactly is missing from fieldset and can we fix that rather than throwing the baby out with the bath water? We should try not to end up with a div soup where authors then need to make an effort to fix up accessibility, as opposed to semantic markup where accessibility is basically built in.

Comment 45

[:Paolo Amadini]

Thanks for the clarification, Marco! In this case, we can probably use <groupbox> and <caption> without a Custom Element but with the accessibility code in place, which we will later probably transition to <fieldset> and <legend>.

This work then depends on inlining the structure of other uses of <groupbox> where we want the current styling. If I remember correctly, the Print dialog is the only place where we continue to use <groupbox> with the native styling, so I should work on that first. We can then remove the shared styling and use <groupbox> just as an unstyled container here.

Comment 46

[Marco Zehe (:MarcoZ)]

Just make sure the <caption> is always picked up by the <groupbox> accessibility code, which usually means it has to be inside the groupbox. If it is not, you need to use aria-labelledby to point at the caption's ID. But you can check all this with the Accessibility Inspector: If the element with ROLE_GROUPING has a name of NULL, it's not being picked up. If it has a sensible name, it is correct.

Comment 47

[:Paolo Amadini]

(In reply to Dão Gottwald [::dao] from comment #44)
> is very awkward to write

Not a problem if this gives us what we need, and as I mentioned it could be made simpler with code that sets the attributes.

Anyways, it looks like <fieldset> and <groupbox> can be combined with <h1> and still gives the result we want, so we can go for that.

Comment 48

[Marco Zehe (:MarcoZ)]

(In reply to Dão Gottwald [::dao] from comment #44)
> I'm having a hard time getting to the meat of Marco's comment. At the end of
> the day, fieldset is established web tech and should be accessible,

Yes, it is accessible. I was referring to the problem with styling it properly which, again, I hear a lot of complaining about from web developers across all browsers.

Hope this clarifies things. See also my comment in comment #43.

Comment 49

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #46)
> Just make sure the <caption> is always picked up by the <groupbox>
> accessibility code, which usually means it has to be inside the groupbox. If
> it is not, you need to use aria-labelledby to point at the caption's ID. But
> you can check all this with the Accessibility Inspector: If the element with
> ROLE_GROUPING has a name of NULL, it's not being picked up. If it has a
> sensible name, it is correct.

a tiny clarification: from a11y standpoint, the <caption> element is not accessible, but its child <label> is, so just replace <caption> by <label> element, when it comes to <groupbox> labelling. Recap: <groupbox>+<label> should be a recipe that works and is accessible, and also can be easily converted to HTML <fieldset>+<legend> bundle.

Comment 50

[:Paolo Amadini]

Attachment: Bug 1429940 - Part 2 - Use HTML headings inside the "label" element for labeling "groupbox" elements in Preferences, instead of the "caption" element. r=MarcoZ,dao,flod

This makes it possible to navigate by headings when using accessibility technology.

Comment 51

[:Paolo Amadini]

Attachment: Bug 1429940 - Part 1 - Use the first "label" element instead of the one inside "caption" to describe the "groupbox" element. r=Jamie

Comment 52

[:Paolo Amadini]

James, do you think you can review the C++ changes that Alex originally suggested?

I should have probably posted these patches earlier, but I wanted to do some manual testing and I didn't get around to do it until now. Here is the first tryserver build, let's see if regression tests pass:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d9671696b9020fc198b19680c7732f3e8b656d7

This doesn't add "h1" headings yet, I may do that in a separate changeset.

Comment 53

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #52)
> This doesn't add "h1" headings yet, I may do that in a separate changeset.

I filed bug 1507806 for this because, once this bug lands, the other one can be developed on artifact builds.

Comment 54

[:Paolo Amadini]

Is there still a requirement for a localization peer review for ".ftl" file changes?

There's now one in this patch that fixes a regression from bug 1502396.

Comment 55

[Francesco Lodolo [:flod]]

I just replied in Phabricator, but

> Is there still a requirement for a localization peer review for ".ftl" file
> changes?

Yes, it's still in place. Also note that we need a new message ID when you change (add, remove, rename) attributes in a string.

> There's now one in this patch that fixes a regression from bug 1502396.

"one"?

Comment 56

[:Paolo Amadini]

(In reply to Francesco Lodolo [:flod] from comment #55)
> Yes, it's still in place. Also note that we need a new message ID when you
> change (add, remove, rename) attributes in a string.

Okay, I've used the "string-name2 =" convention which seems the most common.

Comment 57

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df468d03b50f08befdf781c1cf82cac4549d6086

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/781e474c7571
Part 1 - Use the first "label" element instead of the one inside "caption" to describe the "groupbox" element. r=Jamie
https://hg.mozilla.org/integration/mozilla-inbound/rev/004331df8823
Part 2 - Use HTML headings inside the "label" element for labeling "groupbox" elements in Preferences, instead of the "caption" element. r=MarcoZ,dao,jaws,flod

Comment 59

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/781e474c7571
https://hg.mozilla.org/mozilla-central/rev/004331df8823

Comment 60

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4571

Comment 61

[:Paolo Amadini]

I'm landing a trivial fix since I forgot to address the last review comment in part 1. It's a bit more difficult now to keep on top of all the comments when working on multiple patches, since they're spread out across various pages.

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f13bbf0e59ae
Null-check the GetContent call on the parent. r=Jamie

Comment 63

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f13bbf0e59ae

Comment 64

[[github robot]]

Commits pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/d3c36744cf9e69e7c78248f7b1b0c78935cfb7d3
Backport Bug 1429940 - Use HTML headings inside the label element for labeling groupbox elements in Preferences

https://github.com/mozilla/activity-stream/commit/9cb000b149563238975c7495f9d15b4bbc3ca54a
Merge branch 'central' - Backport Bug 1429940, 1499110

Comment 65

[Daniel Bodea [:danibodea]]

This bug has the "qe-verify+" tag but does not have an easily understandable step to reproduce in order to verify the fix. 
Can you tell me how I should verify it? Thanks.

Comment 66

[:Paolo Amadini]

This bug and bug 1507806 improve the accessibility of headings in the Preferences pages. Previously they were regular text, while now they are proper HTML headings, that are interpreted as such by screen readers. I used the Accessibility Inspector to test the page structure.

Jamie, would you be able to confirm that these pages are now behaving as expected with screen readers?

Comment 67

[James Teh [:Jamie]]

STR (with the NVDA screen reader):
1. Open Firefox Options.
2. NVDA will switch to focus mode, so press escape to switch back to browse mode.
3. Repeatedly press the h key.
Expected: On each press of h, NVDA should speak the heading of a section on the page. Major sections (e.g. General, Language and Appearance) should report as "heading level 1". Sub-sections (e.g. Startup, Fonts & Colors) should report as "heading level 2".

I can confirm that this works as expected.

Comment 68

[Daniel Bodea [:danibodea]]

I have verified this issue in Windows. This feature works nicely in Nightly v65.0a1 from 2018-12-06 with NVDA screen reader, however, it does not work on Mac OS 10.14 with Voice Over screen reader or on Ubuntu 16 with its own screen reader.

Is it intended to be a Windows only feature?

Comment 69

[James Teh [:Jamie]]

(In reply to Bodea Daniel [:danibodea] from comment #68)
>it does not work
> on Mac OS 10.14 with Voice Over screen reader or on Ubuntu 16 with its own
> screen reader.

The instructions I provided in comment 67 are specific to the NVDA screen reader. Other screen readers have a different user interface and thus the STR are different. I don't have access to other platforms at the moment, so I can't provide exact STR. Also, our Mac a11y support is somewhat stagnant at the moment. I think it's enough to verify on Windows (as you have done) and verify with accessibility inspector (as was done in comment 66). We can deal with any remaining issues for other platforms in follow up bugs as necessary. That said, thanks for your commitment to verifying accessibility on multiple platforms.

Comment 70

[Daniel Bodea [:danibodea]]

I can also confirm that I can scroll through the headings of the Preferences page on Mac OS x 10.13.6 on Nightly v65.0a1 from 2018-12-9 using the VoiceOver feature with the keyboard shortcut VO+COMMAND+H for the next and VO+COMMAND+SHIFT+H for the previous header.

I could not find a shortcut to select (and read with Screen Reader) any headers on the Ubuntu 16 test machine. I believe this option does not exist in Ubuntu with its default Screen Reader.

I will mark this bug as verified, but if any of you knows of a way to confirm this fix in Ubuntu/Linux, please provide some information about it. Thank you!