Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2869713003:
Make context menu aware of hidden selection (Closed)
|
Created:
3 years, 7 months ago by hugoh_UTC2 Modified:
3 years, 7 months ago CC:
blink-reviews, chromium-reviews, dtapuska+blinkwatch_chromium.org, Navid Zolghadr Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake context menu aware of hidden selection
When the frame's selection is hidden, the context menu should
use the focused element (not the selection) as context.
BUG=715059
Expected: Context menu for <a>.
Review-Url: https://codereview.chromium.org/2869713003
Cr-Commit-Position: refs/heads/master@{#471719}
Committed: https://chromium.googlesource.com/chromium/src/+/ff7930cd4d17f38e69016d96024acadf8a008a4d
Patch Set 1 #Patch Set 2 : Added a test #
Total comments: 3
Patch Set 3 : Fix nits and style #Patch Set 4 : Add test as NeverFixTests for Mac #
Messages
Total messages: 30 (13 generated)
hugoh@opera.com changed reviewers: + bokan@chromium.org, yosin@chromium.org
PTAL This CL depends on https://codereview.chromium.org/2841093002/ .
Code change is OK. But, we need to have a test for this change. Could you add a test for this? If you can wait for patch[1], it seems build bot has trouble, you can use dependent patch feature of code review tool, see [2] for instruction, branch-off from patch[1] to this patch. See patch[3] as an example of dependent patch, which depends on the patch and some patches depend on patch[3]. [1] http://crrev.com/2841093002: Algorithm for deciding if a frame's selection should be hidden [2] http://dev.chromium.org/developers/how-tos/get-the-code/working-with-branches [3] http://crrev.com/2873483002: [DMC #5.6] Add type cast for SpellCheckMarkerListImpl
I've now added a test. PTAL at PS2 :) The test ensures we don't hit this bug again. Btw, testharness.js cannot send the ContextMenu key events, that's why the test uses the the older "eventSender".
lgtm % nit but please wait for yosin@ too https://codereview.chromium.org/2869713003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/contextmenu-follows-focus.html (right): https://codereview.chromium.org/2869713003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/contextmenu-follows-focus.html:11: <body> Nit: As per layout test style (https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_...), omit <body>
lgtm w/ nit https://codereview.chromium.org/2869713003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/contextmenu-follows-focus.html (right): https://codereview.chromium.org/2869713003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/contextmenu-follows-focus.html:18: if (!window.eventSender) { nit: We found shorter one: |assert_exists(window, eventSender)|[1] [1] http://web-platform-tests.org/writing-tests/testharness-api.html
Patchset #3 (id:40001) has been deleted
Thanks! https://codereview.chromium.org/2869713003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2869713003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1829: if (!override_target_element && start.AnchorNode() && !selection.IsHidden() && I'm wondering if we can't replace this complete statement with just: if (!override_target_element && selection.SelectionHasFocus()) - but, it feels risky to do that in this bugfix CL. I will test this locally.
The CQ bit was checked by hugoh@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2869713003/#ps60001 (title: "Fix nits and style")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hugoh@opera.com
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
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_...)
The CQ bit was checked by hugoh@opera.com
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
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_...)
Hmm, the new test fails on Mac and on Mac only... :/
On 2017/05/12 21:38:08, hugoh_UTC2 wrote: > Hmm, the new test fails on Mac and on Mac only... :/ Mac doesn't have a context menu key so it's ignored there.
On 2017/05/12 21:39:35, bokan wrote: > On 2017/05/12 21:38:08, hugoh_UTC2 wrote: > > Hmm, the new test fails on Mac and on Mac only... :/ > > Mac doesn't have a context menu key so it's ignored there. I think it makes sense to add this test to NeverFixTests for mac, there should already be tests disabled there for this reason.
The CQ bit was checked by hugoh@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2869713003/#ps80001 (title: "Add test as NeverFixTests for Mac")
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": 80001, "attempt_start_ts": 1494837277204670,
"parent_rev": "d999f0f298065398ec9bbc156c177892d74d17f6", "commit_rev":
"ff7930cd4d17f38e69016d96024acadf8a008a4d"}
Message was sent while issue was closed.
Description was changed from ========== Make context menu aware of hidden selection When the frame's selection is hidden, the context menu should use the focused element (not the selection) as context. BUG=715059 TEST= <input autofocus><a href="www">link</a> 1. Hit tab key to go focus the link. 2. Shift+F10. Expected: Context menu for <a>. ========== to ========== Make context menu aware of hidden selection When the frame's selection is hidden, the context menu should use the focused element (not the selection) as context. BUG=715059 Expected: Context menu for <a>. Review-Url: https://codereview.chromium.org/2869713003 Cr-Commit-Position: refs/heads/master@{#471719} Committed: https://chromium.googlesource.com/chromium/src/+/ff7930cd4d17f38e69016d96024a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ff7930cd4d17f38e69016d96024a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
