[AccessFu] Should be able to move caret with braille

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: maxli, Assigned: maxli)

Tracking

Trunk
mozilla25
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Hitting the routing key should move the caret
(Assignee)

Comment 1

5 years ago
Created attachment 768956 [details] [diff] [review]
WIP

I'm still not too sure if I like how this is structured. Pretty much anything related to storing the offsets seems kind of ugly to me.
Attachment #768956 - Flags: feedback?(eitan)
Comment on attachment 768956 [details] [diff] [review]
WIP

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

I actually don't think this is too bad. There is potential for a race condition if the display refreshes as the user presses a key, but I don't think that would happen a lot. It looks correct in the sense that the offset is stored at/near the braille output entry point, so the state should remain pretty consistent (ie. good choice about not storing this in content scripts).

::: accessible/src/jsat/AccessFu.jsm
@@ +465,5 @@
>        androidEvent.type = 'Accessibility:Event';
>        if (androidEvent.bounds)
>          androidEvent.bounds = this._adjustBounds(androidEvent.bounds, aBrowser);
> +      if (androidEvent.brailleText && androidEvent.brailleText.braille) {
> +        AccessFu.Input.startOffset = androidEvent.brailleText.startOffset;

I think it would be perty if startOffset/endOffset were actually members of Output. And maybe brailleStartOffset/brailleEndOffset.

@@ +693,3 @@
>      let mm = Utils.getMessageManager(Utils.CurrentBrowser);
> +    if (this.editState.editing) {
> +      mm.sendAsyncMessage('AccessFu:MoveCaret', {offset: aData.keyIndex - this.startOffset});

I think the offset may be NaN if the user simply double taps on their screen, no? 

Maybe always provide an offset in the message, -1 if none was provided. Don't check editState, let the contents-script.js check if an input element is focused. 

The editState is a necessary evil for the arrow navigation. Although, I could see us finding a better solution for that in the near future.

::: accessible/src/jsat/OutputGenerator.jsm
@@ +471,5 @@
> +    let outputOrder = typeof gUtteranceOrder.value == 'number' ?
> +                      gUtteranceOrder.value : this.defaultOutputOrder;
> +
> +    let acc = aContext.accessible;
> +    if (acc instanceof Ci.nsIAccessibleText) {

That is the first time I have seen this used without an explicit QueryInterface, but it works!
Attachment #768956 - Flags: feedback?(eitan) → feedback+
(Assignee)

Comment 3

5 years ago
Created attachment 770345 [details] [diff] [review]
Patch
Attachment #768956 - Attachment is obsolete: true
Attachment #770345 - Flags: review?(eitan)
Comment on attachment 770345 [details] [diff] [review]
Patch

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

::: accessible/src/jsat/OutputGenerator.jsm
@@ +561,5 @@
>  
>    defaultOutputOrder: OUTPUT_DESC_LAST,
>  
> +  genForContext: function genForContext(aContext) {
> +    let output = {output: OutputGenerator.genForContext.apply(this, arguments)};

The output type should be the same as in the overridden function. If this is necessary, the base method needs to change too.

An alternative would be to have an optional info out argument that could be populated with startOffset/endOffset.

::: accessible/src/jsat/content-script.js
@@ +187,5 @@
>    let accText = accessible.QueryInterface(Ci.nsIAccessibleText);
>    let oldOffset = accText.caretOffset;
>    let text = accText.getText(0, accText.characterCount);
>  
> +  if ('offset' in aMessage.json) {

Giant conditional blocks like this get out of hand fast.

I think this whole |offset| block should be another function, maybe moveCaretTo, and maybe in the scope of activateCurrent, just like we have activateAccessible there.

::: accessible/tests/mochitest/jsat/output.js
@@ +23,5 @@
>    var oldAccessible = getAccessible(aOldAccOrElmOrID);
>    var context = new PivotContext(accessible, oldAccessible);
>    var output = aGenerator.genForContext(context);
> +  if (output.output) {
> +    output = output.output;

The base and utterance generator still returns an array, no?
Attachment #770345 - Flags: review?(eitan)
(Assignee)

Comment 5

5 years ago
Created attachment 770506 [details] [diff] [review]
Patch v2
Attachment #770345 - Attachment is obsolete: true
Attachment #770506 - Flags: review?(eitan)
(Assignee)

Comment 6

5 years ago
Created attachment 770830 [details] [diff] [review]
Patch v2.1

Rebased against m-c to take into account bug 752609.
Attachment #770506 - Attachment is obsolete: true
Attachment #770506 - Flags: review?(eitan)
Attachment #770830 - Flags: review?(eitan)
Comment on attachment 770830 [details] [diff] [review]
Patch v2.1

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

Looking good.

::: accessible/src/jsat/content-script.js
@@ +165,5 @@
> +      accText.caretOffset = aOffset;
> +    }
> +    let newOffset = accText.caretOffset;
> +
> +    presentCaretChange(text, oldOffset, newOffset);

nit, Just accText.caretOffset instead of newOffset.

@@ +239,5 @@
>    }
>  
>    let newOffset = accText.caretOffset;
> +
> +  presentCaretChange(text, oldOffset, newOffset);

same, just accText.caretOffset.
Attachment #770830 - Flags: review?(eitan) → review+
(Assignee)

Updated

5 years ago
Attachment #770830 - Flags: review?(bugmail.mozilla)
Comment on attachment 770830 [details] [diff] [review]
Patch v2.1

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

::: mobile/android/base/GeckoAccessibility.java
@@ +43,5 @@
>      // Used to store the JSON message and populate the event later in the code path.
>      private static JSONObject sEventMessage = null;
>      private static AccessibilityNodeInfo sVirtualCursorNode = null;
>  
> +    private static final int BRAILLE_CLICK_BASE_INDEX = -275000000;

Please add a comment as to where this number is coming from.
Attachment #770830 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Updated

5 years ago
Depends on: 886074
https://hg.mozilla.org/mozilla-central/rev/2ff734019b09
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.