The new tracking protection settings have a few problems: 1. The groupbox and label do not associate properly due to the caption for the groupbox being nested several levels deep in the XUL rather than being a direct child. Decided to solve this by associating the same entity for the label inside the caption to an aria-label on the groupbox. 2. The inner group box should be a group box, not a simple hbox with a label and several controls. However, due to styling, decided to go for an ARIA role rather than a full change of elements in first approach. 3. The link to change the Do Not Track settings is not tabable.

Hi Gijs and Alex, while working on this, I found several problems with the way the xul:groupbox, xul:caption and xul:label elements interact. In the old days, the xul:caption element used to take a label attribute which contained the localized string. Nowadays, the caption element itself no longer holds the label text diretly, but instead gets a child label element (or more) to hold the caption for a groupbox. However, this poses a problem with the way XULGroupboxAccessible::NativeName calculates its name. It queries for a child of role label that has a relation "labeled by", and uses that. However, XULLabelAccessible::NativeName only seems to assign a name for the caption if it is assigned by a label attribute, not by looking at the sub tree if that attribute isn't present. As a result, all the newly constructed groupboxes in all the new in-content preferences no longer have proper labels. Since I've seen the caption element suddenly holding more than one label, one of whom may even be a link, I am leaning towards just not bothering with the APIs, but assigning the groupbox elements explicit aria-labels that have the same localization entity as the main label for that groupbox. That way, people are free to style, nest, or whatever, to their liking, they'll just have to remember to assign the same entity to an aria-label attribute for the groupbox element in the future. One case where just using the whole caption may have undesired side effects can be seen in the first groupbox, the "do not track" settings in browser/components/preferences/in-content/privacy.xul. Given the above, is it OK to move forward as I suggested, with explicit label assignments?

(In reply to Marco Zehe (:MarcoZ) from comment #1) > Hi Gijs and Alex, > > while working on this, I found several problems with the way the > xul:groupbox, xul:caption and xul:label elements interact. In the old days, > the xul:caption element used to take a label attribute which contained the > localized string. Nowadays, the caption element itself no longer holds the > label text diretly, but instead gets a child label element (or more) to hold > the caption for a groupbox. Is this just what happened in the preferences or in our uses of groupboxes/captions? As in, does the "old way" really no longer work? > However, this poses a problem with the way XULGroupboxAccessible::NativeName > calculates its name. It queries for a child of role label that has a > relation "labeled by", and uses that. > > However, XULLabelAccessible::NativeName only seems to assign a name for the > caption if it is assigned by a label attribute, not by looking at the sub > tree if that attribute isn't present. As a result, all the newly constructed > groupboxes in all the new in-content preferences no longer have proper > labels. This is confusing. The in-content preferences are almost identical to the non-in-content ones. Were those also broken? > Since I've seen the caption element suddenly holding more than one label, > one of whom may even be a link, I am leaning towards just not bothering with > the APIs, but assigning the groupbox elements explicit aria-labels that have > the same localization entity as the main label for that groupbox. That way, > people are free to style, nest, or whatever, to their liking, they'll just > have to remember to assign the same entity to an aria-label attribute for > the groupbox element in the future. One case where just using the whole > caption may have undesired side effects can be seen in the first groupbox, > the "do not track" settings in > browser/components/preferences/in-content/privacy.xul. > > Given the above, is it OK to move forward as I suggested, with explicit > label assignments? TBH, I would prefer the APIs to be fixed, but I don't know how easy/hard that would be. I'm also a little confused - in terms of the non-API fix, couldn't we stick an id on the desired label and use aria-labelledby instead of aria-label and duplicating the (sometimes highly finnicky set of) label(s) ?

(In reply to :Gijs Kruitbosch from comment #2) > Is this just what happened in the preferences or in our uses of > groupboxes/captions? As in, does the "old way" really no longer work? I actually didn't try going all the way towards a label attribute on the caption. I was under the impression the labels were there for a reason, like styling, for example. Because normally, the caption element already has a label element as anonymous content in the XBL binding, at least that's what our code in the a11y APIs still assumes. > This is confusing. The in-content preferences are almost identical to the > non-in-content ones. Were those also broken? Back in 2008 or so, I made a pass over the preferences to get all the group box changes speaking automatically, but I never returned to those over the years. And since Preferences isn't something you normally visit every day, unless prompted by something new, it slipped. > TBH, I would prefer the APIs to be fixed, but I don't know how easy/hard > that would be. That's why Alex is also included in the needinfo. ;) I don't know the answer to that question. > I'm also a little confused - in terms of the non-API fix, couldn't we stick > an id on the desired label and use aria-labelledby instead of aria-label and > duplicating the (sometimes highly finnicky set of) label(s) ? Yes we could do that, too.

(In reply to Marco Zehe (:MarcoZ) from comment #1) > However, XULLabelAccessible::NativeName only seems to assign a name for the > caption if it is assigned by a label attribute, not by looking at the sub > tree if that attribute isn't present that seems plausible, and if it's so then the issue should be fixed by making XULLabelAccessible::NativeName to call Accessible::NativeName as a fallback.

Morphing this into an Accessibility APIs bug because this has a generic solution applying to all settings and other places with xul:caption elements that have children instead of a label attribute.

Tested manually that it works, but since my whole Mercurial setup seems to be borked and I can't even push to try, and local mochitests aren't working, don't know if the tests will actually work as intended. Submitting for review anyway since in theory, it should.

Comment on attachment 8750829 [details] [diff] [review] Patch Review of attachment 8750829 [details] [diff] [review]: ----------------------------------------------------------------- this should be working, would it useful to run it on try (you could try to clone a fresh repo if nothing else works) ::: accessible/tests/mochitest/name/test_general.xul @@ +176,5 @@ > + ////////////////////////////////////////////////////////////////////////// > + // groupbox labeling from caption label or its sub tree > + testName("groupbox", "Some caption"); > + testName("groupbox2", "Some caption"); > + nit: whitespaces @@ +347,5 @@ > </hbox> > + > + <!-- Name from label or sub tree --> > + <groupbox id="groupbox"> > + <caption label="Some caption /> nit: missing "

https://hg.mozilla.org/integration/mozilla-inbound/rev/d48d45885ebf03111814590c2f9ed9f3530c43a3 Bug 1248415 - Improve screen reader accessibility of various Preference panes groupbox labeling, r=surkov