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

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Marco Zehe (:MarcoZ)]

Found after bug 490287. The label we mark up as "presentation" still creates an accessible when it shouldn't.

Comment 1

[alexander :surkov (:asurkov)]

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.

Comment 2

[alexander :surkov (:asurkov)]

Could we get your opinion guys? It sounds this bug is important for a11y.

Comment 3

[Neil Deakin]

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.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: 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.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 385626 [details] [diff] [review]
wip

breaks accesskeys on labels

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 8

[Marco Zehe (:MarcoZ)]

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 9

[Neil Deakin]

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.

Comment 10

[alexander :surkov (:asurkov)]

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.

Comment 11

[Neil Deakin]

Comment on attachment 385684 [details] [diff] [review]
patch

Actually, the label should reimplement the disabled property, and the test shouldn't be changed.

Comment 12

[alexander :surkov (:asurkov)]

(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)?

Comment 13

[Neil Deakin]

(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.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 15

[Neil Deakin]

> 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>

Comment 16

[alexander :surkov (:asurkov)]

(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).

Comment 17

[Neil Deakin]

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.

Comment 18

[alexander :surkov (:asurkov)]

landed on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/1c6fdf03e463

Comment 19

[Boris Zbarsky [:bzbarsky]]

The focus changes here should probably be documented.

Comment 20

[Eric Shepherd [:sheppy]]

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?

Comment 21

[alexander :surkov (:asurkov)]

(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.

Comment 22

[Eric Shepherd [:sheppy]]

Added a note about this here:

https://developer.mozilla.org/en/XUL/label