alexander :surkov (:asurkov) Assignee
	Description • 9 years ago

      No description provided.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 2 • 8 years ago

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

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 3 • 8 years ago

See some questions in Comment 2

	alexander :surkov (:asurkov) Assignee
	Comment 4 • 8 years ago

(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

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 5 • 8 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 6 • 8 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 7 • 8 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 8 • 8 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 9 • 8 years ago

it seems there's no way to test the loop

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 10 • 8 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 11 • 8 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 12 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 13 • 8 years ago
bugherder

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