980510 - [AccessFu] After moving by granularity, swiping to next object brings up current object.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

STR:

1. Land on paragraph, hear it read out.
2. Move forward by a few words in paragraph.
3. Try to move to next item.

Result: Paragraph is heard again.
Expected: Next item in page should be read.

Comment 1

[Max Li [:maxli]]

Attachment: Patch

This happens because when you browse by text, you're on the paragraph node, so when you move forward you search into the paragraph's children and land onto the text. So we can avoid this by moving to the child before searching, which this patch does.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8477889 [details] [diff] [review]
Patch

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

I think Eitan can steal this review (Alex is fine with it)

Comment 3

[Eitan Isaacson [:eeejay]]

Comment on attachment 8477889 [details] [diff] [review]
Patch

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

Looks good, aside from a needed null-check.

::: accessible/base/nsAccessiblePivot.cpp
@@ +677,5 @@
>      }
>    }
>  
> +  if (aAccessible == mPosition && mStartOffset != -1 && mEndOffset != -1) {
> +    matched = aAccessible->AsHyperText()->GetChildAtOffset(mStartOffset);

AsHyperText() could return null.

Potentially we could get into a state where mStartOffset/mEndOffset are greater than -1, and mPosition is not hypertext accessible.

Comment 4

[Max Li [:maxli]]

Attachment: Patch v2

Comment 5

[Eitan Isaacson [:eeejay]]

Comment on attachment 8481085 [details] [diff] [review]
Patch v2

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

::: accessible/base/nsAccessiblePivot.cpp
@@ +679,5 @@
>  
> +  if (aAccessible == mPosition && mStartOffset != -1 && mEndOffset != -1) {
> +    HyperTextAccessible* text = aAccessible->AsHyperText();
> +    if (!text) {
> +      matched = text->GetChildAtOffset(mStartOffset);

This doesn't look right, should the conditional be inverted?

Comment 6

[Max Li [:maxli]]

Attachment: Patch v3

(In reply to Eitan Isaacson [:eeejay] from comment #5)
> Comment on attachment 8481085 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8481085 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/base/nsAccessiblePivot.cpp
> @@ +679,5 @@
> >  
> > +  if (aAccessible == mPosition && mStartOffset != -1 && mEndOffset != -1) {
> > +    HyperTextAccessible* text = aAccessible->AsHyperText();
> > +    if (!text) {
> > +      matched = text->GetChildAtOffset(mStartOffset);
> 
> This doesn't look right, should the conditional be inverted?

Hmm, you're right. Not sure what happened there.

Comment 7

[Eitan Isaacson [:eeejay]]

Comment on attachment 8483570 [details] [diff] [review]
Patch v3

Thanks! I am assuming that these tests pass and are happy.

Comment 8

[Max Li [:maxli]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c04e1da900e

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1c04e1da900e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This blocks some test improvements from being uplifted to Aurora. Any chance we could get an approval request for this as well?

Comment 11

[Max Li [:maxli]]

Sure.

Comment 12

[Max Li [:maxli]]

Comment on attachment 8483570 [details] [diff] [review]
Patch v3

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: blocks some test improvements from being uplifted to Aurora
[Describe test coverage new/current, TBPL]: mochitest
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8483570 [details] [diff] [review]
Patch v3

Aurora+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1b49a4b95fd9

Comment 15

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

accessible/tests/mochitest/jsat/test_content_text.html