Closed Bug 1233118 Opened 4 years ago Closed 4 years ago

implement IAccessible2_3::selectionRanges

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Attachment #8704710 - Flags: review?(yzenevich)
Comment on attachment 8704710 [details] [diff] [review]
patch

Review of attachment 8704710 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/generic/Accessible.cpp
@@ +2261,5 @@
> +  }
> +
> +  const Accessible* child = this;
> +  Accessible* parent = nullptr;
> +  while (child && !child->IsHyperText()) {

This seems to only run once?

@@ +2262,5 @@
> +
> +  const Accessible* child = this;
> +  Accessible* parent = nullptr;
> +  while (child && !child->IsHyperText()) {
> +    child = parent;

If child is hypertext, wouldn't both parent and child be set to nullptr?

::: accessible/windows/ia2/ia2Accessible.h
@@ +103,5 @@
>      /* [out, size_is(,*nTargets)] */ IUnknown*** targets,
>      /* [out, retval] */ long* nTargets
>    );
>  
> +  // IAccessibel2_3

typo: IAccessible2_3
See some questions in Comment 2
Flags: needinfo?(surkov.alexander)
(In reply to Yura Zenevich [:yzen] from comment #2)
> Comment on attachment 8704710 [details] [diff] [review]
> patch
> 
> Review of attachment 8704710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/Accessible.cpp
> @@ +2261,5 @@
> > +  }
> > +
> > +  const Accessible* child = this;
> > +  Accessible* parent = nullptr;
> > +  while (child && !child->IsHyperText()) {
> 
> This seems to only run once?

oh, right, I need to have a test where direct parent is not a hypertext :)

> @@ +2262,5 @@
> > +
> > +  const Accessible* child = this;
> > +  Accessible* parent = nullptr;
> > +  while (child && !child->IsHyperText()) {
> > +    child = parent;
> 
> If child is hypertext, wouldn't both parent and child be set to nullptr?

this case is handled by if statement above

> > +  // IAccessibel2_3
> 
> typo: IAccessible2_3

k
Flags: needinfo?(surkov.alexander)
Alex, would you mind updating the patch so I go through it once more (the revised version)? Thanks!
Flags: needinfo?(surkov.alexander)
Attached patch patchSplinter Review
it seems there's no way to test the loop
Attachment #8704710 - Attachment is obsolete: true
Attachment #8704710 - Flags: review?(yzenevich)
Flags: needinfo?(surkov.alexander)
Attachment #8709412 - Flags: review?(yzenevich)
Comment on attachment 8709412 [details] [diff] [review]
patch

Review of attachment 8709412 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good, hopefully I didn't miss anything. Thanks

::: accessible/generic/Accessible.cpp
@@ +2264,5 @@
> +  const Accessible* parent = this;
> +  do {
> +    child = parent;
> +    parent = parent->Parent();
> +  } while (parent && !parent->IsHyperText());

Yeah this loop looks good now, thanks.

::: accessible/tests/mochitest/textrange/test_selection.html
@@ +73,5 @@
> +      var a11yranges = getAccessible(document, [nsIAccessibleText]).selectionRanges;
> +      var a11yrange = a11yranges.queryElementAt(0, nsIAccessibleTextRange);
> +
> +      testTextRange(a11yrange, "selection range #5", p, 0, p, 4);
> +      ok(!a11yrange.crop(getAccessible(a)), "Crop #5 was succeeded while it shouldn't");

Nit: Crop #5 succeeded

@@ +83,5 @@
> +      var a11yrange = a11yranges.queryElementAt(0, nsIAccessibleTextRange);
> +
> +      testTextRange(a11yrange, "selection range #6", p, 6, p, 10);
> +
> +      ok(!a11yrange.crop(getAccessible(a)), "Crop #6 was succeeded while it shouldn't");

Nit: Crop #5 succeeded
Attachment #8709412 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/mozilla-central/rev/c34410da6b01
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.