1475078 - When calling movePreviousByText on an unbounded pivot, it should return the last subtext in the position

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Eitan Isaacson [:eeejay]]

Give the following markup:
<p id="p1">Hello world</p> <p id="p2">I've missed you</p>

If the the pivot is unbound (startOffset == endOffset == -1) on "p2", and we call movePreviousByText with word bounds, we get a new position of "p1" with offsets of 6-11 ("world").

While this is not necessarily incorrect, it is not what a user would expect from virtual cursor navigation. They would expect to navigate the text in the current position backwards, meaning they would expect "you" from "p2".

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Go to last substring in current position when movePreviousByText. r?surkov

In the case of an unbound pivot (endOffset == startOffset == -1), we
should move to the last substring in the current position when
movePreviousByText is called.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 8991475 [details] [diff] [review]
Go to last substring in current position when movePreviousByText. r?surkov

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

::: accessible/base/nsAccessiblePivot.cpp
@@ +456,5 @@
>      }
>  
>      // If the search led to the parent of the node we started on (e.g. when
>      // starting on a text leaf), start the text movement from the end of that
>      // node, otherwise we just default to 0.

please update the comment, will be nice to have an example you provided in the bug

@@ +459,5 @@
>      // starting on a text leaf), start the text movement from the end of that
>      // node, otherwise we just default to 0.
>      if (tempStart == -1) {
> +      if (tempPosition != curPosition && text == curPosition->Parent())
> +        tempStart = text->GetChildOffset(curPosition) + nsAccUtils::TextLength(curPosition);

GetChildOffset(curPosition + 1) will also work?

::: accessible/tests/mochitest/pivot/test_virtualcursor_text.html
@@ +98,5 @@
>                    getAccessible(doc.getElementById("s1-link-1"), nsIAccessibleText)));
>        gQueue.push(new setVCTextInvoker(docAcc, "movePreviousByText", WORD_BOUNDARY, [0, 1],
>                    getAccessible(doc.getElementById("section-1"), nsIAccessibleText)));
>  
> +      gQueue.push(new setVCRangeInvoker(docAcc, getAccessible(doc.getElementById("s1-link-1")), [0, 1]));

why is it [0, 1], not [0, 0]?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d55a38a6be53
Go to last substring in current position when movePreviousByText. r=surkov

Comment 4

[Cristina Coroiu [:ccoroiu]]

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

Comment 5

[Chris Peterson [:cpeterson]]

status-firefox62=wontfix because Eitan recommends that we do not uplift this fix to GV 62 beta.