Last Comment Bug 687393 - HTML select options gets relation from containing label
: HTML select options gets relation from containing label
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: rela11y
  Show dependency treegraph
 
Reported: 2011-09-18 19:23 PDT by alexander :surkov
Modified: 2011-10-01 18:16 PDT (History)
2 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.65 KB, patch)
2011-09-19 01:25 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (17.48 KB, patch)
2011-09-19 02:30 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-09-18 19:23:37 PDT
Example:

<label>Hello
<select>
  <option>option</option>
</select>

Example in web: http://harkin.senate.gov/contact_opinion.cfm
Comment 1 alexander :surkov 2011-09-19 01:25:24 PDT
Created attachment 560861 [details] [diff] [review]
patch

based on bug 673958
Comment 2 alexander :surkov 2011-09-19 02:30:21 PDT
Created attachment 560864 [details] [diff] [review]
patch2
Comment 3 Trevor Saunders (:tbsaunde) 2011-09-28 19:47:45 PDT
Comment on attachment 560864 [details] [diff] [review]
patch2


>+  // document.
>+  nsAccessible* walkUp = mAcc->Parent();
>+  while (walkUp->IsElement()) {
>+    nsIContent* walkUpElm = walkUp->GetContent();

If we really care about pef we should move the nsIContent* local var out of the loop so we can call  the GetContent() vfunc once per loop instead of twice.  (we call it twice here once explicitly and once in IsElement()).
Comment 4 alexander :surkov 2011-09-28 19:56:27 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Comment on attachment 560864 [details] [diff] [review] [diff] [details] [review]
> patch2
> 
> 
> >+  // document.
> >+  nsAccessible* walkUp = mAcc->Parent();
> >+  while (walkUp->IsElement()) {
> >+    nsIContent* walkUpElm = walkUp->GetContent();
> 
> If we really care about pef we should move the nsIContent* local var out of
> the loop so we can call  the GetContent() vfunc once per loop instead of
> twice.  (we call it twice here once explicitly and once in IsElement()).

I'll replace walkUp->IsElement() on !walkUp->IsDoc() that should be more pref and more clear what is it for.
Comment 5 alexander :surkov 2011-09-28 20:14:44 PDT
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/da2b4e883854
Comment 6 Michael Wu [:mwu] 2011-09-29 01:37:56 PDT
https://hg.mozilla.org/mozilla-central/rev/da2b4e883854
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2011-09-29 03:32:05 PDT
Comment on attachment 560864 [details] [diff] [review]
patch2

>--- a/accessible/src/base/AccIterator.cpp
>+++ b/accessible/src/base/AccIterator.cpp
> HTMLLabelIterator::Next()
> {
>   // Get either <label for="[id]"> element which explicitly points to given
>   // element, or <label> ancestor which implicitly point to it.
>   nsAccessible* label = nsnull;
>   while ((label = mRelIter.Next())) {
>     if (label->GetContent()->Tag() == nsGkAtoms::label)
>+    if (walkUpElm->Tag() == nsGkAtoms::label) {
>+    if (walkUpElm->Tag() == nsGkAtoms::form)

Is it safe to assume that these are in the HTML namespace? If so, I'd appreciate assertions.
Comment 8 alexander :surkov 2011-09-29 04:02:34 PDT
(In reply to Ms2ger from comment #7)

> Is it safe to assume that these are in the HTML namespace? If so, I'd
> appreciate assertions.

It's sort of assumption it never happens in real life. It makes sense to keep HTML namespace for correctness.

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