Closed Bug 980510 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: eeejay, Assigned: maxli)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached 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)
Attached 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)
Attached 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: 6 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)
Sure.
Flags: needinfo?(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
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.