Closed
Bug 687414
Opened 13 years ago
Closed 11 years ago
label_for relation is missed on label implicitly associated with control
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: surkov, Assigned: eeejay)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(1 file, 1 obsolete file)
5.69 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
example:
<label>hello<input></label>
Assignee | ||
Comment 1•11 years ago
|
||
Since we use HTMLLabelAccessible now for all HTML <label> tags, I figured this should be an override in that class.
Attachment #806313 -
Flags: review?(surkov.alexander)
Comment 2•11 years ago
|
||
Comment on attachment 806313 [details] [diff] [review]
Add relation_for for implicit labels.
>+++ b/accessible/src/generic/Accessible.cpp
>@@ -25,16 +25,17 @@
> #include "TableAccessible.h"
> #include "TableCellAccessible.h"
> #include "TreeWalker.h"
>
> #include "nsIDOMElement.h"
> #include "nsIDOMDocument.h"
> #include "nsIDOMNodeFilter.h"
> #include "nsIDOMHTMLElement.h"
>+#include "nsIDOMHTMLLabelElement.h"
why?
>+ if (mContent->Tag() == nsGkAtoms::label && !mContent->IsHTML()) {
>+ rel.AppendIter(new IDRefsIterator(mDoc, mContent, nsGkAtoms::control));
>+ }
no braces
>+++ b/accessible/src/html/HTMLElementAccessibles.cpp
>+using namespace mozilla::dom;
no, just use dom:: where you actually need it
>+HTMLLabelAccessible::RelationByType(uint32_t aType)
>+{
>+ Relation rel = AccessibleWrap::RelationByType(aType);
>+ if (aType == nsIAccessibleRelation::RELATION_LABEL_FOR) {
>+ HTMLLabelElement* label = HTMLLabelElement::FromContent(mContent);
>+ if (label) {
I don't think it can actually be null
>+ nsGenericHTMLElement* control = label->GetControl();
>+ if (control) {
no braces
>+ rel.AppendTarget(mDoc, control->AsContent());
AsContent() shouldn't be needed
>+++ b/accessible/src/html/HTMLElementAccessibles.h
>+ virtual Relation RelationByType(uint32_t aType);
MOZ_OVERRIDE
needs tests too
Reporter | ||
Comment 3•11 years ago
|
||
Comment on attachment 806313 [details] [diff] [review]
Add relation_for for implicit labels.
Review of attachment 806313 [details] [diff] [review]:
-----------------------------------------------------------------
::: accessible/src/generic/Accessible.cpp
@@ +2020,5 @@
>
> case nsIAccessibleRelation::RELATION_LABEL_FOR: {
> Relation rel(new RelatedAccIterator(Document(), mContent,
> nsGkAtoms::aria_labelledby));
> + if (mContent->Tag() == nsGkAtoms::label && !mContent->IsHTML()) {
mContent->IsXUL()
::: accessible/src/html/HTMLElementAccessibles.cpp
@@ +66,5 @@
> }
>
> +Relation
> +HTMLLabelAccessible::RelationByType(uint32_t aType)
> +{
wouldn't it be nicer to get this code under class like HTMLLabelledIterator dual to HTMLLabelIterator, at least we will be keep the logic not spread through the code.
Comment 4•11 years ago
|
||
> ::: accessible/src/html/HTMLElementAccessibles.cpp
> @@ +66,5 @@
> > }
> >
> > +Relation
> > +HTMLLabelAccessible::RelationByType(uint32_t aType)
> > +{
>
> wouldn't it be nicer to get this code under class like HTMLLabelledIterator
> dual to HTMLLabelIterator, at least we will be keep the logic not spread
> through the code.
another iterator that's really silly? bleh
besides we already have code that does random things to get relations I don't see why this case is a big deal.
Reporter | ||
Comment 5•11 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> another iterator that's really silly? bleh
the iterator here is probably too cumbersome but it adds symmetry. What we have with this patch:
labelled_by is implemented by Accessible by HTMLLabelIterator in HTML case and generic iterator in XUL case
label_for is implemented by Accessible by iterator in XUL case and implemented by HTMLLabelAccessible class by HTML case with no iterators
what we could have:
both labelled_by and label_for implemented by iterators by Accessible class both for HTML and XUL
> besides we already have code that does random things to get relations I
> don't see why this case is a big deal.
probably I don't really mind what way will you take but it sounds like proposed way makes the code a bit more complicated.
Comment 6•11 years ago
|
||
> probably I don't really mind what way will you take but it sounds like
> proposed way makes the code a bit more complicated.
I'd say adding a useless iterator makes things more complicated
Reporter | ||
Comment 7•11 years ago
|
||
Comment on attachment 806313 [details] [diff] [review]
Add relation_for for implicit labels.
delegating it to Trev
Attachment #806313 -
Flags: review?(surkov.alexander) → review?(trev.saunders)
Comment 8•11 years ago
|
||
Comment on attachment 806313 [details] [diff] [review]
Add relation_for for implicit labels.
I'd like to see patch after comment 2 is fixed
Attachment #806313 -
Flags: review?(trev.saunders) → review-
Assignee | ||
Comment 9•11 years ago
|
||
Much cleaner without all those null checks.
Attachment #806313 -
Attachment is obsolete: true
Attachment #806881 -
Flags: review?(trev.saunders)
Comment 10•11 years ago
|
||
Comment on attachment 806881 [details] [diff] [review]
Add relation_for for implicit labels.
>+ if (aType == nsIAccessibleRelation::RELATION_LABEL_FOR) {
>+ dom::HTMLLabelElement* label = dom::HTMLLabelElement::FromContent(mContent);
might be nicer to use auto here
Attachment #806881 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 11•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → eitan
Comment 12•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•