[Keyboard] Wrong events are generated when longpressing the backspace key

VERIFIED FIXED in Firefox OS v2.0

Status

VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: julienw, Assigned: wdeng)

Tracking

unspecified
2.0 S1 (9may)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(b2g18 wontfix, b2g-v1.2 wontfix, b2g-v1.3 wontfix, b2g-v1.4 affected, b2g-v2.0 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

STR:
* go to http://everlong.org/mozilla/testcase-keyboard/
* enter some text inside the input
* longpress the backspace key

Expected:
* when the text is entered, there are as many keypress, keydown, keyup events as characters
* when backspace is kept pressed, lots of keydown events are generated, but only one keyup, and no keypress
(note: this is the behavior of Firefox Desktop and Google Chrome)

Actual:
* when the text is entered, there are as many keypress, keydown, keyup events as characters
* when backspace is kept pressed, lots of keydown, keyup, keypress events are generated, and in the same count.

Without this we can't accurately know whether a keyboard key is kept pressed (which works only for backspace now, which is fine), and we need this for bug 954712.

Asking 1.4? because this may break the compatibiliy on the web, and I'd even suggest to ask for an approval for 1.3 once a patch is ready.

Qawanted: does that reproduce on 1.3 and 1.2 (need to know if it's a regression)?
(In reply to Julien Wajsberg [:julienw] from comment #0)

> Expected:
> * when the text is entered, there are as many keypress, keydown, keyup
> events as characters
> * when backspace is kept pressed, lots of keydown events are generated, but
> only one keyup, and no keypress
> (note: this is the behavior of Firefox Desktop and Google Chrome)
> 

Sorry, on Firefox Desktop, we get as many keypress events as we have keydown events. I don't know who is right, and I don't really care which behavior you take. The keyup behavior is what matters to me.
Hmm, we don't really have the idea of longpresses in Firefox OS. Every key event that gets fired is a separate call. For every other key than backspace it doesn't insert more than one character either, so that's a special case. Maybe we should have a holdKey method on InputMethod.

Let's ask yxl.
Flags: needinfo?(xyuan)

Comment 3

5 years ago
This does not reproduce on today's Buri 1.3 mozRIL and Buri 1.2 comRIL. Would you like a regression window Julien?
status-b2g-v1.2: --- → unaffected
status-b2g-v1.3: --- → unaffected
Keywords: qawanted → regression

Updated

5 years ago
QA Contact: gbennett

Updated

5 years ago
Keywords: regressionwindow-wanted
(In reply to Julien Wajsberg [:julienw] from comment #1)
> (In reply to Julien Wajsberg [:julienw] from comment #0)
> 
> > Expected:
> > * when the text is entered, there are as many keypress, keydown, keyup
> > events as characters
> > * when backspace is kept pressed, lots of keydown events are generated, but
> > only one keyup, and no keypress
> > (note: this is the behavior of Firefox Desktop and Google Chrome)
> > 
> 
> Sorry, on Firefox Desktop, we get as many keypress events as we have keydown
> events. I don't know who is right, and I don't really care which behavior
> you take. The keyup behavior is what matters to me.

From the spec (http://www.w3.org/TR/DOM-Level-3-Events/#event-type-keypress), "keypress event MUST be dispatched when a key is pressed down, if and only if that key normally produces a character value."
Since backspace doesn't produce a character value, keypress event should not be dispatched.
Flags: needinfo?(xyuan)
(In reply to Jan Jongboom [:janjongboom] from comment #2)
> Hmm, we don't really have the idea of longpresses in Firefox OS. Every key
> event that gets fired is a separate call. For every other key than backspace
> it doesn't insert more than one character either, so that's a special case.
> Maybe we should have a holdKey method on InputMethod.
> 
> Let's ask yxl.

@gbennett, thanks. It's not a regression, but a feature to implement.

@julienw and @janjongboom, currently keyboard API (https://wiki.mozilla.org/WebAPI/KeboardIME) can't synthesize the long pressed key events. We have InputContext.sendKey() to generate a "keydown-keypress-keyup" serial, but cannot generate many keydown events with a single keyup.

Just as janjongboom mentioned, to support long pressed key events, we need to change the keyboard API.
Adding a holdKey method is a way, but seems little complicated. I'd like to allow virtual keyboard to generate a single key event. Then virtual keyboard can do whatever it needed.

To enable keyboard API to send single key event, we can change the existing InputContext.sendKey() method to: 

   InputContext.sendKey(KeyboardEvent[])

or add a new sendKeyEvents method to do that.

cc WebAPI and Keyboard guys to ask for their opinions.
Flags: needinfo?(ehsan)
Keywords: regression, regressionwindow-wanted
Clearing nom as this is a feature, not a regression.
blocking-b2g: 1.4? → ---
Jason, sometimes I dont follow your decisions.

I think this is now clear this is a regression from the new keyboard management. Moreover, and more importantly, this can break the Web.

Therefore, in light of this, I'm requesting 1.4?. We have time for 1.4 so let's do this :)

Thanks to Yuan and Jan!
blocking-b2g: --- → 1.4?
(In reply to Julien Wajsberg [:julienw] from comment #7)
> Jason, sometimes I dont follow your decisions.
> 
> I think this is now clear this is a regression from the new keyboard
> management. Moreover, and more importantly, this can break the Web.
> 
> Therefore, in light of this, I'm requesting 1.4?. We have time for 1.4 so
> let's do this :)
> 
> Thanks to Yuan and Jan!

comment 5 already clarified that this is not a regression. Clearing the nomination again, as this was already clarified in above comment.
blocking-b2g: 1.4? → ---
I'm also unsure of what Garrett's testing validated on 1.3 here.
(In reply to Jason Smith [:jsmith] from comment #9)
> I'm also unsure of what Garrett's testing validated on 1.3 here.

I just checked this on today's build - I can reproduce this on a 1.3 build.

Comment 11

5 years ago
Can't we modify sendKey like this?

Promise sendKey(long keyCode, long charCode, long modifiers, boolean repeat);

The repeat argument mirrors the KeyboardEvent.repeat attribute.
Flags: needinfo?(ehsan)
FWIW I reproduced in 1.1 as well.

Would it be possible to fix this in the 1.4 timeframe ?

Updated

5 years ago
status-b2g18: --- → affected
status-b2g-v1.2: unaffected → affected
status-b2g-v1.3: unaffected → affected
I suspect we'll want to change the keyboard API such that the keyboard app tells the API when a key is pressed down and when it's released.

We can certainly have a sendKey function as well, but it's nice if not every keyboard app has to reimplement the logic of repeating keys.
So, no update here, while it's essential both for the Web and for the feature from Bug 954712.

It's clearly too late and risky for 1.4 but I'd like to prioritize this for 1.5. How do you do this in the Keyboard team? Clearly, setting a 1.5? blocker request won't work as they won't be triaged before weeks.

Can we move forward, please?
Flags: needinfo?(xyuan)
@Jonas. It may be complicate both for the API and the implementation to maintain the key press and down states. The keyboard APIs of the android and the chrome extension don't maintain the key states either and they use the "sendKey" way. So I'd like to try ehsan's way of comment 11 to have a sendKey function like this to solve the problem:

Promise sendKey(long keyCode, long charCode, long modifiers, boolean repeat);
 @julienw. 


If we use the "sendKey" way, I'm sure we can solve this for 1.5.
Flags: needinfo?(xyuan)
I'll leave this up to Ehsan and you to decide.
Yuan, can I tentatively assign the bug to you?
Sure.
Assignee: nobody → xyuan
Status: NEW → ASSIGNED

Comment 19

5 years ago
I think the logic of repeating keys may actually end up being keyboard app specific, for example, the application may want to expose an option to the user whether key repeating is enabled or not, and having the repeating decision made on the platform side based on individual keydown/keyup calls from the keyboard app will make it impossible to write those kinds of apps, unless we expose both ways of sending key events, which may not be ideal.  So my suggestion is to stick with comment 11.
Jonas' suggestion from comment 13 would work as well, I think both proposals are equivalent, although the "sendKey" proposal looks more natural.

Comment 21

5 years ago
(In reply to comment #20)
> Jonas' suggestion from comment 13 would work as well, I think both proposals
> are equivalent, although the "sendKey" proposal looks more natural.

The main difference between the two proposals is where the repeat detection logic lives.  With Jonas' proposal, it lives on the platform side, with my proposal, it lives in the keyboard application side.
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #21)
> (In reply to comment #20)
> > Jonas' suggestion from comment 13 would work as well, I think both proposals
> > are equivalent, although the "sendKey" proposal looks more natural.
> 
> The main difference between the two proposals is where the repeat detection
> logic lives.  With Jonas' proposal, it lives on the platform side, with my
> proposal, it lives in the keyboard application side.

I mean, the keyboard application could just as easily simulate multiple key strokes instead of one long press using Jonas' proposal. But it would be a lot more dirty.

Comment 23

5 years ago
(In reply to comment #22)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #21)
> > (In reply to comment #20)
> > > Jonas' suggestion from comment 13 would work as well, I think both proposals
> > > are equivalent, although the "sendKey" proposal looks more natural.
> > 
> > The main difference between the two proposals is where the repeat detection
> > logic lives.  With Jonas' proposal, it lives on the platform side, with my
> > proposal, it lives in the keyboard application side.
> 
> I mean, the keyboard application could just as easily simulate multiple key
> strokes instead of one long press using Jonas' proposal. But it would be a lot
> more dirty.

It definitely could, I think the cleanliness of the code depends on the application but it would incur more calls into the API.
Wei Deng(wdeng@mozilla.om) will provide a patch to add the 'repeat' parameter to 'sendKey':

Promise sendKey(long keyCode, long charCode, long modifiers, boolean repeat);
Assignee: xyuan → wdeng
(Assignee)

Comment 25

5 years ago
Created attachment 8396113 [details] [diff] [review]
bug-960946.patch

the sendKey API is changed to
Promise sendKey(long keyCode, long charCode, long modifiers, optional boolean repeat);
Attachment #8396113 - Flags: review?(xyuan)
Comment on attachment 8396113 [details] [diff] [review]
bug-960946.patch

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

It seems we can reuse file_test_app.html other than add file_test_backspace_event.html.

::: dom/webidl/InputMethod.webidl
@@ +154,5 @@
>        * @return true if succeeds. Otherwise false if the input context becomes void.
>        * Alternative: sendKey(KeyboardEvent event), but we will likely
>        * waste memory for creating the KeyboardEvent object.
> +      * Note that, if you want to send a key n times repeatedly, make sure set parameter repeat to true and invoke
> +      * sendKey n-1 times, then set repeat to false in the last invoke.

..., and then ...
Attachment #8396113 - Flags: review?(xyuan) → review+
(Assignee)

Comment 27

4 years ago
Created attachment 8396169 [details] [diff] [review]
gecko patch v2
Attachment #8396113 - Attachment is obsolete: true
Attachment #8396169 - Flags: review?(xyuan)
(Assignee)

Comment 28

4 years ago
Created attachment 8396198 [details] [diff] [review]
gecko patch
Attachment #8396169 - Attachment is obsolete: true
Attachment #8396169 - Flags: review?(xyuan)
Attachment #8396198 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Note to sheriff: We'll need a Gaia patch before closing this bug
(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #29)
> Note to sheriff: We'll need a Gaia patch before closing this bug

That's what leave-open is for ;)
Keywords: leave-open
FWIW, it makes sense to me that different keyboard apps might want to use different keypress repetition behavior. So putting repetition logic in the app rather than in the API sounds good.

Please make sure that we end up firing the right events though. As I recall it pressing and holding a key does not generate the same events as pressing the key multiple times really fast. I.e. I think pressing and holding fires "keydown,keypress,keypress,keypress...,keyup", whereas pressing multiple times really fast generates "keydown,keypress,keyup,keydown,keypress,keyup,keydown,keypress,keyup,..."
(In reply to Jonas Sicking (:sicking) from comment #32)

> Please make sure that we end up firing the right events though. As I recall
> it pressing and holding a key does not generate the same events as pressing
> the key multiple times really fast. I.e. I think pressing and holding fires
> "keydown,keypress,keypress,keypress...,keyup",

See comment 0 and comment 1: we get multiple keydown events too.
In Chrome, we don't get keypress events for backspace, but we get them for other normal keys. Firefox generates both keypress/keydown for all keys.

> whereas pressing multiple
> times really fast generates
> "keydown,keypress,keyup,keydown,keypress,keyup,keydown,keypress,keyup,..."
(Assignee)

Comment 35

4 years ago
yes, that's also what I want to say ...
(Assignee)

Comment 36

4 years ago
Created attachment 8397583 [details]
gaia patch

Made change in keyboard to the new sendKey API.
Attachment #8397583 - Flags: review?(rlu)
Comment on attachment 8397583 [details]
gaia patch

Sorry that this patch would cause a problem that, whey you press the backspace once, the suggestion would not be updated (or reverted) because input method won't get the delete key.

I'll clear the review flag first.
Thanks.
Attachment #8397583 - Flags: review?(rlu)
(Assignee)

Comment 38

4 years ago
OK, I see, if so, we have to do some changes in all the input methods I think.


(In reply to Rudy Lu [:rudyl] from comment #37)
> Comment on attachment 8397583 [details]
> gaia patch
> 
> Sorry that this patch would cause a problem that, whey you press the
> backspace once, the suggestion would not be updated (or reverted) because
> input method won't get the delete key.
> 
> I'll clear the review flag first.
> Thanks.
(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #34)
> See comment 0 and comment 1: we get multiple keydown events too.
> In Chrome, we don't get keypress events for backspace, but we get them for
> other normal keys. Firefox generates both keypress/keydown for all keys.

Ok. So I don't know what we should be getting for which keys :) Making sure we align with what web pages in Firefox Desktop get is probably the best thing to do. If we ever change that we can change this too.
(Assignee)

Comment 40

4 years ago
Created attachment 8398377 [details] [review]
gaia patch

Modify keybaord and latin IM according to the new sendKey API.
Attachment #8397583 - Attachment is obsolete: true
Attachment #8398377 - Flags: review?(rlu)
Comment on attachment 8398377 [details] [review]
gaia patch

This patch fixed the issue in my previous comment and per my test, should conform to the event behavior of Firefox Desktop, so giving a r+ here.

Thanks for this work.
Attachment #8398377 - Flags: review?(rlu) → review+
(Assignee)

Comment 42

4 years ago
Thanks for your review. Now, only the latin IM is changed to the new sendKey API, other IMs such as pinyin and zhuyin also should be changed to the new API. But, I'm not familiar with these parts. So, I hope, would you like to inform the IMs owners to make some changes? or anyone can help to do it?
  

(In reply to Rudy Lu [:rudyl] from comment #41)
> Comment on attachment 8398377 [details] [review]
> gaia patch
> 
> This patch fixed the issue in my previous comment and per my test, should
> conform to the event behavior of Firefox Desktop, so giving a r+ here.
> 
> Thanks for this work.
(In reply to wdeng@mozilla.com from comment #42)
> Thanks for your review. Now, only the latin IM is changed to the new sendKey
> API, other IMs such as pinyin and zhuyin also should be changed to the new
> API. But, I'm not familiar with these parts. So, I hope, would you like to
> inform the IMs owners to make some changes? or anyone can help to do it?
>   

This is another concern that I have: it's just too easy to make 3rd party keyboards fail this case :( How can we make it easier to not break the web?
(Assignee)

Updated

4 years ago
Keywords: leave-open → checkin-needed
(Assignee)

Updated

4 years ago
Depends on: 990377
(Assignee)

Updated

4 years ago
Depends on: 990379
(Assignee)

Updated

4 years ago
Blocks: 990379
No longer depends on: 990379
(Assignee)

Updated

4 years ago
No longer depends on: 990377
Master: https://github.com/mozilla-b2g/gaia/commit/66595cb4246de88440b483deb606ccd633d0a366
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-b2g18: affected → wontfix
status-b2g-v1.2: affected → wontfix
status-b2g-v1.3: affected → wontfix
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.5 S1 (9may)
Tested and working
1.5
Hamachi
Gecko 404219c
Gaia 2b8464d
Status: RESOLVED → VERIFIED
Pending v1.4
Status: VERIFIED → RESOLVED
Last Resolved: 4 years ago4 years ago
Loli, we won't uplift to v1.4, as far as I understand.
(In reply to Julien Wajsberg [:julienw] from comment #47)
> Loli, we won't uplift to v1.4, as far as I understand.

Ok, i verify it!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.