Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10389035:
Do not consume an ET_KEY_RELEASED event in NativeTextfieldViews and NativeComboboxViews (Closed)
|
Created:
8 years, 7 months ago by Yusuke Sato Modified:
8 years, 7 months ago CC:
chromium-reviews, tfarina, James Su, penghuang+watch_chromium.org, yusukes+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDo not consume an ET_KEY_RELEASED event in NativeTextfieldViews and NativeComboboxViews so FocusManager could process an ET_KEY_RELEASED accelerator.
BUG=127520, 123856
TEST=try
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137164
Patch Set 1 : review #
Total comments: 4
Messages
Total messages: 14 (0 generated)
http://codereview.chromium.org/10389035/diff/5002/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/10389035/diff/5002/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:497: return false; // crbug.com/127520 Are we sure we don't want to return true when KeyPress was handled? just double checking.
http://codereview.chromium.org/10389035/diff/5002/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/10389035/diff/5002/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:497: return false; // crbug.com/127520 On 2012/05/10 15:36:37, oshima wrote: > Are we sure we don't want to return true when KeyPress was handled? just double > checking. Unconditionally returning false would be better, I believe. For example, when Shift+Alt+X+ET_KEY_PRESSED is always consumed by the key press handler and Shift+Alt+ET_KEY_RELEASED is registered as an accelerator, Shift+Alt+X press/release should not trigger the Shift+Alt accelerator. However, returning true for the X release makes it very hard for the accelerator manager to distinguish Shift+Alt+X and Shit+Alt.
Please feel free to ping me when you find me online. It'd take longer to discuss this across pacific ocean:) http://codereview.chromium.org/10389035/diff/5002/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/10389035/diff/5002/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:497: return false; // crbug.com/127520 On 2012/05/11 02:16:22, Yusuke Sato wrote: > On 2012/05/10 15:36:37, oshima wrote: > > Are we sure we don't want to return true when KeyPress was handled? just > double > > checking. > > Unconditionally returning false would be better, I believe. > > For example, when Shift+Alt+X+ET_KEY_PRESSED is always consumed by the key press > handler and Shift+Alt+ET_KEY_RELEASED is registered as an accelerator, > Shift+Alt+X press/release should not trigger the Shift+Alt accelerator. However, > returning true for the X release makes it very hard for the accelerator manager > to distinguish Shift+Alt+X and Shit+Alt. Does it distinguish it now? I'm probably OK with this change unless this cause some issue, but we may want to think about long term plan.
by the way, you can just write BUG=127520,123856 On 2012/05/11 17:23:03, oshima wrote: > Please feel free to ping me when you find me online. It'd take longer to discuss > this across pacific ocean:) > > http://codereview.chromium.org/10389035/diff/5002/ui/views/controls/textfield... > File ui/views/controls/textfield/native_textfield_views.cc (right): > > http://codereview.chromium.org/10389035/diff/5002/ui/views/controls/textfield... > ui/views/controls/textfield/native_textfield_views.cc:497: return false; // > crbug.com/127520 > On 2012/05/11 02:16:22, Yusuke Sato wrote: > > On 2012/05/10 15:36:37, oshima wrote: > > > Are we sure we don't want to return true when KeyPress was handled? just > > double > > > checking. > > > > Unconditionally returning false would be better, I believe. > > > > For example, when Shift+Alt+X+ET_KEY_PRESSED is always consumed by the key > press > > handler and Shift+Alt+ET_KEY_RELEASED is registered as an accelerator, > > Shift+Alt+X press/release should not trigger the Shift+Alt accelerator. > However, > > returning true for the X release makes it very hard for the accelerator > manager > > to distinguish Shift+Alt+X and Shit+Alt. > > Does it distinguish it now? I'm probably OK with this change unless this cause > some issue, but we may want to think about long term plan.
Sure, I'll ping you tomorrow (on your Monday). http://codereview.chromium.org/10389035/diff/5002/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/10389035/diff/5002/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:497: return false; // crbug.com/127520 On 2012/05/11 17:23:03, oshima wrote: > On 2012/05/11 02:16:22, Yusuke Sato wrote: > > On 2012/05/10 15:36:37, oshima wrote: > > > Are we sure we don't want to return true when KeyPress was handled? just > > double > > > checking. > > > > Unconditionally returning false would be better, I believe. > > > > For example, when Shift+Alt+X+ET_KEY_PRESSED is always consumed by the key > press > > handler and Shift+Alt+ET_KEY_RELEASED is registered as an accelerator, > > Shift+Alt+X press/release should not trigger the Shift+Alt accelerator. > However, > > returning true for the X release makes it very hard for the accelerator > manager > > to distinguish Shift+Alt+X and Shit+Alt. > > Does it distinguish it now? I'm probably OK with this change unless this cause > some issue, but we may want to think about long term plan. Yes. ui::AcceleratorManager (ui/base/accelerators/accelerator_manager.h) has an ability to distinguish between Shift+Alt+X and Shift+Alt. In short, 'bool AcceleratorManager::ShouldHandle(const Accelerator& accelerator) const' method in the manager class returns true when Alt is released after Shift+Alt are pressed, but returns false when Alt is released after Shift+Alt+<whatever> are pressed.
Discussed offline and agreed that better fix is to always update accelerator so that accelerator doesn't depend on keyevent consumer's behavior. yusuke will look into this in separate CL, so this CL LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusukes@chromium.org/10389035/5002
Presubmit check for 10389035-5002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
ui/views
Presubmit checks took 1.1s to calculate.
+sky Could you do an owners review? On 2012/05/15 04:46:22, I haz the power (commit-bot) wrote: > Presubmit check for 10389035-5002 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for files in these directories: > ui/views > > Presubmit checks took 1.1s to calculate.
oshima: http://codereview.chromium.org/10381145/ (not ready for review though) On 2012/05/15 04:47:54, Yusuke Sato wrote: > +sky > > Could you do an owners review? > > On 2012/05/15 04:46:22, I haz the power (commit-bot) wrote: > > Presubmit check for 10389035-5002 failed and returned exit status 1. > > > > Running presubmit commit checks ... > > > > ** Presubmit ERRORS ** > > Missing LGTM from an OWNER for files in these directories: > > ui/views > > > > Presubmit checks took 1.1s to calculate.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusukes@chromium.org/10389035/5002
Change committed as 137164 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
