Closed Bug 1239051 Opened 8 years ago Closed 8 years ago

Label element accessibles should expose labeled controllers action

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(1 file, 1 obsolete file)

When a user clicks on a label with an associated control, the control receives the click and is activated. This behavior should be reflected in the a11y API as well.
Attachment #8707086 - Flags: review?(tbsaunde+mozbugs)
Is there a reason you didn't override ActioCount / ActionName / DoAction in HTMLLabelAccessible? that seems a little better / faster.
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Is there a reason you didn't override ActioCount / ActionName / DoAction in
> HTMLLabelAccessible? that seems a little better / faster.

Although the initial bug was filed against Firefox for Android, I believe we do want this action exposed on XUL labels, too, for desktop. Otherwise I'd agree, overriding the methods in HTMLLabelAccessible would seem cleaner than all these changes to NSAccessible and NSLinkableAccessible. :-)
(In reply to Marco Zehe (:MarcoZ) from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > Is there a reason you didn't override ActioCount / ActionName / DoAction in
> > HTMLLabelAccessible? that seems a little better / faster.
> 
> Although the initial bug was filed against Firefox for Android, I believe we
> do want this action exposed on XUL labels, too, for desktop. Otherwise I'd

Does the same thing work on xul:label? I'm not off hand sure it does, and if it does I could believe it already works or something.

Anyway the patch isn't going to work for xul:label as is so that's not really an argument for doing it this way.  If you want to support xul:label it probably needs some custom handling so you can just add overrides in XULLabelAccessible for that.
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Is there a reason you didn't override ActioCount / ActionName / DoAction in
> HTMLLabelAccessible? that seems a little better / faster.

I can do that. I somehow missed the existence of HTMLLabelAccessible.

If we want the text leaf children of labels to have associated children, we will need to modify LinkableAccessible as well. Are you OK with that?

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> Does the same thing work on xul:label? I'm not off hand sure it does, and if
> it does I could believe it already works or something.
> 

It does already work. No fix needed for XUL label. I'm not entirely sure why..
(In reply to Eitan Isaacson [:eeejay] from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > Is there a reason you didn't override ActioCount / ActionName / DoAction in
> > HTMLLabelAccessible? that seems a little better / faster.
> 
> I can do that. I somehow missed the existence of HTMLLabelAccessible.
> 
> If we want the text leaf children of labels to have associated children, we
> will need to modify LinkableAccessible as well. Are you OK with that?

sure, I found myself wondering if it couldn't be merged with one of the existing flags, but I guess right now those flags are set up to say what sort of thing isthere not what actions are available.  Maybe worth looking at more, but no reason to do it here.

> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > Does the same thing work on xul:label? I'm not off hand sure it does, and if
> > it does I could believe it already works or something.
> > 
> 
> It does already work. No fix needed for XUL label. I'm not entirely sure
> why..

my guess would be the xbl binding already uses onclick listeners or something we already look for.
Attachment #8707950 - Flags: review?(tbsaunde+mozbugs)
Attachment #8707086 - Attachment is obsolete: true
Attachment #8707086 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8707950 [details] [diff] [review]
Labels should expose labeled controllers action.

> bool
>+nsCoreUtils::IsLabelWithControl(nsIContent *aContent)

I wish this was part of HTMLLabelAccessible, but I see we don't support downcasting to that at the moment so this is fine.

nit, * goes with type.

>+{
>+  dom::HTMLLabelElement* label = dom::HTMLLabelElement::FromContent(aContent);
>+  if (label && label->GetControl())
>+    return eClickAction;

true would be clearer

>+uint8_t HTMLLabelAccessible::ActionCount()

return type on its own line
Attachment #8707950 - Flags: review?(tbsaunde+mozbugs) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> >+{
> >+  dom::HTMLLabelElement* label = dom::HTMLLabelElement::FromContent(aContent);
> >+  if (label && label->GetControl())
> >+    return eClickAction;
> 
> true would be clearer
> 

Oops! That should have been true.
(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> (In reply to Eitan Isaacson [:eeejay] from comment #5)
> > If we want the text leaf children of labels to have associated children, we
> > will need to modify LinkableAccessible as well. Are you OK with that?
> 
> sure, I found myself wondering if it couldn't be merged with one of the
> existing flags, but I guess right now those flags are set up to say what
> sort of thing isthere not what actions are available.  Maybe worth looking
> at more, but no reason to do it here.

I wonder why we don't simply walk up the ancestry and proxy the ActionAccessible of the first ancestor with an action instead of duplicating a lot of the Accessible logic in LinkAccessible. I guess there might be cases when we don't want that? In any case, that is for another bug.
https://hg.mozilla.org/mozilla-central/rev/152e692692f5
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: