1212588 - [Accessibility] Keyboard delete should have a role key instead of button.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

All keyboard/keypads/pinpads etc should have delete control updated from being a button to being a key (role). This means that delete action would now happen on tapup instead of double tap.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

We need to update these keyboards:

apps/callscreen
apps/communications/dialer
apps/emergency-call
apps/keyboard
apps/system/lockscreen

Comment 2

[Shivam]

Hey i would like to work on this bug ! can u please help me get started?

Comment 3

[Santiago Saavedra]

I would like to work on this bug. I have updated already most of the keyboards, but I would like input on the apps/keyboard app.

Concisely, I'd like to know whether you are talking about the "backspace key", and if so, maybe other keys should also be rendered as role="key", not just delete (maybe Shift or Alt). These are all currently buttons.

Comment 4

[Santiago Saavedra]

I have what I have fixed at https://github.com/ssaavedra/gaia/tree/bugfix-1212588-keyboard-delete-role-key

Comment 5

[Alberto Pastor [:albertopq]]

Santiago, create a pull request to master with the title 'Bug 1212588 - ... ', and that will automatically attach a patch here. Thanks for working on this!

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Santiago Saavedra from comment #4)
> I have what I have fixed at
> https://github.com/ssaavedra/gaia/tree/bugfix-1212588-keyboard-delete-role-
> key

Hi Santiago, thanks for taking this, what Alberto mentioned about creating Pull Request is right. I'll then be able to give you inline comments straight in Github.

Regarding your question about other buttons within a keyboard, I think it's unnecessary at this point. The reason is that role key allows screen reader users to activate the button without double tapping (simply focusing on the button will activate it). This is something users on other platform are used to so we'll avoid it for now.

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] ssaavedra:bugfix-1212588-keyboard-delete-role-key > mozilla-b2g:master

Comment 8

[Santiago Saavedra]

