886074 - [AccessFu] Braille should update when editing

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Max Li [:maxli]]

The braille text should respond to text change events.

Comment 1

[Max Li [:maxli]]

Attachment: Patch

This patch reveals a bug with respect to the caret navigation and editing, namely sometimes the caret moves visually but typing does not move it. I will investigate further.

Comment 2

[Marco Zehe (:MarcoZ)]

(In reply to Max Li [:maxli] from comment #1)
> This patch reveals a bug with respect to the caret navigation and editing,
> namely sometimes the caret moves visually but typing does not move it. I
> will investigate further.

Could this be similar to the fact that, when typing, TalkBack speaks all of the previously typed text as "deleted", and then sometimes also speaks all previously typed text again? As in: Every typed character causes all text to be removed and re-added to the widget we use for HTML input and textarea elements? I've never seen this behavior in other native Android widgets, not even our address bar. Could this be related?

Comment 3

[Eitan Isaacson [:eeejay]]

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

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

I think we got a lot of overhead here!

How about we refactor all the braille states into its own object that would be a member of Output. Maybe with a method called update(), that takes the new text and returns a brailleText structure like the one you put in Android events.

The object and its methods should be generic enough that we could use it outside of Android when the time comes.

Comment 4

[Max Li [:maxli]]

Attachment: Patch v2

Comment 5

[Eitan Isaacson [:eeejay]]

Comment on attachment 772133 [details] [diff] [review]
Patch v2

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

::: accessible/src/jsat/AccessFu.jsm
@@ +505,4 @@
>        if (androidEvent.brailleText && 'output' in androidEvent.brailleText) {
> +        this.brailleState.startOffset = androidEvent.brailleText.startOffset;
> +        this.brailleState.endOffset = androidEvent.brailleText.endOffset;
> +        this.brailleState.text = androidEvent.brailleText.output;

Why isn't all this encapsulated in this.brailleState? It could be done in update().

Comment 6

[Max Li [:maxli]]

(In reply to Eitan Isaacson [:eeejay] from comment #5)

> Why isn't all this encapsulated in this.brailleState? It could be done in
> update().

update() (at least as the patch is written currently) only gets called on text change events.

Comment 7

[Max Li [:maxli]]

Attachment: Patch v3

Comment 8

[Eitan Isaacson [:eeejay]]

Comment on attachment 772348 [details] [diff] [review]
Patch v3

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

Looks good!

Comment 9

[Max Li [:maxli]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff0f3b303c66

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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