nsAccessible::GetFirstAvailableAccessible should use the same DOM tree traversal as in accessible tree creation

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks 2 bugs, {access})

unspecified
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

spun off bug 638326. I think it should be fixed by bug 634202. But prefer to keep this bug separately just in case. Don't forget to fix todo in text/test_hypertext.html.
Blocks: texta11y
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Attachment #8374787 - Flags: review?(trev.saunders)
Attachment #8374787 - Flags: review?(trev.saunders)
Attachment #8374787 - Attachment is obsolete: true
Posted patch patchSplinter Review
Attachment #8375709 - Flags: review?(trev.saunders)
Comment on attachment 8375709 [details] [diff] [review]
patch

>+      if (childNode == anchorNode)
>+        return NextChildInternal(false);
>+    }
>+    PopState();
>+
>+    anchorNode = parentNode->AsElement();

why do you need the AsElement()

>+  enum {
>+    eWalkCache = 1,
>+    eWalkContextTree = 2 | eWalkCache

Its weird how one flag implies another. also comment what they mean?


>   virtual ~TreeWalker();

btw why is that virtual?

>+    descendant = mDoc->GetAccessible(findNode);
>+    if (!descendant && findNode->IsElement()) {
>+      Accessible* container = mDoc->GetContainerAccessible(findNode);

what's wrong with text nodes?
Attachment #8375709 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> >+    anchorNode = parentNode->AsElement();
> 
> why do you need the AsElement()

to assign it to anchorNode which is a content

> >+  enum {
> >+    eWalkCache = 1,
> >+    eWalkContextTree = 2 | eWalkCache
> 
> Its weird how one flag implies another. also comment what they mean?

not sure how to make it nicer, I don't want to have an option to go outside the given element and create the tree. I will document.

> >   virtual ~TreeWalker();
> 
> btw why is that virtual?

change it to MOZ_FINAL while I'm here?

> >+    descendant = mDoc->GetAccessible(findNode);
> >+    if (!descendant && findNode->IsElement()) {
> >+      Accessible* container = mDoc->GetContainerAccessible(findNode);
> 
> what's wrong with text nodes?

text nodes should be accessible in general, I guess it's technically possible to stop at not accessible text node though. I will change that to nsIContent then.
> > >+  enum {
> > >+    eWalkCache = 1,
> > >+    eWalkContextTree = 2 | eWalkCache
> > 
> > Its weird how one flag implies another. also comment what they mean?
> 
> not sure how to make it nicer, I don't want to have an option to go outside
> the given element and create the tree. I will document.

fair enough

> > >   virtual ~TreeWalker();
> > 
> > btw why is that virtual?
> 
> change it to MOZ_FINAL while I'm here?

it shouldn't have any virtual functions, but I guess you might as well.
https://hg.mozilla.org/mozilla-central/rev/d8462ffa097d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.