Do not create accessibles for XUL label or description having a role of 'presentation'

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: MarcoZ, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access, dev-doc-complete})

Trunk
access, dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

5.30 KB, patch
Neil Deakin (not available until Aug 9)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
Found after bug 490287. The label we mark up as "presentation" still creates an accessible when it shouldn't.
(Assignee)

Updated

8 years ago
Blocks: 492516
(Assignee)

Comment 1

8 years ago
We have common rules for presentation accessible creation, it doesn't depend on it's XUL, HTML. The problem is xul:label is focusable, i.e. nsIContent::IsFocusable returns true since xul:lable implements nsIDOMXULControlElement (http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULElement.cpp#568). I think it's wrong. Cc'ing Olli and Neil to get options.
(Assignee)

Comment 2

8 years ago
Could we get your opinion guys? It sounds this bug is important for a11y.
Labels shouldn't implement nsIDOMXULControlElement, nor should they be in the tab order. However, accesskeys can be associated with them and clicking them should focus the corresponding control. Perhaps label.focus() should as well as some code might be relying on it, although I wouldn't mind if this wasn't done. This latter part is the only case that would be trickier to handle if labels were not focusable.
Right, we always expose focusable nodes regardless of role=presentation.

Like Neil, I say no to tab order. I think the label shouldn't be focusable on its own unless it is possible to perform some action specific to the label. The only other case I think it is OK to focus a label is when it is focused along with it's control (IIRC this happens on IE with checkboxes); but I prefer just the control getting focus.

The benefit of allowing clicking on the label to focus the corresponding control is useful for people with targeting issues. I've seen UI where clicking on the label also toggles the control (in the case of toggles); but maybe consistency should rule here.

Moving over to XUL, let's make sure we agree there, and fix this at the source.
Component: Disability Access APIs → XUL
QA Contact: accessibility-apis → xptoolkit.widgets
(Assignee)

Updated

8 years ago
Summary: Do not create accessibles for XUL or XBL content that has a role of 'presentation' → Do not create accessibles for XUL label or description having a role of 'presentation'
(Assignee)

Comment 5

8 years ago
Created attachment 385626 [details] [diff] [review]
wip

xulLabel.focus() makes nothing, probably because of focus handling reorg (http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#950 hasn't special processing for XUL labels). If it's a bug then I think it can be fixed in GetRedirectedFocus() (http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#265) I think. I can file bug for this. Note HTML labels has own implementation of focus() method and therefore htmlLabel.focus() works.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
(Assignee)

Comment 6

8 years ago
Comment on attachment 385626 [details] [diff] [review]
wip

breaks accesskeys on labels
Attachment #385626 - Attachment is obsolete: true
(Assignee)

Comment 7

8 years ago
Created attachment 385684 [details] [diff] [review]
patch
Attachment #385684 - Flags: review?(marco.zehe)
Attachment #385684 - Flags: review?(enndeakin)
(Reporter)

Updated

8 years ago
Attachment #385684 - Flags: review?(marco.zehe) → review+
(Reporter)

Comment 8

8 years ago
Comment on attachment 385684 [details] [diff] [review]
patch

r=me for the tests and the functional aspect. This does what we want, and NVDA now sees no "unknown object" anymore.
Comment on attachment 385684 [details] [diff] [review]
patch

Let's try this then and see if something breaks.

I'm assuming the change to privacypane_tests.js isn't meant to be part of this patch.
Attachment #385684 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 10

8 years ago
Comment on attachment 385684 [details] [diff] [review]
patch

(In reply to comment #9)
> (From update of attachment 385684 [details] [diff] [review])
> Let's try this then and see if something breaks.
> 
> I'm assuming the change to privacypane_tests.js isn't meant to be part of this
> patch.

No, it is. This mochitest fails with my patch. The problem is they test xul:label as XUL control and I removed it from tests. Let's ask additional review from Ehsan since he is author of this mochitest.
Attachment #385684 - Flags: review?(ehsan.akhgari)
(Assignee)

Updated

8 years ago
Attachment #385684 - Flags: superreview?(Olli.Pettay)

Updated

8 years ago
Attachment #385684 - Flags: superreview?(Olli.Pettay) → superreview+
Comment on attachment 385684 [details] [diff] [review]
patch

Actually, the label should reimplement the disabled property, and the test shouldn't be changed.
Attachment #385684 - Flags: review?(ehsan.akhgari)
Attachment #385684 - Flags: review-
Attachment #385684 - Flags: review+
(Assignee)

Comment 12

8 years ago
(In reply to comment #11)
> (From update of attachment 385684 [details] [diff] [review])
> Actually, the label should reimplement the disabled property, and the test
> shouldn't be changed.

1. Should this property be a pure XBL property or should it go from IDL interface?
2. Why do we need to have disabled property on the label at all? It makes nothing on the XUL label.
3. Should I reimplement accesskey property on the label?
4. What others properties should I reintroduce on the label (#basecontrol: disabled, tabIndex, #basetext: label, crop, image, command, accesskey)?
(In reply to comment #12)
> 1. Should this property be a pure XBL property or should it go from IDL
> interface?

Just the property is OK.

> 2. Why do we need to have disabled property on the label at all? It makes
> nothing on the XUL label.

Because the label can appear disabled. Also, the click handler checks the disabled property.

> 3. Should I reimplement accesskey property on the label?

The accessKey property is already implemented on the label binding. A disabled implementation should just go alongside it.

> 4. What others properties should I reintroduce on the label (#basecontrol:
> disabled, tabIndex, #basetext: label, crop, image, command, accesskey)?

None of the others.
(Assignee)

Comment 14

8 years ago
Created attachment 387362 [details] [diff] [review]
patch2
Attachment #385684 - Attachment is obsolete: true
Attachment #387362 - Flags: review?(enndeakin)
> The accessKey property is already implemented on the label binding. A disabled
> implementation should just go alongside it.

I meant very specifically to put the disabled property on the label implementation ("#text-label"), not the base binding which is also used for <description>
(Assignee)

Comment 16

8 years ago
(In reply to comment #15)

> I meant very specifically to put the disabled property on the label
> implementation ("#text-label"), not the base binding which is also used for
> <description>

But disabled property is defined in nsIDOMXULDescriptionElement interface (http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/xul/nsIDOMXULDescriptionElement.idl#45).
Attachment #387362 - Flags: review?(enndeakin) → review+
Comment on attachment 387362 [details] [diff] [review]
patch2

OK, leave as is then. The description interface shouldn't really have a disabled property, but that's a separate issue.
(Assignee)

Comment 18

8 years ago
landed on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/1c6fdf03e463
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 503829

Updated

8 years ago
Depends on: 513186
The focus changes here should probably be documented.
Keywords: dev-doc-needed
Looking at the discussion and code, it looks like what needs documenting here is that labels can't be focused? Or am I missing a nuance here?
(Assignee)

Comment 21

7 years ago
(In reply to comment #20)
> Looking at the discussion and code, it looks like what needs documenting here
> is that labels can't be focused? Or am I missing a nuance here?

labels don't implement nsIDOMXULControlElement interface and as consequence are not focusable.
Added a note about this here:

https://developer.mozilla.org/en/XUL/label
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.