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)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: eeejay, Assigned: eeejay)
References
Details
Attachments
(1 file, 1 obsolete file)
10.08 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8707086 -
Flags: review?(tbsaunde+mozbugs)
Comment 2•8 years ago
|
||
Is there a reason you didn't override ActioCount / ActionName / DoAction in HTMLLabelAccessible? that seems a little better / faster.
Comment 3•8 years ago
|
||
(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. :-)
Comment 4•8 years ago
|
||
(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.
Assignee | ||
Comment 5•8 years ago
|
||
(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..
Comment 6•8 years ago
|
||
(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.
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8707950 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8707086 -
Attachment is obsolete: true
Attachment #8707086 -
Flags: review?(tbsaunde+mozbugs)
Comment 8•8 years ago
|
||
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+
Assignee | ||
Comment 9•8 years ago
|
||
(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.
Assignee | ||
Comment 10•8 years ago
|
||
(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.
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/152e692692f5
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•