Last Comment Bug 494345 - Do not create accessibles for XUL label or description having a role of 'presentation'
: Do not create accessibles for XUL label or description having a role of 'pres...
Status: RESOLVED FIXED
: access, dev-doc-complete
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
:
Mentors:
Depends on: 503829 513186
Blocks: 492516 490287
  Show dependency treegraph
 
Reported: 2009-05-21 22:36 PDT by Marco Zehe (:MarcoZ) on PTO until August 15
Modified: 2010-12-27 09:18 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (863 bytes, patch)
2009-06-28 06:36 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch (5.71 KB, patch)
2009-06-28 20:06 PDT, alexander :surkov
mzehe: review+
enndeakin: review-
bugs: superreview+
Details | Diff | Splinter Review
patch2 (5.30 KB, patch)
2009-07-07 19:53 PDT, alexander :surkov
enndeakin: review+
Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) on PTO until August 15 2009-05-21 22:36:35 PDT
Found after bug 490287. The label we mark up as "presentation" still creates an accessible when it shouldn't.
Comment 1 alexander :surkov 2009-05-22 09:37:59 PDT
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 2009-06-24 01:00:26 PDT
Could we get your opinion guys? It sounds this bug is important for a11y.
Comment 3 Neil Deakin 2009-06-24 05:07:41 PDT
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] 2009-06-24 06:12:43 PDT
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 2009-06-28 06:36:57 PDT
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.
Comment 6 alexander :surkov 2009-06-28 09:07:37 PDT
Comment on attachment 385626 [details] [diff] [review]
wip

breaks accesskeys on labels
Comment 7 alexander :surkov 2009-06-28 20:06:57 PDT
Created attachment 385684 [details] [diff] [review]
patch
Comment 8 Marco Zehe (:MarcoZ) on PTO until August 15 2009-06-29 01:05:52 PDT
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 2009-07-06 10:31:47 PDT
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 2009-07-06 20:51:51 PDT
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 2009-07-07 04:39:39 PDT
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 2009-07-07 19:31:44 PDT
(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 2009-07-07 19:39:45 PDT
(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 2009-07-07 19:53:58 PDT
Created attachment 387362 [details] [diff] [review]
patch2
Comment 15 Neil Deakin 2009-07-07 19:57:21 PDT
> 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 2009-07-07 20:03:17 PDT
(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 2009-07-09 06:53:49 PDT
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 2009-07-10 04:18:39 PDT
landed on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/1c6fdf03e463
Comment 19 Boris Zbarsky [:bz] 2010-01-26 21:12:35 PST
The focus changes here should probably be documented.
Comment 20 Eric Shepherd [:sheppy] 2010-08-19 07:47:24 PDT
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 2010-08-24 19:56:08 PDT
(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] 2010-12-27 09:18:23 PST
Added a note about this here:

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

Note You need to log in before you can comment on or make changes to this bug.