Closed Bug 1011886 Opened 10 years ago Closed 10 years ago

[AccessFu] Introduce key echo by character, word, and character and word

Categories

(Core :: Disability Access APIs, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: MarcoZ, Assigned: maxli)

Details

Attachments

(1 file, 3 obsolete files)

Right now, when we type something in a text box or text area on Firefox OS, the whole text gets repeated on each letter entered. This is probably due to the introduction of reacting to valueChangedEvent. We should make it smarter and make it only repeat the last character entered, or the last word, or both, and yes that should be a keyboard echo setting. :-)
Attached patch Patch (obsolete) — Splinter Review
Is this along the lines of what you're thinking? (Right now, there's no setting to change which one happens, so for now both character and words are spoken.)
Attachment #8496676 - Flags: feedback?(mzehe)
Assignee: nobody → maxli
Comment on attachment 8496676 [details] [diff] [review]
Patch

Yes, this goes in the direction I like! :) Do we know if the word gets repeated if, for example, first a period, then a space is input? Repeating the word again when the space is pressed would be superflous, but this could be dealt with in a separate followup patch.
Attachment #8496676 - Flags: feedback?(mzehe) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8496676 - Attachment is obsolete: true
Attachment #8503020 - Flags: review?(eitan)
(In reply to Marco Zehe (:MarcoZ) from comment #2)
> Comment on attachment 8496676 [details] [diff] [review]
> Patch
> 
> Yes, this goes in the direction I like! :) Do we know if the word gets
> repeated if, for example, first a period, then a space is input? Repeating
> the word again when the space is pressed would be superflous, but this could
> be dealt with in a separate followup patch.

The word doesn't get repeated if you input two word separators in a row. Though the way the text interface works is that a period (and probably most/all punctuation) isn't considered a word separator, just things like spaces and newlines.
(In reply to Max Li [:maxli] from comment #4)
> The word doesn't get repeated if you input two word separators in a row.
> Though the way the text interface works is that a period (and probably
> most/all punctuation) isn't considered a word separator, just things like
> spaces and newlines.

Cool, that's fine, too! Thank you!
Comment on attachment 8503020 [details] [diff] [review]
Patch v2

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

::: accessible/jsat/Presentation.jsm
@@ +458,5 @@
> +B2GPresenter.prototype.keyboardEchoSetting =
> +  new PrefCache('accessibility.accessfu.keyboard_echo');
> +B2GPresenter.prototype.CHARACTER_ECHO = 0;
> +B2GPresenter.prototype.WORD_ECHO = 1;
> +B2GPresenter.prototype.CHARACTER_AND_WORD_ECHO = 2;

I believe we'll actually want 4 settings: None, Character, Word, Character and Word here. Yes, there are those purists who'll turn off keyboard echo completely and rely on the spoken keys alone when typing.
Attached patch Patch v2 with 4 settings (obsolete) — Splinter Review
Attachment #8503020 - Attachment is obsolete: true
Attachment #8503020 - Flags: review?(eitan)
Attachment #8503281 - Flags: review?(eitan)
Comment on attachment 8503281 [details] [diff] [review]
Patch v2 with 4 settings

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

This looks pretty good! Could you add a test, please? How are you testing this?
Attachment #8503281 - Flags: review?(eitan)
I would add the test in test_content_text.html.
Attached patch Patch with testsSplinter Review
(In reply to Eitan Isaacson [:eeejay] from comment #8)
> Comment on attachment 8503281 [details] [diff] [review]
> Patch v2 with 4 settings
> 
> Review of attachment 8503281 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty good! Could you add a test, please? How are you testing
> this?

I've been testing it using the screen reader simulator addon (don't have an actual device).
Attachment #8503281 - Attachment is obsolete: true
Attachment #8513538 - Flags: review?(eitan)
Comment on attachment 8513538 [details] [diff] [review]
Patch with tests

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

This looks good! As we talked on IRC, we should have a more intentional echo for password fields. Also, is the default b2g setting of character+word good? I think Marco should have a final word on that, code-wise this is great!
Attachment #8513538 - Flags: review?(eitan)
Attachment #8513538 - Flags: review+
Attachment #8513538 - Flags: feedback?(mzehe)
Comment on attachment 8513538 [details] [diff] [review]
Patch with tests

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

::: b2g/app/b2g.js
@@ +798,5 @@
>  // Whether to skip images with empty alt text
>  pref("accessibility.accessfu.skip_empty_images", true);
> +// Setting to change the verbosity of entered text (0 - none, 1 - characters,
> +// 2 - words, 3 - both)
> +pref("accessibility.accessfu.keyboard_echo", 3);

Yes, this is fine for an initial value. We'll obviously have to have a followup bug to introduce changing the setting in the Accessibility settings in Gaia Settings so users can adjust this to their liking.

Thanks also for these thorough tests, maxLi! :)
Attachment #8513538 - Flags: feedback?(mzehe) → feedback+
https://hg.mozilla.org/mozilla-central/rev/b5a7134e728c
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.