|
|
DescriptionRemove duplicate Caps Lock handling in CRD
In Chrome Remote Desktop, original code tries to match the client's caps
lock state with host's (Chrome OS) by toggling Caps Lock twice. But caps
lock state switch is triggered in different ways in different systems.
For example, Chrome OS triggers both ON and OFF on key release while
Linux triggers ON on key press and OFF on key release.
Let's assume client is Linux while host is Chrome OS. The caps lock
state of both systems is initialized to OFF. If you tap caps lock key in
client twice, the caps lock state at the time the key event is generated
(not the time the key event takes effect) is:
Press Release Press Release
Client OFF ON ON ON
Host OFF OFF ON ON
If the caps lock state of client is OFF while that of host is ON, the
state table will be:
Press Release Press Release
Client OFF ON ON ON
Host ON ON OFF OFF
So, if we trigger state toggling whenever state mismatches in client and
host, then we may toggle duplicately. So the solution is to trigger
toggling on key press when state mismatches. Also, we should not
trigger toggling for key that is possibly mapped to caps lock in event
rewriter. Otherwise, the toggling could occur duplicately.
BUG=719996
TEST=NONE
Review-Url: https://codereview.chromium.org/2881293002
Cr-Commit-Position: refs/heads/master@{#477110}
Committed: https://chromium.googlesource.com/chromium/src/+/64dd0e1fab7201713af95e7293e428074ef8536c
Patch Set 1 #Patch Set 2 : Remove duplicate Caps Lock handling in CRD #Patch Set 3 : Exclude all keycode possibly mapped to caps lock #
Total comments: 2
Patch Set 4 : Include reference. #Messages
Total messages: 37 (17 generated)
The CQ bit was checked by weidongg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
weidongg@chromium.org changed reviewers: + jamiewalch@chromium.org
jamiewalch@chromium.org changed reviewers: + rkjnsn@chromium.org
I'll let Erik take a first pass at this, since he's more familiar with the code than I am. This essentially reverts https://codereview.chromium.org/2852813002/, which suggests that we never had a problem with Caps Lock on ChromeOs. Erik, do you know if that's true, or did you verify the bug before working on the ChromeOS fix?
On 2017/05/16 00:03:34, Jamie wrote: > I'll let Erik take a first pass at this, since he's more familiar with the code > than I am. This essentially reverts https://codereview.chromium.org/2852813002/, > which suggests that we never had a problem with Caps Lock on ChromeOs. Erik, do > you know if that's true, or did you verify the bug before working on the > ChromeOS fix? The CL description mentions presses of caps lock getting double processed, which is true. However, the code in InputInjectorChromeos is primarily to intended to synchronize caps lock state when a *different* key is pressed (e.g., in the case where the user toggles caps lock locally when the CRD session doesn't have focus, which I did verify was not happening, before). If this is causing issues with caps lock key presses themselves (I didn't notice any, but it's certainly conceivable), the correct solution would be to only synchronize if the key press is not caps lock. That said, I see this is intended to address 719996, which I filed because caps lock synchronization was not being reflected on screen. I'll follow up there with more details of what we're trying to do.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Remove duplicate Caps Lock handling Tapping Caps Lock key in the client of Chrome Remote Desktop will trigger handling in both InputInjectorChromeos and AcceleratorController. So just remove the handling in the InputInjectorChromeos. BUG=719996 TEST=NONE ========== to ========== Remove duplicate Caps Lock handling in CRD In Chrome Remote Desktop, original code tries to match the client's caps lock state with host's (Chrome OS) by toggling Caps Lock twice. But caps lock state switch is triggered in different ways in different systems. For example, Chrome OS triggers both ON and OFF on key release while Linux triggers ON on key press and OFF on key release. Let's assume client is Linux while host is Chrome OS. The caps lock state of both systems is initialized to OFF. If you tap caps lock key in client twice, the caps lock state at the time the key event is generated (not the time the key event takes effect) is: Press Release Press Release Client OFF ON ON ON Host OFF OFF ON ON If the caps lock state of client is OFF while that of host is ON, the state table will be: Press Release Press Release Client OFF ON ON ON Host ON ON OFF OFF So, if we trigger state toggling whenever state mismatches in client and host, then we may toggle duplicately. So the solution is to trigger toggling on key press when state mismatches. BUG=719996 TEST=NONE ==========
I updated the CL description. This new patch set fixes the problem for Caps Lock. However, this does not work for remapped keys, as InputInjectorChromeos does not know whether the key will be mapped to Caps Lock in the EventRewriterChromeos which is called in EventSource after InputInjectorChromeos dispatches the key event.
On 2017/05/17 20:42:31, weidongg wrote: > I updated the CL description. This new patch set fixes the problem for Caps > Lock. However, this does not work for remapped keys, as InputInjectorChromeos > does not know whether the key will be mapped to Caps Lock in the > EventRewriterChromeos which is called in EventSource after InputInjectorChromeos > dispatches the key event. In addition to remapping, this also misses the not-so-uncommon case of using Alt+Meta to toggle caps lock when connecting from another Chrome OS machine. While we could add more special cases, I am wondering if the problem is worth the complexity of trying to address it. In my testing, when pressing caps lock on the client, the indicator flickers in the status bar, and then comes on (and stays on) when another key is pressed. This is obviously not ideal, but I'm not sure it's a serious problem. What do you think? Are there more significant issues that the extra toggles could cause?
On 2017/05/31 23:36:50, rkjnsn wrote: > On 2017/05/17 20:42:31, weidongg wrote: > > I updated the CL description. This new patch set fixes the problem for Caps > > Lock. However, this does not work for remapped keys, as InputInjectorChromeos > > does not know whether the key will be mapped to Caps Lock in the > > EventRewriterChromeos which is called in EventSource after > InputInjectorChromeos > > dispatches the key event. > > In addition to remapping, this also misses the not-so-uncommon case of using > Alt+Meta to toggle caps lock when connecting from another Chrome OS machine. > While we could add more special cases, I am wondering if the problem is worth > the complexity of trying to address it. In my testing, when pressing caps lock > on the client, the indicator flickers in the status bar, and then comes on (and > stays on) when another key is pressed. This is obviously not ideal, but I'm not > sure it's a serious problem. What do you think? Are there more significant > issues that the extra toggles could cause? (To clarify, the observed behavior I mentioned is what occurs when the original code is changed to call ImeKeyboard::SetCapsLockEnabled without including a special case for pressing the Caps Lock key, itself.)
On 2017/05/31 23:49:54, rkjnsn wrote: > On 2017/05/31 23:36:50, rkjnsn wrote: > > On 2017/05/17 20:42:31, weidongg wrote: > > > I updated the CL description. This new patch set fixes the problem for Caps > > > Lock. However, this does not work for remapped keys, as > InputInjectorChromeos > > > does not know whether the key will be mapped to Caps Lock in the > > > EventRewriterChromeos which is called in EventSource after > > InputInjectorChromeos > > > dispatches the key event. > > > > In addition to remapping, this also misses the not-so-uncommon case of using > > Alt+Meta to toggle caps lock when connecting from another Chrome OS machine. > > While we could add more special cases, I am wondering if the problem is worth > > the complexity of trying to address it. In my testing, when pressing caps lock > > on the client, the indicator flickers in the status bar, and then comes on > (and > > stays on) when another key is pressed. This is obviously not ideal, but I'm > not > > sure it's a serious problem. What do you think? Are there more significant > > issues that the extra toggles could cause? > > (To clarify, the observed behavior I mentioned is what occurs when the original > code is changed to call ImeKeyboard::SetCapsLockEnabled without including a > special case for pressing the Caps Lock key, itself.) Yes, you are right, alt+meta is another case to consider in the CL. In the change you mentioned, raw caps lock key seems to not work as well. eg. I could turn on indicator on system tray by pressing caps lock from client, but I could not turn it off by pressing again. Then I press a alphabet key, the indicator get turned off. If we completely remove the caps lock match here, this problem seems to be a lot easier to solve, at least there's no complicated bugs. When the client and host do not match, user could always manually match it. I mean, is there any concern to completely remove the caps lock match?
On 2017/06/01 00:31:43, weidongg wrote: > On 2017/05/31 23:49:54, rkjnsn wrote: > > On 2017/05/31 23:36:50, rkjnsn wrote: > > > On 2017/05/17 20:42:31, weidongg wrote: > > > > I updated the CL description. This new patch set fixes the problem for > Caps > > > > Lock. However, this does not work for remapped keys, as > > InputInjectorChromeos > > > > does not know whether the key will be mapped to Caps Lock in the > > > > EventRewriterChromeos which is called in EventSource after > > > InputInjectorChromeos > > > > dispatches the key event. > > > > > > In addition to remapping, this also misses the not-so-uncommon case of using > > > Alt+Meta to toggle caps lock when connecting from another Chrome OS machine. > > > While we could add more special cases, I am wondering if the problem is > worth > > > the complexity of trying to address it. In my testing, when pressing caps > lock > > > on the client, the indicator flickers in the status bar, and then comes on > > (and > > > stays on) when another key is pressed. This is obviously not ideal, but I'm > > not > > > sure it's a serious problem. What do you think? Are there more significant > > > issues that the extra toggles could cause? > > > > (To clarify, the observed behavior I mentioned is what occurs when the > original > > code is changed to call ImeKeyboard::SetCapsLockEnabled without including a > > special case for pressing the Caps Lock key, itself.) > > Yes, you are right, alt+meta is another case to consider in the CL. > In the change you mentioned, raw caps lock key seems to not work as well. eg. I > could turn on indicator on system tray by pressing caps lock from client, but I > could not turn it off by pressing again. Then I press a alphabet key, the > indicator get turned off. > If we completely remove the caps lock match here, this problem seems to be a lot > easier to solve, at least there's no complicated bugs. When the client and host > do not match, user could always manually match it. I mean, is there any concern > to completely remove the caps lock match? Are you suggesting we just don't toggle the caps lock state on the host to match client at all? We need this in some form to address https://crbug.com/176436
On 2017/06/01 01:33:43, rkjnsn wrote: > On 2017/06/01 00:31:43, weidongg wrote: > > On 2017/05/31 23:49:54, rkjnsn wrote: > > > On 2017/05/31 23:36:50, rkjnsn wrote: > > > > On 2017/05/17 20:42:31, weidongg wrote: > > > > > I updated the CL description. This new patch set fixes the problem for > > Caps > > > > > Lock. However, this does not work for remapped keys, as > > > InputInjectorChromeos > > > > > does not know whether the key will be mapped to Caps Lock in the > > > > > EventRewriterChromeos which is called in EventSource after > > > > InputInjectorChromeos > > > > > dispatches the key event. > > > > > > > > In addition to remapping, this also misses the not-so-uncommon case of > using > > > > Alt+Meta to toggle caps lock when connecting from another Chrome OS > machine. > > > > While we could add more special cases, I am wondering if the problem is > > worth > > > > the complexity of trying to address it. In my testing, when pressing caps > > lock > > > > on the client, the indicator flickers in the status bar, and then comes on > > > (and > > > > stays on) when another key is pressed. This is obviously not ideal, but > I'm > > > not > > > > sure it's a serious problem. What do you think? Are there more significant > > > > issues that the extra toggles could cause? > > > > > > (To clarify, the observed behavior I mentioned is what occurs when the > > original > > > code is changed to call ImeKeyboard::SetCapsLockEnabled without including a > > > special case for pressing the Caps Lock key, itself.) > > > > Yes, you are right, alt+meta is another case to consider in the CL. > > In the change you mentioned, raw caps lock key seems to not work as well. eg. > I > > could turn on indicator on system tray by pressing caps lock from client, but > I > > could not turn it off by pressing again. Then I press a alphabet key, the > > indicator get turned off. > > If we completely remove the caps lock match here, this problem seems to be a > lot > > easier to solve, at least there's no complicated bugs. When the client and > host > > do not match, user could always manually match it. I mean, is there any > concern > > to completely remove the caps lock match? > > Are you suggesting we just don't toggle the caps lock state on the host to match > client at all? We need this in some form to address https://crbug.com/176436 IMHO, even if we solve the problem above, we could only implement client->host match instead of bidirectional match, as InputInjector only takes input from client while does not give feedback, right? Think about this: Client is linux and host is chrome os. Initially, both client and host are caps lock on. The user turn off the caps lock using 'alt+search key' from client. The result is that host's caps lock is off, but client's is on. Then the user types some text, these text will be lower case due to the client->host match. And this is not very user friendly right?
On 2017/06/01 03:24:28, weidongg wrote: > On 2017/06/01 01:33:43, rkjnsn wrote: > > On 2017/06/01 00:31:43, weidongg wrote: > > > On 2017/05/31 23:49:54, rkjnsn wrote: > > > > On 2017/05/31 23:36:50, rkjnsn wrote: > > > > > On 2017/05/17 20:42:31, weidongg wrote: > > > > > > I updated the CL description. This new patch set fixes the problem for > > > Caps > > > > > > Lock. However, this does not work for remapped keys, as > > > > InputInjectorChromeos > > > > > > does not know whether the key will be mapped to Caps Lock in the > > > > > > EventRewriterChromeos which is called in EventSource after > > > > > InputInjectorChromeos > > > > > > dispatches the key event. > > > > > > > > > > In addition to remapping, this also misses the not-so-uncommon case of > > using > > > > > Alt+Meta to toggle caps lock when connecting from another Chrome OS > > machine. > > > > > While we could add more special cases, I am wondering if the problem is > > > worth > > > > > the complexity of trying to address it. In my testing, when pressing > caps > > > lock > > > > > on the client, the indicator flickers in the status bar, and then comes > on > > > > (and > > > > > stays on) when another key is pressed. This is obviously not ideal, but > > I'm > > > > not > > > > > sure it's a serious problem. What do you think? Are there more > significant > > > > > issues that the extra toggles could cause? > > > > > > > > (To clarify, the observed behavior I mentioned is what occurs when the > > > original > > > > code is changed to call ImeKeyboard::SetCapsLockEnabled without including > a > > > > special case for pressing the Caps Lock key, itself.) > > > > > > Yes, you are right, alt+meta is another case to consider in the CL. > > > In the change you mentioned, raw caps lock key seems to not work as well. > eg. > > I > > > could turn on indicator on system tray by pressing caps lock from client, > but > > I > > > could not turn it off by pressing again. Then I press a alphabet key, the > > > indicator get turned off. > > > If we completely remove the caps lock match here, this problem seems to be a > > lot > > > easier to solve, at least there's no complicated bugs. When the client and > > host > > > do not match, user could always manually match it. I mean, is there any > > concern > > > to completely remove the caps lock match? > > > > Are you suggesting we just don't toggle the caps lock state on the host to > match > > client at all? We need this in some form to address https://crbug.com/176436 > IMHO, even if we solve the problem above, we could only implement > client->host match instead of bidirectional match, as InputInjector > only takes input from client while does not give feedback, right? > Think about this: Client is linux and host is chrome os. Initially, > both client and host are caps lock on. The user turn off the caps > lock using 'alt+search key' from client. The result is that host's > caps lock is off, but client's is on. Then the user types some text, > these text will be lower case due to the client->host match. And > this is not very user friendly right? I don't think there's a universally right answer here. The scenario you describe is valid (although it's almost certain that the client keyboard has a Caps Lock key, and I think it's more likely that the user would use that in preference to Alt+Search). However, if we choose not to synchronize Caps Lock state, then any changes to the client state that occur while the client window is unfocused will result in it being out-of-sync between client and host. For ChromeOS, there's an argument that this is okay, because it has an on-screen Caps Lock indicator (assuming that that is always the case), but it complicates the cross-platform story, because now we have to preface any description of this feature with "except ChromeOS". Allowing Caps Lock desync also means that any workflow that involves switching between local and remote applications requires the user to toggle Caps Lock each time, so even if we can point to the host-side indicator and say that we're trying to respect it, I think it's pretty poor UX.
Description was changed from ========== Remove duplicate Caps Lock handling in CRD In Chrome Remote Desktop, original code tries to match the client's caps lock state with host's (Chrome OS) by toggling Caps Lock twice. But caps lock state switch is triggered in different ways in different systems. For example, Chrome OS triggers both ON and OFF on key release while Linux triggers ON on key press and OFF on key release. Let's assume client is Linux while host is Chrome OS. The caps lock state of both systems is initialized to OFF. If you tap caps lock key in client twice, the caps lock state at the time the key event is generated (not the time the key event takes effect) is: Press Release Press Release Client OFF ON ON ON Host OFF OFF ON ON If the caps lock state of client is OFF while that of host is ON, the state table will be: Press Release Press Release Client OFF ON ON ON Host ON ON OFF OFF So, if we trigger state toggling whenever state mismatches in client and host, then we may toggle duplicately. So the solution is to trigger toggling on key press when state mismatches. BUG=719996 TEST=NONE ========== to ========== Remove duplicate Caps Lock handling in CRD In Chrome Remote Desktop, original code tries to match the client's caps lock state with host's (Chrome OS) by toggling Caps Lock twice. But caps lock state switch is triggered in different ways in different systems. For example, Chrome OS triggers both ON and OFF on key release while Linux triggers ON on key press and OFF on key release. Let's assume client is Linux while host is Chrome OS. The caps lock state of both systems is initialized to OFF. If you tap caps lock key in client twice, the caps lock state at the time the key event is generated (not the time the key event takes effect) is: Press Release Press Release Client OFF ON ON ON Host OFF OFF ON ON If the caps lock state of client is OFF while that of host is ON, the state table will be: Press Release Press Release Client OFF ON ON ON Host ON ON OFF OFF So, if we trigger state toggling whenever state mismatches in client and host, then we may toggle duplicately. So the solution is to trigger toggling on key press when state mismatches. Also we should not toggle when key code is possible mapped to caps lock in event rewriter. Otherwise, the toggling could occur duplicately. BUG=719996 TEST=NONE ==========
Description was changed from ========== Remove duplicate Caps Lock handling in CRD In Chrome Remote Desktop, original code tries to match the client's caps lock state with host's (Chrome OS) by toggling Caps Lock twice. But caps lock state switch is triggered in different ways in different systems. For example, Chrome OS triggers both ON and OFF on key release while Linux triggers ON on key press and OFF on key release. Let's assume client is Linux while host is Chrome OS. The caps lock state of both systems is initialized to OFF. If you tap caps lock key in client twice, the caps lock state at the time the key event is generated (not the time the key event takes effect) is: Press Release Press Release Client OFF ON ON ON Host OFF OFF ON ON If the caps lock state of client is OFF while that of host is ON, the state table will be: Press Release Press Release Client OFF ON ON ON Host ON ON OFF OFF So, if we trigger state toggling whenever state mismatches in client and host, then we may toggle duplicately. So the solution is to trigger toggling on key press when state mismatches. BUG=719996 TEST=NONE ========== to ========== Remove duplicate Caps Lock handling in CRD In Chrome Remote Desktop, original code tries to match the client's caps lock state with host's (Chrome OS) by toggling Caps Lock twice. But caps lock state switch is triggered in different ways in different systems. For example, Chrome OS triggers both ON and OFF on key release while Linux triggers ON on key press and OFF on key release. Let's assume client is Linux while host is Chrome OS. The caps lock state of both systems is initialized to OFF. If you tap caps lock key in client twice, the caps lock state at the time the key event is generated (not the time the key event takes effect) is: Press Release Press Release Client OFF ON ON ON Host OFF OFF ON ON If the caps lock state of client is OFF while that of host is ON, the state table will be: Press Release Press Release Client OFF ON ON ON Host ON ON OFF OFF So, if we trigger state toggling whenever state mismatches in client and host, then we may toggle duplicately. So the solution is to trigger toggling on key press when state mismatches. Also, we should not trigger toggling for key that is possibly mapped to caps lock in event rewriter. Otherwise, the toggling could occur duplicately. BUG=719996 TEST=NONE ==========
Description was changed from ========== Remove duplicate Caps Lock handling in CRD In Chrome Remote Desktop, original code tries to match the client's caps lock state with host's (Chrome OS) by toggling Caps Lock twice. But caps lock state switch is triggered in different ways in different systems. For example, Chrome OS triggers both ON and OFF on key release while Linux triggers ON on key press and OFF on key release. Let's assume client is Linux while host is Chrome OS. The caps lock state of both systems is initialized to OFF. If you tap caps lock key in client twice, the caps lock state at the time the key event is generated (not the time the key event takes effect) is: Press Release Press Release Client OFF ON ON ON Host OFF OFF ON ON If the caps lock state of client is OFF while that of host is ON, the state table will be: Press Release Press Release Client OFF ON ON ON Host ON ON OFF OFF So, if we trigger state toggling whenever state mismatches in client and host, then we may toggle duplicately. So the solution is to trigger toggling on key press when state mismatches. Also, we should not trigger toggling for key that is possibly mapped to caps lock in event rewriter. Otherwise, the toggling could occur duplicately. BUG=719996 TEST=NONE ========== to ========== Remove duplicate Caps Lock handling in CRD In Chrome Remote Desktop, original code tries to match the client's caps lock state with host's (Chrome OS) by toggling Caps Lock twice. But caps lock state switch is triggered in different ways in different systems. For example, Chrome OS triggers both ON and OFF on key release while Linux triggers ON on key press and OFF on key release. Let's assume client is Linux while host is Chrome OS. The caps lock state of both systems is initialized to OFF. If you tap caps lock key in client twice, the caps lock state at the time the key event is generated (not the time the key event takes effect) is: Press Release Press Release Client OFF ON ON ON Host OFF OFF ON ON If the caps lock state of client is OFF while that of host is ON, the state table will be: Press Release Press Release Client OFF ON ON ON Host ON ON OFF OFF So, if we trigger state toggling whenever state mismatches in client and host, then we may toggle duplicately. So the solution is to trigger toggling on key press when state mismatches. Also, we should not trigger toggling for key that is possibly mapped to caps lock in event rewriter. Otherwise, the toggling could occur duplicately. BUG=719996 TEST=NONE ==========
On 2017/06/01 16:24:36, Jamie wrote: > On 2017/06/01 03:24:28, weidongg wrote: > > On 2017/06/01 01:33:43, rkjnsn wrote: > > > On 2017/06/01 00:31:43, weidongg wrote: > > > > On 2017/05/31 23:49:54, rkjnsn wrote: > > > > > On 2017/05/31 23:36:50, rkjnsn wrote: > > > > > > On 2017/05/17 20:42:31, weidongg wrote: > > > > > > > I updated the CL description. This new patch set fixes the problem > for > > > > Caps > > > > > > > Lock. However, this does not work for remapped keys, as > > > > > InputInjectorChromeos > > > > > > > does not know whether the key will be mapped to Caps Lock in the > > > > > > > EventRewriterChromeos which is called in EventSource after > > > > > > InputInjectorChromeos > > > > > > > dispatches the key event. > > > > > > > > > > > > In addition to remapping, this also misses the not-so-uncommon case of > > > using > > > > > > Alt+Meta to toggle caps lock when connecting from another Chrome OS > > > machine. > > > > > > While we could add more special cases, I am wondering if the problem > is > > > > worth > > > > > > the complexity of trying to address it. In my testing, when pressing > > caps > > > > lock > > > > > > on the client, the indicator flickers in the status bar, and then > comes > > on > > > > > (and > > > > > > stays on) when another key is pressed. This is obviously not ideal, > but > > > I'm > > > > > not > > > > > > sure it's a serious problem. What do you think? Are there more > > significant > > > > > > issues that the extra toggles could cause? > > > > > > > > > > (To clarify, the observed behavior I mentioned is what occurs when the > > > > original > > > > > code is changed to call ImeKeyboard::SetCapsLockEnabled without > including > > a > > > > > special case for pressing the Caps Lock key, itself.) > > > > > > > > Yes, you are right, alt+meta is another case to consider in the CL. > > > > In the change you mentioned, raw caps lock key seems to not work as well. > > eg. > > > I > > > > could turn on indicator on system tray by pressing caps lock from client, > > but > > > I > > > > could not turn it off by pressing again. Then I press a alphabet key, the > > > > indicator get turned off. > > > > If we completely remove the caps lock match here, this problem seems to be > a > > > lot > > > > easier to solve, at least there's no complicated bugs. When the client and > > > host > > > > do not match, user could always manually match it. I mean, is there any > > > concern > > > > to completely remove the caps lock match? > > > > > > Are you suggesting we just don't toggle the caps lock state on the host to > > match > > > client at all? We need this in some form to address https://crbug.com/176436 > > IMHO, even if we solve the problem above, we could only implement > > client->host match instead of bidirectional match, as InputInjector > > only takes input from client while does not give feedback, right? > > Think about this: Client is linux and host is chrome os. Initially, > > both client and host are caps lock on. The user turn off the caps > > lock using 'alt+search key' from client. The result is that host's > > caps lock is off, but client's is on. Then the user types some text, > > these text will be lower case due to the client->host match. And > > this is not very user friendly right? > > I don't think there's a universally right answer here. The scenario you describe > is valid (although it's almost certain that the client keyboard has a Caps Lock > key, and I think it's more likely that the user would use that in preference to > Alt+Search). However, if we choose not to synchronize Caps Lock state, then any > changes to the client state that occur while the client window is unfocused will > result in it being out-of-sync between client and host. For ChromeOS, there's an > argument that this is okay, because it has an on-screen Caps Lock indicator > (assuming that that is always the case), but it complicates the cross-platform > story, because now we have to preface any description of this feature with > "except ChromeOS". Allowing Caps Lock desync also means that any workflow that > involves switching between local and remote applications requires the user to > toggle Caps Lock each time, so even if we can point to the host-side indicator > and say that we're trying to respect it, I think it's pretty poor UX. Hey, I would like to propose a solution to match the lock states. We could only set lock states on key press to prevent duplicate toggling. We also need to exclude all the key that is possibly mapped to caps lock to prevent duplicate toggling too. With this solution, host's lock states will match client's when user tap keys that are could not be rewritten (I think it's pretty reasonable since caps lock state essentially has not effect on the rewritten keys like shift, ctrl, etc., right?). Both alt+search and rewritten caps lock key would work properly and indicator shows correctly in matched situation. For details, please check out the patch set 3.
The CQ bit was checked by weidongg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This approach seems reasonable. https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_inj... File remoting/host/input_injector_chromeos.cc (right): https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:54: // rewriter. Can you include a reference to where this list comes from?
https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_inj... File remoting/host/input_injector_chromeos.cc (right): https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:54: // rewriter. On 2017/06/05 17:49:23, rkjnsn wrote: > Can you include a reference to where this list comes from? https://cs.chromium.org/chromium/src/ui/chromeos/events/event_rewriter_chrome...
On 2017/06/05 20:06:57, weidongg wrote: > https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_inj... > File remoting/host/input_injector_chromeos.cc (right): > > https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_inj... > remoting/host/input_injector_chromeos.cc:54: // rewriter. > On 2017/06/05 17:49:23, rkjnsn wrote: > > Can you include a reference to where this list comes from? > > https://cs.chromium.org/chromium/src/ui/chromeos/events/event_rewriter_chrome... Thanks, but can you include this in the code as well? Even if the location changes, I think knowing the original location will be useful for future forensic efforts to understand how these keys were chosen. Also, looking at that code, it seems that only the first case of the switch statement refers to CapsLock. Is it simply that GetRemappedKey could conceivably return CapsLock for any input?
On 2017/06/05 20:24:39, Jamie wrote: > On 2017/06/05 20:06:57, weidongg wrote: > > > https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_inj... > > File remoting/host/input_injector_chromeos.cc (right): > > > > > https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_inj... > > remoting/host/input_injector_chromeos.cc:54: // rewriter. > > On 2017/06/05 17:49:23, rkjnsn wrote: > > > Can you include a reference to where this list comes from? > > > > > https://cs.chromium.org/chromium/src/ui/chromeos/events/event_rewriter_chrome... > > Thanks, but can you include this in the code as well? Even if the location > changes, I think knowing the original location will be useful for future > forensic efforts to understand how these keys were chosen. Yes, the URL is too long, so I include the method name in the new patch set. > > Also, looking at that code, it seems that only the first case of the switch > statement refers to CapsLock. Is it simply that GetRemappedKey could conceivably > return CapsLock for any input? Yes, all these cases can be mapped to caps lock. You could go to settings->device->keyboard to check these rewritten keys.
On 2017/06/05 20:24:39, Jamie wrote: > Also, looking at that code, it seems that only the first case of the switch > statement refers to CapsLock. Is it simply that GetRemappedKey could conceivably > return CapsLock for any input? Indeed, any of these keys could be configured to generate caps lock in the keyboard settings. LGTM Jamie, can you give owner approval?
lgtm
Thanks for the review.
The CQ bit was checked by weidongg@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496700349257460, "parent_rev": "582feef2ec35d80ce6c16da102f7fc337f59be21", "commit_rev": "64dd0e1fab7201713af95e7293e428074ef8536c"}
Message was sent while issue was closed.
Description was changed from ========== Remove duplicate Caps Lock handling in CRD In Chrome Remote Desktop, original code tries to match the client's caps lock state with host's (Chrome OS) by toggling Caps Lock twice. But caps lock state switch is triggered in different ways in different systems. For example, Chrome OS triggers both ON and OFF on key release while Linux triggers ON on key press and OFF on key release. Let's assume client is Linux while host is Chrome OS. The caps lock state of both systems is initialized to OFF. If you tap caps lock key in client twice, the caps lock state at the time the key event is generated (not the time the key event takes effect) is: Press Release Press Release Client OFF ON ON ON Host OFF OFF ON ON If the caps lock state of client is OFF while that of host is ON, the state table will be: Press Release Press Release Client OFF ON ON ON Host ON ON OFF OFF So, if we trigger state toggling whenever state mismatches in client and host, then we may toggle duplicately. So the solution is to trigger toggling on key press when state mismatches. Also, we should not trigger toggling for key that is possibly mapped to caps lock in event rewriter. Otherwise, the toggling could occur duplicately. BUG=719996 TEST=NONE ========== to ========== Remove duplicate Caps Lock handling in CRD In Chrome Remote Desktop, original code tries to match the client's caps lock state with host's (Chrome OS) by toggling Caps Lock twice. But caps lock state switch is triggered in different ways in different systems. For example, Chrome OS triggers both ON and OFF on key release while Linux triggers ON on key press and OFF on key release. Let's assume client is Linux while host is Chrome OS. The caps lock state of both systems is initialized to OFF. If you tap caps lock key in client twice, the caps lock state at the time the key event is generated (not the time the key event takes effect) is: Press Release Press Release Client OFF ON ON ON Host OFF OFF ON ON If the caps lock state of client is OFF while that of host is ON, the state table will be: Press Release Press Release Client OFF ON ON ON Host ON ON OFF OFF So, if we trigger state toggling whenever state mismatches in client and host, then we may toggle duplicately. So the solution is to trigger toggling on key press when state mismatches. Also, we should not trigger toggling for key that is possibly mapped to caps lock in event rewriter. Otherwise, the toggling could occur duplicately. BUG=719996 TEST=NONE Review-Url: https://codereview.chromium.org/2881293002 Cr-Commit-Position: refs/heads/master@{#477110} Committed: https://chromium.googlesource.com/chromium/src/+/64dd0e1fab7201713af95e7293e4... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/64dd0e1fab7201713af95e7293e4... |