(In reply to Yura Zenevich [:yzen] from comment #6)
> Hi Santiago, thanks for taking this, what Alberto mentioned about creating
> Pull Request is right. I'll then be able to give you inline comments
> straight in Github.

Alright, that has just been done.
 
> Regarding your question about other buttons within a keyboard, I think it's
> unnecessary at this point. The reason is that role key allows screen reader
> users to activate the button without double tapping (simply focusing on the
> button will activate it). This is something users on other platform are used
> to so we'll avoid it for now.

I am talking specifically about keys such as ALT and CAPS_LOCK.

Speaking about other platforms, I don't know about iOS, but Talkback in Android both with AOSP Keyboard and Swype, assumes every key is a "key" and acts on keyup (including for Enter/Search, spacebar, ALT and shift buttons). I find that to be much less frustrating to use.

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Santiago Saavedra from comment #8)
> (In reply to Yura Zenevich [:yzen] from comment #6)
> > Hi Santiago, thanks for taking this, what Alberto mentioned about creating
> > Pull Request is right. I'll then be able to give you inline comments
> > straight in Github.
> 
> Alright, that has just been done.
>  
> > Regarding your question about other buttons within a keyboard, I think it's
> > unnecessary at this point. The reason is that role key allows screen reader
> > users to activate the button without double tapping (simply focusing on the
> > button will activate it). This is something users on other platform are used
> > to so we'll avoid it for now.
> 
> I am talking specifically about keys such as ALT and CAPS_LOCK.
> 
> Speaking about other platforms, I don't know about iOS, but Talkback in
> Android both with AOSP Keyboard and Swype, assumes every key is a "key" and
> acts on keyup (including for Enter/Search, spacebar, ALT and shift buttons).
> I find that to be much less frustrating to use.

Good point, yes, on iOS double tap is required for all keyboard buttons. We are trying out something in between at the moment. Delete was an initial control suggested to be converted into key as it gets as much use as character keys. We might be consider adding the ones you mentioned later on , once we have some feedback.

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

ok Santiago, talked a bit more with a screen reader user, lets add role key for delete + caps + alt.

Now regarding the logic in key_view.js , we do not want to loose styling that special keys have so we need to do both: add styling and role key to them. We might have to check if target keyCode is one of delete, caps or alt and if so set the attr in the if (target.isSpecialKey) {...} case. I'm pretty sure semantically those keys are special (non-chacaracter) keys so we should still account for them inside the isSpecialKey logic.

If you have some suggestions about making it more awesome, feel free to share.

Comment 11

[Santiago Saavedra]

(In reply to Yura Zenevich [:yzen] from comment #10)
> ok Santiago, talked a bit more with a screen reader user, lets add role key
> for delete + caps + alt.

Agreed.

> Now regarding the logic in key_view.js , we do not want to loose styling
> that special keys have so we need to do both: add styling and role key to
> them. We might have to check if target keyCode is one of delete, caps or alt
> and if so set the attr in the if (target.isSpecialKey) {...} case. I'm
> pretty sure semantically those keys are special (non-chacaracter) keys so we
> should still account for them inside the isSpecialKey logic.
> 
> If you have some suggestions about making it more awesome, feel free to
> share.

I found the screen reader to be somewhat unreliable (on the Flame, sometimes the audio flinches when changing from letter to letter), but that does not seem to be related to this issue. I might open a bug about that, but I'm clueless on where to start with it.

Regarding semantics, I agree with you that they should probably be kept as special keys.

I added the logic where you said, and therefore, I also had to add KeyEvents as a global to key_view.js to keep the linter happy and be able to test for it.

However, I don't seem totally comfortable with that logic inside key_view.js.

Why? I can't tell any specifics currently in the codebase, but I imagine that if the Japanese keyboard had a different layout and had a key to switch from hiragana to katakana (different to Alt, which would still be used for symbols): then that key should also be handled as a non-button special key.

I think it can be more rational if we get the check the other way around: test which keys must be buttons (and probably argue why). At the end of the day, it's a keyboard rendered on a screen. Semantically, all there is are keys, no buttons. The enter key on my laptop does not behave that much different than any of my character keys.

This is of course just my opinion. I would love to know your thoughts about this. If you like the current situation, it's already on the pull request. If you incline with the alternative, it's in an alternate branch also on my repo (not squashed commit, so that differences are easier to see).

Change is at:

https://github.com/ssaavedra/gaia/tree/bugfix-1212588-keyboard-delete-role-key-alternative

Specifically, https://github.com/ssaavedra/gaia/commit/f5e34ffdcbddd46ffa5348cc0edf9b3ef1c49df2


BTW, I haven't been introduced to proper netiquette in this matter, so I ask you: is it ok that I needinfo? you each time I answer?

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Santiago Saavedra from comment #11)
> (In reply to Yura Zenevich [:yzen] from comment #10)
> > ok Santiago, talked a bit more with a screen reader user, lets add role key
> > for delete + caps + alt.
> 
> Agreed.
> 
> > Now regarding the logic in key_view.js , we do not want to loose styling
> > that special keys have so we need to do both: add styling and role key to
> > them. We might have to check if target keyCode is one of delete, caps or alt
> > and if so set the attr in the if (target.isSpecialKey) {...} case. I'm
> > pretty sure semantically those keys are special (non-chacaracter) keys so we
> > should still account for them inside the isSpecialKey logic.
> > 
> > If you have some suggestions about making it more awesome, feel free to
> > share.
> 
> I found the screen reader to be somewhat unreliable (on the Flame, sometimes
> the audio flinches when changing from letter to letter), but that does not
> seem to be related to this issue. I might open a bug about that, but I'm
> clueless on where to start with it.

Yeah we have a bug in audio channel management, it's pretty bad and affects other things such as tone sounds in the Dialer app 

> 
> Regarding semantics, I agree with you that they should probably be kept as
> special keys.
> 
> I added the logic where you said, and therefore, I also had to add KeyEvents
> as a global to key_view.js to keep the linter happy and be able to test for
> it.
> 
> However, I don't seem totally comfortable with that logic inside key_view.js.
> 
> Why? I can't tell any specifics currently in the codebase, but I imagine
> that if the Japanese keyboard had a different layout and had a key to switch
> from hiragana to katakana (different to Alt, which would still be used for
> symbols): then that key should also be handled as a non-button special key.
> 
> I think it can be more rational if we get the check the other way around:
> test which keys must be buttons (and probably argue why). At the end of the
> day, it's a keyboard rendered on a screen. Semantically, all there is are
> keys, no buttons. The enter key on my laptop does not behave that much
> different than any of my character keys.

Totally agree, we will definitely have less cases where something is not a key, so if you are willing to tackle that here, go for it.

> 
> This is of course just my opinion. I would love to know your thoughts about
> this. If you like the current situation, it's already on the pull request.
> If you incline with the alternative, it's in an alternate branch also on my
> repo (not squashed commit, so that differences are easier to see).
> 
> Change is at:
> 
> https://github.com/ssaavedra/gaia/tree/bugfix-1212588-keyboard-delete-role-
> key-alternative
> 
> Specifically,
> https://github.com/ssaavedra/gaia/commit/
> f5e34ffdcbddd46ffa5348cc0edf9b3ef1c49df2
> 
> 
> BTW, I haven't been introduced to proper netiquette in this matter, so I ask
> you: is it ok that I needinfo? you each time I answer?

Yes, for sure, otherwise I will likely miss it. This way it shows up as a request on my dashboard (https://bugzilla.mozilla.org/page.cgi?id=mydashboard.html).

Comment 13

[Santiago Saavedra]

I have updated it as a new approach that I think is better, let me know. I isolated the logic into the layout_normalizer, and added a key.isButton property, as there exists a key.isSpecial already to handle this case in a more general case. This way we have the KeyEvents enumerated all in one file (both for special keys and for button-special keys).

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Santiago Saavedra from comment #13)
> I have updated it as a new approach that I think is better, let me know. I
> isolated the logic into the layout_normalizer, and added a key.isButton
> property, as there exists a key.isSpecial already to handle this case in a
> more general case. This way we have the KeyEvents enumerated all in one file
> (both for special keys and for button-special keys).

Hi Santiago, the PR looks good, left one nit in there and also notice there's a failing unit test. You also need to add a unit test for the JS part that you added.

Comment 15

[Santiago Saavedra]

That should now be it then.

Comment 16

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8690848 [details] [review]
[gaia] ssaavedra:bugfix-1212588-keyboard-delete-role-key > mozilla-b2g:master

Tests look nice, thanks! Forwarding the review to Jan, who's the module peer.

Comment 17

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8690848 [details] [review]
[gaia] ssaavedra:bugfix-1212588-keyboard-delete-role-key > mozilla-b2g:master

Flipping the review to Tim

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8690848 [details] [review]
[gaia] ssaavedra:bugfix-1212588-keyboard-delete-role-key > mozilla-b2g:master

Looks good! Sorry for the delay.

Comment 19

[Santiago Saavedra]

Sorry for my lack of understanding, but what remains here before accepting the patch? Is there anything else I should do? (rebase?, anything?)

Comment 20

[Yura Zenevich [:yzen] (please use "needinfo")]

Hi Santiago, this is pretty much it. 

There's one Gij test faling with PR (not related to your changes) but we need to be green to merge. Could you close/open pull request (via Github UI), this will re-trigger our CI hook and run the tests again? 

To request the merge (since you have the r+) simply add a 'checkin-needed' in the keywords field on this bug.

Comment 21

[Yura Zenevich [:yzen] (please use "needinfo")]

Merged https://github.com/mozilla-b2g/gaia/commit/67ff5b3ec59a3e3730b82010568a319ca283392d

Thanks Santiago!

Comment 22

[Yura Zenevich [:yzen] (please use "needinfo")]

Also let me know if you are interested in any bugs, for example bug 1097827

Comment 23

[Santiago Saavedra]

That bug is already assigned.., is it ok anyway? But yes, I am interested in those.

Comment 24

[Yura Zenevich [:yzen] (please use "needinfo")]

Should not be a problem, the assignee was not very active in the last 10m. I'll assign it to you if that's ok?