Correct logic "Should ContextMenu target the selection?"

Correct logic "Should ContextMenu target the selection?"

If the selection doesn't have focus, it shouldn't be the target
of the context menu.

For example, an _unfocused_ range selection should not be the
context menu's target (the focused element should be the target).

BUG=725005, 725022
Review-Url: https://codereview.chromium.org/2880313002
Cr-Commit-Position: refs/heads/master@{#473842}
Committed: https://chromium.googlesource.com/chromium/src/+/48496741476caaedc14e16c9e4e4c90373b6e47e

[hugoh_UTC2, 2017-05-15 13:21:16]

The CQ bit was checked by hugoh@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 13:21:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 13:21:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 13:21:34]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[rune, 2017-05-15 13:24:26]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 13:24:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hugoh_UTC2, 2017-05-15 15:46:24]

Description was changed from

==========
Simplify expression "FrameSelection should be context menu's context?"

If selection doesn't have focus, it should not be the target of
the context menu. An unfocused selection should not receive input.

BUG=715059
TEST=No behavior change.
==========

to

==========
Simplify expression "FrameSelection should be context menu's context?"

If the selection doesn't have focus, it shouldn't be the target of
the context menu. An unfocused selection should not receive input.

BUG=715059
TEST=No behavior change.
==========

[commit-bot: I haz the power, 2017-05-15 18:57:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 18:57:27]

Dry run: This issue passed the CQ dry run.

[rune, 2017-05-15 21:11:08]

rune@opera.com changed reviewers:
- rune@opera.com

[hugoh_UTC2, 2017-05-16 06:48:02]

hugoh@opera.com changed reviewers:
+ yosin@chromium.org

[hugoh_UTC2, 2017-05-16 06:48:02]

yosin@ do you think this simplification is OK ?

[yosin_UTC9, 2017-05-16 09:45:22]

https://codereview.chromium.org/2880313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2880313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:1827: if
(!override_target_element && selection.SelectionHasFocus()) {
It seems |start.AnchorNode() && !selection.IsHidden()| means
selection.IsRange().
But, SelectionHasFocus() return true if start position is in focused element.

Hence, in case of caret selection in focused non-editable, this patch executes
then-clause,
where original code doesn't.

Note: this new behavior is as same as before [1] unless considering focus.

WDYT?


[1] http://crrev.com/2869713003 Make context menu aware of hidden selection

[hugoh_UTC2, 2017-05-16 12:41:43]

Description was changed from

==========
Simplify expression "FrameSelection should be context menu's context?"

If the selection doesn't have focus, it shouldn't be the target of
the context menu. An unfocused selection should not receive input.

BUG=715059
TEST=No behavior change.
==========

to

==========
Simplify expression "FrameSelection should be context menu's context?"

BUG=715059
TEST=No behavior change.
==========

[hugoh_UTC2, 2017-05-16 12:48:14]

https://codereview.chromium.org/2880313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2880313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:1827: if
(!override_target_element && selection.SelectionHasFocus()) {
On 2017/05/16 09:45:22, yosin_UTC9 wrote:
> It seems |start.AnchorNode() && !selection.IsHidden()| means
> selection.IsRange().
> But, SelectionHasFocus() return true if start position is in focused element.
> 
> Hence, in case of caret selection in focused non-editable, this patch executes
> then-clause,
> where original code doesn't.

Right! Context menu should not use a hidden caret selection as context... PS1
was too optimistic.

In PS2 I dropped the redundant start.AnchorNode() and moved the slower
IsHidden() to the end. I also moved the quick IsRange() before the slower
RootEditableElement().

[yosin_UTC9, 2017-05-17 01:54:58]

yosin@chromium.org changed reviewers:
+ xiaochengh@chromium.org

[yosin_UTC9, 2017-05-17 01:54:59]

+xiaochengh@, who also reviewed FS::IsHidden(), for second eye

https://codereview.chromium.org/2880313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2880313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1830:
(selection.ComputeVisibleSelectionInDOMTreeDeprecated().IsRange() ||
I would like to move this condition expression, except for
|!override_target_element|,
into named function, e.g. ShouldShowContextMenuAtSelection(const LocalFrame&) ->
bool.
To help to avoid to use ComputeVisibleSelectionInDOMTreeDeprecated() easier.

[Xiaocheng, 2017-05-17 02:06:27]

https://codereview.chromium.org/2880313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/2880313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1831: if
(!override_target_element && start.AnchorNode() && !selection.IsHidden() &&
The current impl is incorrect. When we have an unfocused range selection in
contenteditable, the context menu is still targeted at the selection instead of
the focused element.

We should check FS::SelectionHasFocus() here instead of FS::IsHidden().

FS::IsHidden() should only be used by paint code. That's why I insisted having
two separate functions HasFocus() and IsHidden() to serve different purposes at
the beginning, instead of one IsActive().

[hugoh_UTC2, 2017-05-17 09:02:51]

Description was changed from

==========
Simplify expression "FrameSelection should be context menu's context?"

BUG=715059
TEST=No behavior change.
==========

to

==========
Simplify expression "FrameSelection should be context menu's context?"

If the selection doesn't have focus, it shouldn't be the target of the
context menu. For example, an _unfocused_ range selection made inside a
contenteditable element should not be the context menu's target.

BUG=715059

TEST=
  1. Create a range selection in a <div contenteditable>.
  2. Let JavaScript do alink.focus().
  3. Hit "context menu" key on a PC keyboard.
  Expected: Context menu for <a> is shown.
==========

[hugoh_UTC2, 2017-05-17 09:07:47]

https://codereview.chromium.org/2880313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/2880313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1831: if
(!override_target_element && start.AnchorNode() && !selection.IsHidden() &&
On 2017/05/17 02:06:27, Xiaocheng wrote:
> The current impl is incorrect. When we have an unfocused range selection in
> contenteditable, the context menu is still targeted at the selection instead
of
> the focused element.
Thanks. You're right. I fixed this in PS3.

https://codereview.chromium.org/2880313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2880313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1830:
(selection.ComputeVisibleSelectionInDOMTreeDeprecated().IsRange() ||
On 2017/05/17 01:54:59, yosin_UTC9 wrote:
> I would like to move this condition expression, except for
> |!override_target_element|,
> into named function, e.g. ShouldShowContextMenuAtSelection(const LocalFrame&)
->
> bool.
> To help to avoid to use ComputeVisibleSelectionInDOMTreeDeprecated() easier.

Done, see PS3.

[Xiaocheng, 2017-05-17 19:20:26]

Code change looks fine. Please add a regression test for the case of unfocused
range selection in a <div contenteditable>.

https://codereview.chromium.org/2880313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2880313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1805: static bool
ShouldShowContextMenuAtSelection(const LocalFrame& frame) {
nit: I prefer passing FrameSelection to reduce a call of LocalFrame::Selection()

[yosin_UTC9, 2017-05-18 01:17:15]

https://codereview.chromium.org/2880313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2880313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1805: static bool
ShouldShowContextMenuAtSelection(const LocalFrame& frame) {
On 2017/05/17 at 19:20:26, Xiaocheng wrote:
> nit: I prefer passing FrameSelection to reduce a call of
LocalFrame::Selection()

me too.

https://codereview.chromium.org/2880313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1811: 
nit: Please remove an extra blank line.

[hugoh_UTC2, 2017-05-22 11:13:55]

Description was changed from

==========
Simplify expression "FrameSelection should be context menu's context?"

If the selection doesn't have focus, it shouldn't be the target of the
context menu. For example, an _unfocused_ range selection made inside a
contenteditable element should not be the context menu's target.

BUG=715059

TEST=
  1. Create a range selection in a <div contenteditable>.
  2. Let JavaScript do alink.focus().
  3. Hit "context menu" key on a PC keyboard.
  Expected: Context menu for <a> is shown.
==========

to

==========
Correct logic "Is the selection context menu's target?"

If the selection doesn't have focus, it shouldn't be the target of the context
menu.

For example, an _unfocused_ range selection should not be the context menu's
target.

BUG=725005, 725022
==========

[hugoh_UTC2, 2017-05-22 11:18:29]

Patchset #4 (id:60001) has been deleted

[hugoh_UTC2, 2017-05-22 12:22:55]

Patchset #4 (id:80001) has been deleted

[hugoh_UTC2, 2017-05-22 12:26:12]

PTAL

https://codereview.chromium.org/2880313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2880313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1805: static bool
ShouldShowContextMenuAtSelection(const LocalFrame& frame) {
On 2017/05/18 01:17:15, yosin_UTC9 wrote:
> On 2017/05/17 at 19:20:26, Xiaocheng wrote:
> > nit: I prefer passing FrameSelection to reduce a call of
> LocalFrame::Selection()
> 
> me too.

Done.

https://codereview.chromium.org/2880313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1811: 
On 2017/05/18 01:17:15, yosin_UTC9 wrote:
> nit: Please remove an extra blank line.

Done.

[hugoh_UTC2, 2017-05-22 12:29:45]

Description was changed from

==========
Correct logic "Is the selection context menu's target?"

If the selection doesn't have focus, it shouldn't be the target of the context
menu.

For example, an _unfocused_ range selection should not be the context menu's
target.

BUG=725005, 725022
==========

to

==========
Correct logic "Should ContextMenu target the selection?"

If the selection doesn't have focus, it shouldn't be the target
of the context menu.

For example, an _unfocused_ range selection should not be the
context menu's target (the focused element should be the target).

BUG=725005, 725022
==========

[Xiaocheng, 2017-05-22 17:50:22]

lgtm

[yosin_UTC9, 2017-05-23 01:28:23]

yosin@chromium.org changed reviewers:
+ tkent@chromium.org

[yosin_UTC9, 2017-05-23 01:28:24]

lgtm

+tkent@ for core/input/EventHandler.cpp

https://codereview.chromium.org/2880313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2880313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1806: VisibleSelection
visible_selection =
nit: s/VisibleSelection/const VisibleSelection&/

[tkent, 2017-05-23 01:48:32]

lgtm

[hugoh_UTC2, 2017-05-23 07:48:52]

The CQ bit was checked by hugoh@opera.com

[hugoh_UTC2, 2017-05-23 07:48:52]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yosin@chromium.org, xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2880313002/#ps120001
(title: "Add a const")

[commit-bot: I haz the power, 2017-05-23 07:49:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 09:47:53]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495525732203180,
"parent_rev": "3c1005eef79c0534edfec6a950839944a10054b3", "commit_rev":
"48496741476caaedc14e16c9e4e4c90373b6e47e"}

[commit-bot: I haz the power, 2017-05-23 09:48:05]

Description was changed from

==========
Correct logic "Should ContextMenu target the selection?"

If the selection doesn't have focus, it shouldn't be the target
of the context menu.

For example, an _unfocused_ range selection should not be the
context menu's target (the focused element should be the target).

BUG=725005, 725022
==========

to

==========
Correct logic "Should ContextMenu target the selection?"

If the selection doesn't have focus, it shouldn't be the target
of the context menu.

For example, an _unfocused_ range selection should not be the
context menu's target (the focused element should be the target).

BUG=725005, 725022

Review-Url: https://codereview.chromium.org/2880313002
Cr-Commit-Position: refs/heads/master@{#473842}
Committed:
https://chromium.googlesource.com/chromium/src/+/48496741476caaedc14e16c9e4e4...
==========

[commit-bot: I haz the power, 2017-05-23 09:48:06]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/48496741476caaedc14e16c9e4e4...