Last Comment Bug 687414 - label_for relation is missed on label implicitly associated with control
: label_for relation is missed on label implicitly associated with control
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla27
Assigned To: Eitan Isaacson [:eeejay]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: rela11y
  Show dependency treegraph
 
Reported: 2011-09-19 01:14 PDT by alexander :surkov
Modified: 2013-09-20 03:04 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add relation_for for implicit labels. (4.03 KB, patch)
2013-09-17 15:53 PDT, Eitan Isaacson [:eeejay]
tbsaunde+mozbugs: review-
Details | Diff | Splinter Review
Add relation_for for implicit labels. (5.69 KB, patch)
2013-09-18 13:49 PDT, Eitan Isaacson [:eeejay]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description User image alexander :surkov 2011-09-19 01:14:21 PDT
example:
<label>hello<input></label>
Comment 1 User image Eitan Isaacson [:eeejay] 2013-09-17 15:53:09 PDT
Created attachment 806313 [details] [diff] [review]
Add relation_for for implicit labels.

Since we use HTMLLabelAccessible now for all HTML <label> tags, I figured this should be an override in that class.
Comment 2 User image Trevor Saunders (:tbsaunde) 2013-09-17 16:12:33 PDT
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
Comment 3 User image alexander :surkov 2013-09-18 05:56:54 PDT
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 User image Trevor Saunders (:tbsaunde) 2013-09-18 08:35:53 PDT
> ::: 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.
Comment 5 User image alexander :surkov 2013-09-18 08:55:30 PDT
(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 User image Trevor Saunders (:tbsaunde) 2013-09-18 10:29:46 PDT
> 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
Comment 7 User image alexander :surkov 2013-09-18 11:15:05 PDT
Comment on attachment 806313 [details] [diff] [review]
Add relation_for for implicit labels.

delegating it to Trev
Comment 8 User image Trevor Saunders (:tbsaunde) 2013-09-18 11:37:19 PDT
Comment on attachment 806313 [details] [diff] [review]
Add relation_for for implicit labels.

I'd like to see patch after comment 2 is fixed
Comment 9 User image Eitan Isaacson [:eeejay] 2013-09-18 13:49:57 PDT
Created attachment 806881 [details] [diff] [review]
Add relation_for for implicit labels.

Much cleaner without all those null checks.
Comment 10 User image Trevor Saunders (:tbsaunde) 2013-09-18 14:59:28 PDT
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
Comment 12 User image Ed Morley [:emorley] 2013-09-20 03:04:32 PDT
https://hg.mozilla.org/mozilla-central/rev/caf1b9e056a7

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