Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2200833007:
Change return value of FrameSelection::modify() to prevent unnecessary window scrolling (Closed)
|
Created:
4 years, 4 months ago by yosin_UTC9 Modified:
4 years, 4 months ago CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange return value of FrameSelection::modify() to prevent unnecessary window scrolling
This patch changes return value of |FrameSelection::modify()| to prevent
unnecessary window scrolling by returning true, indicating user action is
processed. Note: for spacial navigation case, |FrameSelection::modify()| returns
false to allow default key event handler of spacial navigation to move
focus to another element.
Before this patch, hitting right arrow key at end of text field causes
window scrolling by WebViewImpl::keyEventDefault() due by
|FrameSelection::modify()| returns false since selection isn't changed.
BUG=633376
TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.ModifyWithUserTriggered
Committed: https://crrev.com/8e786c79ce204adb73db01db98f5a606c8365c2a
Cr-Commit-Position: refs/heads/master@{#409995}
Patch Set 1 : 2016-08-03T16:34:59 #Patch Set 2 : 2016-08-03T18:38:52 #Patch Set 3 : 2016-08-04T10:29:12 #
Total comments: 4
Patch Set 4 : 2016-08-05T13:02:14 #
Messages
Total messages: 25 (19 generated)
Description was changed from ========== 2016-08-03T16:34:59 BUG= ========== to ========== Change return value of FrameSelection::modify() to prevent unnecessary window scrolling BUG=633376 TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.ModifyWithUserTriggered ==========
The CQ bit was checked by yosin@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Change return value of FrameSelection::modify() to prevent unnecessary window scrolling BUG=633376 TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.ModifyWithUserTriggered ========== to ========== Change return value of FrameSelection::modify() to prevent unnecessary window scrolling BUG=633376 TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.ModifyWithUserTriggered # Failed Tests - fast/spatial-navigation/snav-textarea.html - fast/spatial-navigation/snav-input.html ==========
The CQ bit was checked by yosin@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yosin@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.
Description was changed from ========== Change return value of FrameSelection::modify() to prevent unnecessary window scrolling BUG=633376 TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.ModifyWithUserTriggered # Failed Tests - fast/spatial-navigation/snav-textarea.html - fast/spatial-navigation/snav-input.html ========== to ========== Change return value of FrameSelection::modify() to prevent unnecessary window scrolling This patch changes return value of |FrameSelection::modify()| to prevent unnecessary window scrolling by returning true, indicating user action is processed. Note: for spacial navigation case, |FrameSelection::modify()| returns false to allow default key event handler of spacial navigation to move focus to another element. Before this patch, hitting right arrow key at end of text field causes window scrolling by WebViewImpl::keyEventDefault() due by |FrameSelection::modify()| returns false since selection isn't changed. BUG=633376 TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.ModifyWithUserTriggered ==========
yosin@chromium.org changed reviewers: + xiaochengh@chromium.org, yoichio@chromium.org
PTAL
lgtm with some wording suggestions https://codereview.chromium.org/2200833007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/2200833007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:189: << "Selection.modify() returns false for user triggered call when selection isn't modified."; non-user-triggered call Reference: https://en.wikipedia.org/wiki/Hyphen#Compound_modifiers https://codereview.chromium.org/2200833007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:194: << "Selection.modify() returns true for user triggered call"; ditto: user-triggered call
Thanks for reviewing! Committing... https://codereview.chromium.org/2200833007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/2200833007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:189: << "Selection.modify() returns false for user triggered call when selection isn't modified."; On 2016/08/05 at 01:51:25, Xiaocheng wrote: > non-user-triggered call > > Reference: https://en.wikipedia.org/wiki/Hyphen#Compound_modifiers Done https://codereview.chromium.org/2200833007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:194: << "Selection.modify() returns true for user triggered call"; On 2016/08/05 at 01:51:25, Xiaocheng wrote: > ditto: user-triggered call Done.
The CQ bit was checked by yosin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiaochengh@chromium.org Link to the patchset: https://codereview.chromium.org/2200833007/#ps60001 (title: "2016-08-05T13:02:14")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Change return value of FrameSelection::modify() to prevent unnecessary window scrolling This patch changes return value of |FrameSelection::modify()| to prevent unnecessary window scrolling by returning true, indicating user action is processed. Note: for spacial navigation case, |FrameSelection::modify()| returns false to allow default key event handler of spacial navigation to move focus to another element. Before this patch, hitting right arrow key at end of text field causes window scrolling by WebViewImpl::keyEventDefault() due by |FrameSelection::modify()| returns false since selection isn't changed. BUG=633376 TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.ModifyWithUserTriggered ========== to ========== Change return value of FrameSelection::modify() to prevent unnecessary window scrolling This patch changes return value of |FrameSelection::modify()| to prevent unnecessary window scrolling by returning true, indicating user action is processed. Note: for spacial navigation case, |FrameSelection::modify()| returns false to allow default key event handler of spacial navigation to move focus to another element. Before this patch, hitting right arrow key at end of text field causes window scrolling by WebViewImpl::keyEventDefault() due by |FrameSelection::modify()| returns false since selection isn't changed. BUG=633376 TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.ModifyWithUserTriggered Committed: https://crrev.com/8e786c79ce204adb73db01db98f5a606c8365c2a Cr-Commit-Position: refs/heads/master@{#409995} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8e786c79ce204adb73db01db98f5a606c8365c2a Cr-Commit-Position: refs/heads/master@{#409995} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
