Closed
Bug 886075
Opened 11 years ago
Closed 11 years ago
[AccessFu] Should be able to move caret with braille
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: maxli, Assigned: maxli)
References
Details
Attachments
(1 file, 3 obsolete files)
18.34 KB,
patch
|
eeejay
:
review+
kats
:
review+
|
Details | Diff | Splinter Review |
Hitting the routing key should move the caret
Assignee | ||
Comment 1•11 years ago
|
||
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 2•11 years ago
|
||
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•11 years ago
|
||
Attachment #768956 -
Attachment is obsolete: true
Attachment #770345 -
Flags: review?(eitan)
Comment 4•11 years ago
|
||
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•11 years ago
|
||
Attachment #770345 -
Attachment is obsolete: true
Attachment #770506 -
Flags: review?(eitan)
Assignee | ||
Comment 6•11 years ago
|
||
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 7•11 years ago
|
||
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•11 years ago
|
Attachment #770830 -
Flags: review?(bugmail.mozilla)
Comment 8•11 years ago
|
||
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 | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff734019b09
Comment 10•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2ff734019b09
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•