886076 - [AccessFu] Support movement by granularity

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Max Li [:maxli]]

We should support movement by granularity when outside of editing mode.

Comment 1

[Max Li [:maxli]]

Attachment: WIP

This gets most of the way there. The paragraph granularity is pretty half-hearted. The character and word movements seem to work fine, minus the fact that once you reach the end it just stops instead of moving the pivot (I assume we want that?).

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 770168 [details] [diff] [review]
WIP

>+      if (aDetails.granularity === 8) {

Can you replace this with a constant defining what "8" actually means?

>+      oldOffset = startOffset; // XXX: this won't work if we move pivots and we're going backwards

What are the consequences, or what are the situations where this condition in the comment is met? Do you still plan to handle this, or is this scoped for a different bug?

Comment 3

[Max Li [:maxli]]

(In reply to Marco Zehe (:MarcoZ) from comment #2)
> What are the consequences, or what are the situations where this condition
> in the comment is met? Do you still plan to handle this, or is this scoped
> for a different bug?

This is in reference to the last bit of what I wrote when I posted the patch. In that if you're at the end of a word (for example), and you swipe further right, one might expect (though correct me if I'm wrong) that the pivot moves to the next element and reads the next word. (Though I can see arguments on both sides of whether or not this should be the behaviour.) (If we're going to do that, the amount of change required may be sufficient to handle that in another bug.)

Comment 4

[Marco Zehe (:MarcoZ)]

I think we're OK for this one. If at the last word of an element in native Android, and swiping right, it also doesn't move to the next element. iOS doesn't do that, either.

Comment 5

[Eitan Isaacson [:eeejay]]

Comment on attachment 770168 [details] [diff] [review]
WIP

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

I'm surprised that changing the caretOffset in non-editable content gives any output. I should test that. In theory, this is what the startOffset and endOffset in the nsIAccessiblePivot is for. They have not been implemented, but moveNextByText and movePreviousByText were meant to be exactly for this. So this may be an opportunity to implement them.

Check out: http://mxr.mozilla.org/mozilla-central/source/accessible/public/nsIAccessiblePivot.idl

::: accessible/src/jsat/content-script.js
@@ +193,5 @@
> +
> +  if (typeof childIndex === 'number') {
> +    Logger.info('Calculating the oldOffset; the current child has index ' + childIndex);
> +    for (let child = accessible.firstChild; child.indexInParent < childIndex; child = child.nextSibling) {
> +      startOffset += child instanceof Ci.nsIAccessibleText ? child.characterCount : child.name.length;

I'm pretty sure you could do this with nsIAccessibleHyperLink.startIndex more easily.

@@ +195,5 @@
> +    Logger.info('Calculating the oldOffset; the current child has index ' + childIndex);
> +    for (let child = accessible.firstChild; child.indexInParent < childIndex; child = child.nextSibling) {
> +      startOffset += child instanceof Ci.nsIAccessibleText ? child.characterCount : child.name.length;
> +    }
> +    endOffset = startOffset + text.length;

.. and nsIAccessibleHyperLink.endIndex.

Comment 6

[Max Li [:maxli]]

Attachment: WIP v2

Comment 7

[Eitan Isaacson [:eeejay]]

Comment on attachment 778487 [details] [diff] [review]
WIP v2

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

I have been testing it, and it works nicely, for the most part. There are some hiccups, like the bounds grab the whole paragraph when at the last word before a line-wrap. I think this has to do with the fact that you move forward solely with word-end boundary, so we get everything leading up to that word, in this case, a new line. When I attempted to do this in the past, I got the new end offset the same way you do, and then use the end offset with GetTextBeforeOffset() and a boundary of word-start to get the start offset. Then we get a start and end offset of the word alone and no trailing or leading white space.

I also see occasional errors when navigating back by word and crossing line boundaries, these need to be looked into. I could try to set up a test case if you can't reproduce that. Here is the stack trace, in any case:

[AccessFu] ERROR Error handing accessible event:
 Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIAccessibleText.getRangeExtents]
  getTextBounds@resource://gre/modules/accessibility/Utils.jsm:234
  VisualPresenter_textSelectionChanged@resource://gre/modules/accessibility/Presentation.jsm:180
  textSelectionChanged@resource://gre/modules/accessibility/Presentation.jsm:542
  handleAccEvent@resource://gre/modules/accessibility/EventManager.jsm:163
  observe@resource://gre/modules/accessibility/EventManager.jsm:442
  moveByGranularity@chrome://global/content/accessibility/content-script.js:249
  moveCaret@chrome://global/content/accessibility/content-script.js:261


This is not a caret move, or a selection move. So besides a similar entry-point in Android, there should be no relation between the two. In other words, it is not moveCaret, and the resulting event is not a text selection change.

A text pivot change is still a pivot change. What I originally envisioned, was that the visual pivotChanged method would also check the text offsets of the vc, if it exists it would send a second rectangle (or collection) with the bounds of the current text. The Output.Visual method would then draw another (differently styled) rectangle(s) around the text.
The biggest change I would like to see in a future patch would be a better separation of editing text movements and pivot text movements.

::: accessible/src/jsat/EventManager.jsm
@@ +161,3 @@
>  
> +          this.present(Presentation.textSelectionChanged(position, accText.getText(0,-1),
> +                     pivot.endOffset, pivot.endOffset, pivot.startOffset, pivot.startOffset, true));

This is a pivot change, so I would use the pivot change presenter function. Then you don't have to check if it is an entry later on either.

::: accessible/src/jsat/Presentation.jsm
@@ +168,5 @@
> +  textSelectionChanged: function VisualPresenter_textSelectionChanged(aAccessible, aText,
> +                                                      aStart, aEnd,
> +                                                      aOldStart, aOldEnd,
> +                                                      aIsFromUser) {
> +      if (!aAccessible || aAccessible.role === Ci.nsIAccessibleRole.ROLE_ENTRY) {

We use two column indentation.

@@ +169,5 @@
> +                                                      aStart, aEnd,
> +                                                      aOldStart, aOldEnd,
> +                                                      aIsFromUser) {
> +      if (!aAccessible || aAccessible.role === Ci.nsIAccessibleRole.ROLE_ENTRY) {
> +        return;

Return a value if it is a function that is supposed to return values.

::: accessible/src/jsat/TraversalRules.jsm
@@ +255,5 @@
> +     ROLE_SECTION],
> +    function Paragraph_match(aAccessible) {
> +      for (let child = aAccessible.firstChild; child; child = child.nextSibling) {
> +        if (child.role === ROLE_TEXT_LEAF) {
> +          return FILTER_MATCH;

I think you want to ignore the subtree as well. If you don't you will keep on navigating into nested sections.

::: accessible/src/jsat/Utils.jsm
@@ +226,5 @@
>        aAccessible.getBounds(objX, objY, objW, objH);
>        return new Rect(objX.value, objY.value, objW.value, objH.value);
>    },
>  
> +  getTextBounds: function getTextBounds(aAccessible, aStart, aEnd) {

What we really want is a union of 1 or more bounds, but the a11y API does not provide that right now, oh well.

Comment 8

[Max Li [:maxli]]

Attachment: Patch

> I also see occasional errors when navigating back by word and crossing line
> boundaries

I think I've fixed all instances of this.

> The Output.Visual method would then draw
> another (differently styled) rectangle(s) around the text.

The larger rectangle sounds like it would be confusing as the pivot position moves around.

Comment 9

[Eitan Isaacson [:eeejay]]

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

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

Glad to see tests here.
VisualPresenter.viewportChanged() needs to be updated so that text offsets are cached, and the text bounds is used. Besides that I don't see any other reason why this shouldn't land now. So r=me once you fix the viewPortChanged.

::: accessible/src/jsat/Presentation.jsm
@@ +152,5 @@
>          Ci.nsIAccessibleScrollType.SCROLL_TYPE_ANYWHERE);
> +
> +      let bounds = (aContext.startOffset === -1 && aContext.endOffset === -1) ?
> +                   aContext.bounds : Utils.getTextBounds(aContext.accessible,
> +                                     aContext.startOffset, aContext.endOffset);

The pivot context could potentially hold the bounds of the text when text offsets are given.

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to Max Li [:maxli] from comment #8)
> > The Output.Visual method would then draw
> > another (differently styled) rectangle(s) around the text.
> 
> The larger rectangle sounds like it would be confusing as the pivot position
> moves around.

Maybe. I'll play around with it.

Comment 11

[Eitan Isaacson [:eeejay]]

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

Flagging Alex for an extra review on the core bits.

Comment 12

[alexander :surkov (:asurkov)]

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

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

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +304,5 @@
>    NS_ENSURE_ARG(aResult);
>  
>    *aResult = false;
>  
> +  int32_t oldStart = mStartOffset, oldEnd = mEndOffset, newStart, newEnd;

in general it's a good practice to initialize variables

@@ +305,5 @@
>  
>    *aResult = false;
>  
> +  int32_t oldStart = mStartOffset, oldEnd = mEndOffset, newStart, newEnd;
> +  nsAutoString s;

's' is not a good name, if this variable value is not in use then name it unusedString or unusedText and please define it where you use it.

@@ +309,5 @@
> +  nsAutoString s;
> +  bool usingParent = false;
> +  HyperTextAccessible* accText = mPosition->AsHyperText();
> +
> +  if (!accText) {

nit: no new line pls
nit: please name it as hyperText or simply text (we tend to omit 'acc' from variable names of accessible objects)

@@ +315,5 @@
> +    usingParent = true;
> +  }
> +
> +  if (mEndOffset == accText->CharacterCount())
> +    return NS_OK;

so you don't cross position boundaries. Say if you have
<p>paragraph</p><p>second paragraph</p>
1) moveNext to 1st p
2) moveTextNext it
3) moveNext to 2nd p
4) moveTextNext it

instead of simple moveTextNext

wouldn't it be more comfortable if you were able to traverse the text in whole?

also I don't how you go into subtree. Say you have
<p>hello <a>link</a></p>

then if you do moveTextNext then you wont' go into link text automatically. Is this logic outside tree traversal?

@@ +337,5 @@
> +
> +  accText->GetTextAtOffset(mEndOffset, endBoundary, &newStart, &newEnd, s);
> +  mEndOffset = newEnd;
> +
> +  accText->GetTextBeforeOffset(mEndOffset, startBoundary, &newStart, &newEnd, s);

in case of word boundary I think you need to use GetTextAtOffset

Comment 13

[Max Li [:maxli]]

(In reply to alexander :surkov from comment #12)
> so you don't cross position boundaries. 
> 
> wouldn't it be more comfortable if you were able to traverse the text in
> whole?
> 
> also I don't how you go into subtree. Say you have
> <p>hello <a>link</a></p>
> 
> then if you do moveTextNext then you wont' go into link text automatically.

I'm think these should be left to followups. FWIW from comment #4 the current behaviour is typical on iOS and Android.


> > +
> > +  accText->GetTextBeforeOffset(mEndOffset, startBoundary, &newStart, &newEnd, s);
> 
> in case of word boundary I think you need to use GetTextAtOffset

That doesn't seem to work (and the current way does).

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Max Li [:maxli] from comment #13)
> (In reply to alexander :surkov from comment #12)
> > so you don't cross position boundaries. 
> > 
> > wouldn't it be more comfortable if you were able to traverse the text in
> > whole?
> > 
> > also I don't how you go into subtree. Say you have
> > <p>hello <a>link</a></p>
> > 
> > then if you do moveTextNext then you wont' go into link text automatically.
> 
> I'm think these should be left to followups. 

I'm fine as long as you don't put the logic on accessFu level

> > > +  accText->GetTextBeforeOffset(mEndOffset, startBoundary, &newStart, &newEnd, s);
> > 
> > in case of word boundary I think you need to use GetTextAtOffset
> 
> That doesn't seem to work (and the current way does).

ok, btw, why do you want to extract word and ignore punctuation?

Comment 15

[Max Li [:maxli]]

(In reply to alexander :surkov from comment #14)
> ok, btw, why do you want to extract word and ignore punctuation?

Extra spaces (more specifically newlines) were causing undesirable behaviours when grabbing the bounds of the word. (Also it does maintain punctuation (at least the common ones I tried it with).)

Comment 16

[Max Li [:maxli]]

Attachment: Updated Pivot Patch

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 780559 [details] [diff] [review]
Updated Pivot Patch

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

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +308,5 @@
> +  int32_t oldStart = mStartOffset, oldEnd = mEndOffset, newStart = 0, newEnd = 0;
> +  bool usingParent = false;
> +  HyperTextAccessible* text = mPosition->AsHyperText();
> +  if (!text) {
> +    text = mPosition->Parent()->AsHyperText();

you may crash here, parent is not necessary a hypertext. as long as you can set it into any position it's valid scenario

@@ +309,5 @@
> +  bool usingParent = false;
> +  HyperTextAccessible* text = mPosition->AsHyperText();
> +  if (!text) {
> +    text = mPosition->Parent()->AsHyperText();
> +    usingParent = true;

it seems like text != mPosition should works instead this variable

@@ +312,5 @@
> +    text = mPosition->Parent()->AsHyperText();
> +    usingParent = true;
> +  }
> +
> +  if (mEndOffset == text->CharacterCount())

if you went to parent, mEndOffset is relative mPostion then is it correct to compare them?

@@ +316,5 @@
> +  if (mEndOffset == text->CharacterCount())
> +    return NS_OK;
> +
> +  if (mEndOffset == -1)
> +    mEndOffset = usingParent ? text->GetChildOffset(mPosition) : 0;

wouldn't it be good to change mPosition here since you change mEndOffset here?

Comment 18

[Max Li [:maxli]]

Attachment: Updated Pivot Patch v2

> wouldn't it be good to change mPosition here since you change mEndOffset
> here?

I don't think mPosition should change until we're sure there will be a pivot change (so I could move it up a bit, but I don't think all the way up there).

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Max Li [:maxli] from comment #18)
> Created attachment 780611 [details] [diff] [review]
> Updated Pivot Patch v2
> 
> > wouldn't it be good to change mPosition here since you change mEndOffset
> > here?
> 
> I don't think mPosition should change until we're sure there will be a pivot
> change (so I could move it up a bit, but I don't think all the way up there).

Is it ok if mEndOffset and mPosition are out of sync?

Comment 20

[Max Li [:maxli]]

Attachment: Pivot patch v3

> Is it ok if mEndOffset and mPosition are out of sync?

After discussion, I think this makes sense. So I've changed it.

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 780959 [details] [diff] [review]
Pivot patch v3

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

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +310,5 @@
> +  while (!text) {
> +    text = mPosition->Parent()->AsHyperText();
> +    if (!text)
> +      mPosition = mPosition->Parent();
> +  }

perhaps
while (true) {
  text = mPosition->Parent()->AsHyperText();
  if (text)
    break;

  mPostion = mPosition->Parent();
}

to avoid !text twice

@@ +316,5 @@
> +  if (mEndOffset == -1)
> +    mEndOffset = text != mPosition ? text->GetChildOffset(mPosition) : 0;
> +
> +  if (text != mPosition)
> +    mPosition = mPosition->Parent();

you changed it above already, right?

@@ +337,5 @@
> +  }
> +
> +  nsAutoString unusedText;
> +  text->GetTextAtOffset(mEndOffset, endBoundary, &newStart, &newEnd, unusedText);
> +  mEndOffset = newEnd;

can't you pass mEndOffset instead newEnd?

@@ +341,5 @@
> +  mEndOffset = newEnd;
> +
> +  text->GetTextBeforeOffset(mEndOffset, startBoundary, &newStart, &newEnd,
> +                            unusedText);
> +  mStartOffset = newEnd == mEndOffset ? newStart : newEnd;

I still think this shouldn't work. For example in case of word end offset. Say you have: "hello my friend". Offset is 9, right after 'my' word. getTextAtOffset returns ' friend', getTextBeforeOffset returns 'hello '. You should select 'my friend' instead presumably 'friend'. Actually I'm surprised you don't use GetTextAfter to move word next what seems logical here.

If this double call is just avoiding extra spaces (not punctuation) then it's easier perhaps to trim spaces. However I'm not sure why excess space would affect on announcements.

Comment 22

[Max Li [:maxli]]

Attachment: Pivot patch v4

> perhaps
> while (true) {
>   text = mPosition->Parent()->AsHyperText();
>   if (text)
>     break;
> 
>   mPostion = mPosition->Parent();
> }
> 
> to avoid !text twice

That's not exactly equivalent. It changes the behaviour if mPosition was already a HyperText. But I tweaked it to avoid two checks per loop.

> you changed it above already, right?

Same reason as above.

> I still think this shouldn't work. For example in case of word end offset.
> Say you have: "hello my friend". Offset is 9, right after 'my' word.
> getTextAtOffset returns ' friend', getTextBeforeOffset returns 'hello '. You
> should select 'my friend' instead presumably 'friend'. Actually I'm
> surprised you don't use GetTextAfter to move word next what seems logical
> here.
> 
> If this double call is just avoiding extra spaces (not punctuation) then
> it's easier perhaps to trim spaces. However I'm not sure why excess space
> would affect on announcements.

Assuming you mean offset is 8 (since 9 is the one before friend, not after my, but it doesn't actually change this example anyways). getTextAtOffset returns ' friend', so the new mEndOffset is 16. Using getTextBeforeOffset (using offset 16) then returns 'my ', which taking the end offset of that produces 'friend'. 

Since its the offsets which matter from this method (and not the string itself), it doesn't seem easier to trim. Trimming was necessary to make the bounds of the word correct when the word was at the end of a line.

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 781021 [details] [diff] [review]
Pivot patch v4

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

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +319,5 @@
> +  if (mEndOffset == -1)
> +    mEndOffset = text != mPosition ? text->GetChildOffset(mPosition) : 0;
> +
> +  if (text != mPosition)
> +    mPosition = mPosition->Parent();

it's a bit complicated code to change mPosition. Perhaps you could have variable for oldPosition and then change mPosition when you want? Does it make code clearer?

Comment 24

[Max Li [:maxli]]

Attachment: Pivot patch v4.1

Okay, I think this is a bit clearer.

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 781039 [details] [diff] [review]
Pivot patch v4.1

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

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +304,5 @@
>    NS_ENSURE_ARG(aResult);
>  
>    *aResult = false;
>  
> +  int32_t oldStart = mStartOffset, oldEnd = mEndOffset, newStart = 0, newEnd = 0;

pls define newStart and newEnd variables where you need them

@@ +308,5 @@
> +  int32_t oldStart = mStartOffset, oldEnd = mEndOffset, newStart = 0, newEnd = 0;
> +  HyperTextAccessible* text = mPosition->AsHyperText();
> +  Accessible* oldPosition = mPosition;
> +  while (!text) {
> +    oldPosition = mPosition;

after you assign it it's not old position any more :) it's rather child at offset.

@@ +316,5 @@
> +
> +  if (mEndOffset == -1)
> +    mEndOffset = text != oldPosition ? text->GetChildOffset(oldPosition) : 0;
> +
> +  if (mEndOffset == text->CharacterCount())

so 'text' can be different from mPosition, you compare mEndOffset relative 'text' offsets what means that mEndOffset may be disconnected from mPosition. probably you should do this check before you change mPosition?

Comment 26

[Max Li [:maxli]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/784d2686a389
https://hg.mozilla.org/integration/mozilla-inbound/rev/b34e30147678

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/784d2686a389
https://hg.mozilla.org/mozilla-central/rev/b34e30147678