1233118 - implement IAccessible2_3::selectionRanges

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

See some questions in Comment 2

Comment 4

[alexander :surkov (:asurkov)]

(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

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

Alex, would you mind updating the patch so I go through it once more (the revised version)? Thanks!

Comment 6

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8190ef3200f0

Comment 7

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f14bf18b42e

Comment 8

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ede1d8691e6

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch

it seems there's no way to test the loop

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Comment 11

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=153f30a7957a

Comment 12

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c34410da6b0130bc5430d18a20ad0357114ab0ed
Bug 1233118 - implement IAccessible2_3::selectionRanges, r=yzen

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c34410da6b01