Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1849273003:
Fix for Alt+Left/Right page navigation with focus on radio buttons. (Closed)
|
Created:
4 years, 8 months ago by Paritosh Kumar Modified:
4 years, 7 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, Habib Virji Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix for Alt+Left/Right page navigation with focus on radio buttons.
Added a condition for modifier state before handling keydown event.
BUG=599009
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Messages
Total messages: 11 (2 generated)
Description was changed from ========== [WIP] Fix for Alt+Left/Right page navigation with focus on radio buttons. Added a condition for modifier state before handling keydown event. BUG=599009 ========== to ========== Fix for Alt+Left/Right page navigation with focus on radio buttons. Added a condition for modifier state before handling keydown event. BUG=599009 ==========
paritosh.in@samsung.com changed reviewers: + dtapuska@chromium.org, tkent@chromium.org
Please have a look. I have added a check for modifier state before handling key down. I tried writing tests for this in this way: 1. Navigate from first page to second page 2. On second page trigger alt+Left key event on radio button. 3. And use url of current window for validation. But, validation on first page doesn't work. Please share your input.
https://codereview.chromium.org/1849273003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/forms/RadioInputType.cpp (right): https://codereview.chromium.org/1849273003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/forms/RadioInputType.cpp:98: if (event->modifiers() & PlatformEvent::AltKey) How does this work on Mac? I know Option left/right implements the direction there. I wonder if this should be if (event->modifiers() & PlatformEvent:KeyModifiers) instead?
Thanks dtapuska for your input. https://codereview.chromium.org/1849273003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/forms/RadioInputType.cpp (right): https://codereview.chromium.org/1849273003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/forms/RadioInputType.cpp:98: if (event->modifiers() & PlatformEvent::AltKey) > How does this work on Mac? I know Option left/right implements the direction > there. I think AltKey is same as OptionKey of Mac. > I wonder if this should be if (event->modifiers() & PlatformEvent:KeyModifiers) > instead? But, I think this will prevent user to select another radio button using arrow key with any one of (ctrl/shift/Fn/Os) key. I compared the behavior of different browsers,(i.e. IE and FF) ctrl+Left - No effect on the selection/Page Navigation shift/Fn + Left - Selection changes as expected. Alt+Left - Page Navigation. After this change we will differ with IE and FF only on ctrl+Left. For this, I think we should use, if (event->ctrlKey() || event->metaKey() || event->altKey()). Please suggest.
On 2016/04/04 18:00:06, Paritosh Kumar wrote: > Thanks dtapuska for your input. > > https://codereview.chromium.org/1849273003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/forms/RadioInputType.cpp (right): > > https://codereview.chromium.org/1849273003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/forms/RadioInputType.cpp:98: if > (event->modifiers() & PlatformEvent::AltKey) > > > How does this work on Mac? I know Option left/right implements the direction > > there. > > I think AltKey is same as OptionKey of Mac. > > > I wonder if this should be if (event->modifiers() & > PlatformEvent:KeyModifiers) > > instead? > > But, I think this will prevent user to select another radio button using arrow > key with any one of (ctrl/shift/Fn/Os) key. > > I compared the behavior of different browsers,(i.e. IE and FF) > ctrl+Left - No effect on the selection/Page Navigation > shift/Fn + Left - Selection changes as expected. > Alt+Left - Page Navigation. > > After this change we will differ with IE and FF only on ctrl+Left. > For this, I think we should use, > if (event->ctrlKey() || event->metaKey() || event->altKey()). > > Please suggest. Yes please do this as you have written. In that it will allow shift to change the selection.
On 2016/04/04 19:08:26, dtapuska wrote: > On 2016/04/04 18:00:06, Paritosh Kumar wrote: > > Thanks dtapuska for your input. > > > > > https://codereview.chromium.org/1849273003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/html/forms/RadioInputType.cpp (right): > > > > > https://codereview.chromium.org/1849273003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/html/forms/RadioInputType.cpp:98: if > > (event->modifiers() & PlatformEvent::AltKey) > > > > > How does this work on Mac? I know Option left/right implements the direction > > > there. > > > > I think AltKey is same as OptionKey of Mac. > > > > > I wonder if this should be if (event->modifiers() & > > PlatformEvent:KeyModifiers) > > > instead? > > > > But, I think this will prevent user to select another radio button using arrow > > key with any one of (ctrl/shift/Fn/Os) key. > > > > I compared the behavior of different browsers,(i.e. IE and FF) > > ctrl+Left - No effect on the selection/Page Navigation > > shift/Fn + Left - Selection changes as expected. > > Alt+Left - Page Navigation. > > > > After this change we will differ with IE and FF only on ctrl+Left. > > For this, I think we should use, > > if (event->ctrlKey() || event->metaKey() || event->altKey()). > > > > Please suggest. > > Yes please do this as you have written. > In that it will allow shift to change the selection. Thanks. Update the cl. Please have a look.
On 2016/04/05 07:25:47, Paritosh Kumar wrote: > On 2016/04/04 19:08:26, dtapuska wrote: > > On 2016/04/04 18:00:06, Paritosh Kumar wrote: > > > Thanks dtapuska for your input. > > > > > > > > > https://codereview.chromium.org/1849273003/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/html/forms/RadioInputType.cpp (right): > > > > > > > > > https://codereview.chromium.org/1849273003/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/html/forms/RadioInputType.cpp:98: if > > > (event->modifiers() & PlatformEvent::AltKey) > > > > > > > How does this work on Mac? I know Option left/right implements the > direction > > > > there. > > > > > > I think AltKey is same as OptionKey of Mac. > > > > > > > I wonder if this should be if (event->modifiers() & > > > PlatformEvent:KeyModifiers) > > > > instead? > > > > > > But, I think this will prevent user to select another radio button using > arrow > > > key with any one of (ctrl/shift/Fn/Os) key. > > > > > > I compared the behavior of different browsers,(i.e. IE and FF) > > > ctrl+Left - No effect on the selection/Page Navigation > > > shift/Fn + Left - Selection changes as expected. > > > Alt+Left - Page Navigation. > > > > > > After this change we will differ with IE and FF only on ctrl+Left. > > > For this, I think we should use, > > > if (event->ctrlKey() || event->metaKey() || event->altKey()). > > > > > > Please suggest. > > > > Yes please do this as you have written. > > In that it will allow shift to change the selection. > > Thanks. Update the cl. > Please have a look. lgtm; you will need to wait for tkent@ to approve since I am not an owner.
Please add a test.
On 2016/04/07 23:04:52, tkent wrote: > Please add a test. Closing issue as landed hare
On 2016/05/06 11:09:06, Paritosh Kumar wrote: > On 2016/04/07 23:04:52, tkent wrote: > > Please add a test. > Closing issue as landed hare https://codereview.chromium.org/1944923002/ |
