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

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: eeejay, Assigned: maxli)

Tracking

unspecified
mozilla35
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Assignee

Comment 1

5 years ago
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8477889 - Flags: review?(surkov.alexander)
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)
Attachment #8477889 - Flags: review?(surkov.alexander) → review?(eitan)
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.
Attachment #8477889 - Flags: review?(eitan)
Assignee

Comment 4

5 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
Assignee: nobody → maxli
Attachment #8477889 - Attachment is obsolete: true
Attachment #8481085 - Flags: review?(eitan)
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?
Attachment #8481085 - Flags: review?(eitan)
Assignee

Comment 6

5 years ago
Posted patch Patch v3Splinter Review
(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.
Attachment #8481085 - Attachment is obsolete: true
Attachment #8483570 - Flags: review?(eitan)
Comment on attachment 8483570 [details] [diff] [review]
Patch v3

Thanks! I am assuming that these tests pass and are happy.
Attachment #8483570 - Flags: review?(eitan) → review+
https://hg.mozilla.org/mozilla-central/rev/1c04e1da900e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
This blocks some test improvements from being uplifted to Aurora. Any chance we could get an approval request for this as well?
Flags: needinfo?(maxli)
Assignee

Comment 11

5 years ago
Sure.
Flags: needinfo?(maxli)
Assignee

Comment 12

5 years ago
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
Attachment #8483570 - Flags: approval-mozilla-aurora?
Comment on attachment 8483570 [details] [diff] [review]
Patch v3

Aurora+
Attachment #8483570 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
accessible/tests/mochitest/jsat/test_content_text.html
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.