Algorithm for deciding if a frame's selection should be hidden

Algorithm for deciding if a frame's selection should be hidden

Background:
Crrev.com/464698 introduced "hiding" of unfocused selections
in text controls. Hiding avoids clearing the selection upon change
of focus.

Problem:
Now we only hide selections inside text controls.
Selections within content-editable elements must also be hidden.

Solution:
Generalize previous work into an algorithm that, given current
DOM and its activeElement, determines whether a frame's selection
should be hidden.

See the algorithm in InHidden() for documentation and read its
unit tests in FrameSelectionTest.cpp.

BUG=715059, 715889
Review-Url: https://codereview.chromium.org/2841093002
Cr-Commit-Position: refs/heads/master@{#470822}
Committed: https://chromium.googlesource.com/chromium/src/+/8549ed4eaf9d95db494bd7028bf17110b26b6918

[hugoh_UTC2, 2017-04-26 14:44:02]

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

[hugoh_UTC2, 2017-04-26 14:44:03]

yosin@, this is a quick fix for the given bug. I regret that Blink needs to be
aware of the selection's paint-state.

[yosin_UTC9, 2017-04-27 06:33:52]

On 2017/04/26 at 14:44:03, hugoh wrote:
> yosin@, this is a quick fix for the given bug. I regret that Blink needs to be
aware of the selection's paint-state.

The reason of regret comes from we did hack way for solving unfocused selection.

This patch doesn't work for contenteditable, e.g.
<div contenteditable>def</div><a href="www">link</a>

1. Select all in contenteditable
2. Hit Tab key
3. Ctrl+F10 to show context menu

Since, selection for contenteditable is still show == (force_hide_ == false)

To avoid using |FrameSelection::IsHidden()|, we need to have proper version of
SelectionHasFocus() which we gave up 
to implement proper version.

Current implementation used in EditorKeyBindings.cpp is
 focused_element->ContainsIncludeHostElements(selection.start)

Problem of this implementation is when BODY has focus, and selection in INPUT,
SelectionHasFocus().
To avoid this situation, we should up tree from selection start instead of down
tree to selection start
not to crossing editing boundary with special handling of shadow host.

Here is sketch:

bool FrameSelection::HasFocus() {
  const Element* const focus_element = ...
  if (!focus_element)
    return false;
  if (focus_element is INPUT or TEXTAREA)
     return focused_element->ContainsIncludeHostELements(selection.start);
  bool has_editable_style = HasEditableStyle(selection.start);
  for (runner in inclusive ancestor of selection.start) {
     if (runner == focus_element)
       return true;
     if (has_editable_style != HasEditableStyle(runner))
      return false; // we cross over editing boundary
  }
  return false;
}

We can use FrameSelection::HasFocus() instead of IsHidden() in this patch.
And, this works both INPUT/TEXTAREA and contenteditable.

Also, we can use HasFocus() to replace current hack.
WDYT?

BTW, could you add a test case for this change?

[hugoh_UTC2, 2017-04-27 08:43:40]

Patchset #2 (id:20001) has been deleted

[hugoh_UTC2, 2017-04-27 09:03:51]

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

[commit-bot: I haz the power, 2017-04-27 09:04:06]

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-04-27 09:04:06]

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

[commit-bot: I haz the power, 2017-04-27 09:04:07]

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.

[yosin_UTC9, 2017-04-27 09:43:30]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 09:43:48]

Dry run: CQ is trying da patch.

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

[yosin_UTC9, 2017-04-27 09:44:14]

On 2017/04/27 at 06:33:52, yosin_UTC9 wrote:
> On 2017/04/26 at 14:44:03, hugoh wrote:
> > yosin@, this is a quick fix for the given bug. I regret that Blink needs to
be aware of the selection's paint-state.
> 
> The reason of regret comes from we did hack way for solving unfocused
selection.
> 
> This patch doesn't work for contenteditable, e.g.
> <div contenteditable>def</div><a href="www">link</a>
> 
> 1. Select all in contenteditable
> 2. Hit Tab key
> 3. Ctrl+F10 to show context menu
> 
> Since, selection for contenteditable is still show == (force_hide_ == false)
> 
> To avoid using |FrameSelection::IsHidden()|, we need to have proper version of
SelectionHasFocus() which we gave up 
> to implement proper version.
> 
> Current implementation used in EditorKeyBindings.cpp is
>  focused_element->ContainsIncludeHostElements(selection.start)
> 
> Problem of this implementation is when BODY has focus, and selection in INPUT,
SelectionHasFocus().
> To avoid this situation, we should up tree from selection start instead of
down tree to selection start
> not to crossing editing boundary with special handling of shadow host.
> 
> Here is sketch:
> 
> bool FrameSelection::HasFocus() {
>   const Element* const focus_element = ...
>   if (!focus_element)
>     return false;
>   if (focus_element is INPUT or TEXTAREA)
>      return focused_element->ContainsIncludeHostELements(selection.start);
>   bool has_editable_style = HasEditableStyle(selection.start);
>   for (runner in inclusive ancestor of selection.start) {
>      if (runner == focus_element)
>        return true;
>      if (has_editable_style != HasEditableStyle(runner))
>       return false; // we cross over editing boundary
>   }
>   return false;
> }
> 
> We can use FrameSelection::HasFocus() instead of IsHidden() in this patch.
> And, this works both INPUT/TEXTAREA and contenteditable.
> 
> Also, we can use HasFocus() to replace current hack.
> WDYT?
> 
> BTW, could you add a test case for this change?

Let's see try bot result. I hope there are no failures... (人)

[hugoh_UTC2, 2017-04-27 10:42:52]

On 2017/04/27 09:44:14, yosin_UTC9 wrote:
> On 2017/04/27 at 06:33:52, yosin_UTC9 wrote:
> > On 2017/04/26 at 14:44:03, hugoh wrote:
> > > yosin@, this is a quick fix for the given bug. I regret that Blink needs
to
> be aware of the selection's paint-state.
> > 
> > The reason of regret comes from we did hack way for solving unfocused
> selection.
> > 
> > This patch doesn't work for contenteditable, e.g.
> > <div contenteditable>def</div><a href="www">link</a>
> > 
> > 1. Select all in contenteditable
> > 2. Hit Tab key
> > 3. Ctrl+F10 to show context menu
> > 
> > Since, selection for contenteditable is still show == (force_hide_ == false)
> > 
> > To avoid using |FrameSelection::IsHidden()|, we need to have proper version
of
> SelectionHasFocus() which we gave up 
> > to implement proper version.
> > 
> > Current implementation used in EditorKeyBindings.cpp is
> >  focused_element->ContainsIncludeHostElements(selection.start)
> > 
> > Problem of this implementation is when BODY has focus, and selection in
INPUT,
> SelectionHasFocus().
> > To avoid this situation, we should up tree from selection start instead of
> down tree to selection start
> > not to crossing editing boundary with special handling of shadow host.
> > 
> > Here is sketch:
> > 
> > bool FrameSelection::HasFocus() {
> >   const Element* const focus_element = ...
> >   if (!focus_element)
> >     return false;
> >   if (focus_element is INPUT or TEXTAREA)
> >      return focused_element->ContainsIncludeHostELements(selection.start);
> >   bool has_editable_style = HasEditableStyle(selection.start);
> >   for (runner in inclusive ancestor of selection.start) {
> >      if (runner == focus_element)
> >        return true;
> >      if (has_editable_style != HasEditableStyle(runner))
> >       return false; // we cross over editing boundary
> >   }
> >   return false;
> > }
> > 
> > We can use FrameSelection::HasFocus() instead of IsHidden() in this patch.
> > And, this works both INPUT/TEXTAREA and contenteditable.
> > 
> > Also, we can use HasFocus() to replace current hack.
> > WDYT?
> > 
> > BTW, could you add a test case for this change?
> 
> Let's see try bot result. I hope there are no failures... (人)

Thanks for triggering the bots.
I know that at least mixed-editability-2.html will fail. I fixed it in PS3.

However, when I wrap mixed-editability-2.html inside yet another contenteditable
<div> the test fails so I believe HasFocus() needs more tweaking.

<body>
<div contenteditable="true"> <!-- added -->
  <div contenteditable="true" id="parent">
  OnlyThe<span id="base" style="font-weight:bold;"
contenteditable="false">Bold</span>TextShould<span id="extent">Be</span>Selected
  </div>
</div>
<script>
document.getElementById("parent").focus();
var s = window.getSelection();
var base = document.getElementById("base");
var extent = document.getElementById("extent");
s.setBaseAndExtent(base, 0, extent, 0);
</script>
</body>

[yosin_UTC9, 2017-04-27 12:36:39]

On 2017/04/27 at 10:42:52, hugoh wrote:
> On 2017/04/27 09:44:14, yosin_UTC9 wrote:
> > On 2017/04/27 at 06:33:52, yosin_UTC9 wrote:
> > > On 2017/04/26 at 14:44:03, hugoh wrote:
> > > > yosin@, this is a quick fix for the given bug. I regret that Blink needs
to
> > be aware of the selection's paint-state.
> > > 
> > > The reason of regret comes from we did hack way for solving unfocused
> > selection.
> > > 
> > > This patch doesn't work for contenteditable, e.g.
> > > <div contenteditable>def</div><a href="www">link</a>
> > > 
> > > 1. Select all in contenteditable
> > > 2. Hit Tab key
> > > 3. Ctrl+F10 to show context menu
> > > 
> > > Since, selection for contenteditable is still show == (force_hide_ ==
false)
> > > 
> > > To avoid using |FrameSelection::IsHidden()|, we need to have proper
version of
> > SelectionHasFocus() which we gave up 
> > > to implement proper version.
> > > 
> > > Current implementation used in EditorKeyBindings.cpp is
> > >  focused_element->ContainsIncludeHostElements(selection.start)
> > > 
> > > Problem of this implementation is when BODY has focus, and selection in
INPUT,
> > SelectionHasFocus().
> > > To avoid this situation, we should up tree from selection start instead of
> > down tree to selection start
> > > not to crossing editing boundary with special handling of shadow host.
> > > 
> > > Here is sketch:
> > > 
> > > bool FrameSelection::HasFocus() {
> > >   const Element* const focus_element = ...
> > >   if (!focus_element)
> > >     return false;
> > >   if (focus_element is INPUT or TEXTAREA)
> > >      return focused_element->ContainsIncludeHostELements(selection.start);
> > >   bool has_editable_style = HasEditableStyle(selection.start);
> > >   for (runner in inclusive ancestor of selection.start) {
> > >      if (runner == focus_element)
> > >        return true;
> > >      if (has_editable_style != HasEditableStyle(runner))
> > >       return false; // we cross over editing boundary
> > >   }
> > >   return false;
> > > }
> > > 
> > > We can use FrameSelection::HasFocus() instead of IsHidden() in this patch.
> > > And, this works both INPUT/TEXTAREA and contenteditable.
> > > 
> > > Also, we can use HasFocus() to replace current hack.
> > > WDYT?
> > > 
> > > BTW, could you add a test case for this change?
> > 
> > Let's see try bot result. I hope there are no failures... (人)
> 
> Thanks for triggering the bots.
> I know that at least mixed-editability-2.html will fail. I fixed it in PS3.
> 
> However, when I wrap mixed-editability-2.html inside yet another
contenteditable <div> the test fails so I believe HasFocus() needs more
tweaking.
> 
> <body>
> <div contenteditable="true"> <!-- added -->
>   <div contenteditable="true" id="parent">
>   OnlyThe<span id="base" style="font-weight:bold;"
contenteditable="false">Bold</span>TextShould<span id="extent">Be</span>Selected
>   </div>
> </div>
> <script>
> document.getElementById("parent").focus();
> var s = window.getSelection();
> var base = document.getElementById("base");
> var extent = document.getElementById("extent");
> s.setBaseAndExtent(base, 0, extent, 0);
> </script>
> </body>

Yes, we need to handle *range* selection contains focused element.
We saw this case Google internal crash log page, click to select stack trace and
copy them into clipboard.
Sorry, I forgot the bug number, but we forgot to support that in the patch
before revert for this reason.

[hugoh_UTC2, 2017-04-27 19:15:41]

On 2017/04/27 12:36:39, yosin_UTC9 wrote:
> On 2017/04/27 at 10:42:52, hugoh wrote:
> > On 2017/04/27 09:44:14, yosin_UTC9 wrote:
> > > On 2017/04/27 at 06:33:52, yosin_UTC9 wrote:
> > > > On 2017/04/26 at 14:44:03, hugoh wrote:
> > > > > yosin@, this is a quick fix for the given bug. I regret that Blink
needs
> to
> > > be aware of the selection's paint-state.
> > > > 
> > > > The reason of regret comes from we did hack way for solving unfocused
> > > selection.
> > > > 
> > > > This patch doesn't work for contenteditable, e.g.
> > > > <div contenteditable>def</div><a href="www">link</a>
> > > > 
> > > > 1. Select all in contenteditable
> > > > 2. Hit Tab key
> > > > 3. Ctrl+F10 to show context menu
> > > > 
> > > > Since, selection for contenteditable is still show == (force_hide_ ==
> false)
> > > > 
> > > > To avoid using |FrameSelection::IsHidden()|, we need to have proper
> version of
> > > SelectionHasFocus() which we gave up 
> > > > to implement proper version.
> > > > 
> > > > Current implementation used in EditorKeyBindings.cpp is
> > > >  focused_element->ContainsIncludeHostElements(selection.start)
> > > > 
> > > > Problem of this implementation is when BODY has focus, and selection in
> INPUT,
> > > SelectionHasFocus().
> > > > To avoid this situation, we should up tree from selection start instead
of
> > > down tree to selection start
> > > > not to crossing editing boundary with special handling of shadow host.
> > > > 
> > > > Here is sketch:
> > > > 
> > > > bool FrameSelection::HasFocus() {
> > > >   const Element* const focus_element = ...
> > > >   if (!focus_element)
> > > >     return false;
> > > >   if (focus_element is INPUT or TEXTAREA)
> > > >      return
focused_element->ContainsIncludeHostELements(selection.start);
> > > >   bool has_editable_style = HasEditableStyle(selection.start);
> > > >   for (runner in inclusive ancestor of selection.start) {
> > > >      if (runner == focus_element)
> > > >        return true;
> > > >      if (has_editable_style != HasEditableStyle(runner))
> > > >       return false; // we cross over editing boundary
> > > >   }
> > > >   return false;
> > > > }
> > > > 
> > > > We can use FrameSelection::HasFocus() instead of IsHidden() in this
patch.
> > > > And, this works both INPUT/TEXTAREA and contenteditable.
> > > > 
> > > > Also, we can use HasFocus() to replace current hack.
> > > > WDYT?
> > > > 
> > > > BTW, could you add a test case for this change?
> > > 
> > > Let's see try bot result. I hope there are no failures... (人)
> > 
> > Thanks for triggering the bots.
> > I know that at least mixed-editability-2.html will fail. I fixed it in PS3.
> > 
> > However, when I wrap mixed-editability-2.html inside yet another
> contenteditable <div> the test fails so I believe HasFocus() needs more
> tweaking.
> > 
> > <body>
> > <div contenteditable="true"> <!-- added -->
> >   <div contenteditable="true" id="parent">
> >   OnlyThe<span id="base" style="font-weight:bold;"
> contenteditable="false">Bold</span>TextShould<span
id="extent">Be</span>Selected
> >   </div>
> > </div>
> > <script>
> > document.getElementById("parent").focus();
> > var s = window.getSelection();
> > var base = document.getElementById("base");
> > var extent = document.getElementById("extent");
> > s.setBaseAndExtent(base, 0, extent, 0);
> > </script>
> > </body>
> 
> Yes, we need to handle *range* selection contains focused element.
> We saw this case Google internal crash log page, click to select stack trace
and
> copy them into clipboard.
> Sorry, I forgot the bug number, but we forgot to support that in the patch
> before revert for this reason.

I wrote 8 new unit tests to cover scenarios where we do/do not expect a hidden
selection, PTAL at FrameSelectionTest.cpp. To cover the new tests I rewrote
FrameSelection::HasFocus(). After the rewrite it felt more natural to call it
FrameSelection::IsHidden().

Could you trigger the bots again? Or give me access so I can trigger the bots
myself? :)

[yosin_UTC9, 2017-04-28 01:11:34]

On 2017/04/27 at 19:15:41, hugoh wrote:
> On 2017/04/27 12:36:39, yosin_UTC9 wrote:
> > On 2017/04/27 at 10:42:52, hugoh wrote:
> > > On 2017/04/27 09:44:14, yosin_UTC9 wrote:
> > > > On 2017/04/27 at 06:33:52, yosin_UTC9 wrote:
> > > > > On 2017/04/26 at 14:44:03, hugoh wrote:
> > > > > > yosin@, this is a quick fix for the given bug. I regret that Blink
needs
> > to
> > > > be aware of the selection's paint-state.
> > > > > 
> > > > > The reason of regret comes from we did hack way for solving unfocused
> > > > selection.
> > > > > 
> > > > > This patch doesn't work for contenteditable, e.g.
> > > > > <div contenteditable>def</div><a href="www">link</a>
> > > > > 
> > > > > 1. Select all in contenteditable
> > > > > 2. Hit Tab key
> > > > > 3. Ctrl+F10 to show context menu
> > > > > 
> > > > > Since, selection for contenteditable is still show == (force_hide_ ==
> > false)
> > > > > 
> > > > > To avoid using |FrameSelection::IsHidden()|, we need to have proper
> > version of
> > > > SelectionHasFocus() which we gave up 
> > > > > to implement proper version.
> > > > > 
> > > > > Current implementation used in EditorKeyBindings.cpp is
> > > > >  focused_element->ContainsIncludeHostElements(selection.start)
> > > > > 
> > > > > Problem of this implementation is when BODY has focus, and selection
in
> > INPUT,
> > > > SelectionHasFocus().
> > > > > To avoid this situation, we should up tree from selection start
instead of
> > > > down tree to selection start
> > > > > not to crossing editing boundary with special handling of shadow host.
> > > > > 
> > > > > Here is sketch:
> > > > > 
> > > > > bool FrameSelection::HasFocus() {
> > > > >   const Element* const focus_element = ...
> > > > >   if (!focus_element)
> > > > >     return false;
> > > > >   if (focus_element is INPUT or TEXTAREA)
> > > > >      return
focused_element->ContainsIncludeHostELements(selection.start);
> > > > >   bool has_editable_style = HasEditableStyle(selection.start);
> > > > >   for (runner in inclusive ancestor of selection.start) {
> > > > >      if (runner == focus_element)
> > > > >        return true;
> > > > >      if (has_editable_style != HasEditableStyle(runner))
> > > > >       return false; // we cross over editing boundary
> > > > >   }
> > > > >   return false;
> > > > > }
> > > > > 
> > > > > We can use FrameSelection::HasFocus() instead of IsHidden() in this
patch.
> > > > > And, this works both INPUT/TEXTAREA and contenteditable.
> > > > > 
> > > > > Also, we can use HasFocus() to replace current hack.
> > > > > WDYT?
> > > > > 
> > > > > BTW, could you add a test case for this change?
> > > > 
> > > > Let's see try bot result. I hope there are no failures... (人)
> > > 
> > > Thanks for triggering the bots.
> > > I know that at least mixed-editability-2.html will fail. I fixed it in
PS3.
> > > 
> > > However, when I wrap mixed-editability-2.html inside yet another
> > contenteditable <div> the test fails so I believe HasFocus() needs more
> > tweaking.
> > > 
> > > <body>
> > > <div contenteditable="true"> <!-- added -->
> > >   <div contenteditable="true" id="parent">
> > >   OnlyThe<span id="base" style="font-weight:bold;"
> > contenteditable="false">Bold</span>TextShould<span
id="extent">Be</span>Selected
> > >   </div>
> > > </div>
> > > <script>
> > > document.getElementById("parent").focus();
> > > var s = window.getSelection();
> > > var base = document.getElementById("base");
> > > var extent = document.getElementById("extent");
> > > s.setBaseAndExtent(base, 0, extent, 0);
> > > </script>
> > > </body>
> > 
> > Yes, we need to handle *range* selection contains focused element.
> > We saw this case Google internal crash log page, click to select stack trace
and
> > copy them into clipboard.
> > Sorry, I forgot the bug number, but we forgot to support that in the patch
> > before revert for this reason.
> 
> I wrote 8 new unit tests to cover scenarios where we do/do not expect a hidden
selection, PTAL at FrameSelectionTest.cpp. To cover the new tests I rewrote
FrameSelection::HasFocus(). After the rewrite it felt more natural to call it
FrameSelection::IsHidden().
> 
> Could you trigger the bots again? 

Kick trybots, and applying patch failed... (T_T)

> Or give me access so I can trigger the bots myself? :)
I sent guide for getting trybot access.

[yosin_UTC9, 2017-04-28 01:14:33]

https://codereview.chromium.org/2841093002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:467: return false;
We also need to handle Range selection case.
We should paint range selection if focused element in range, e.g.
selection.start <= focused_element < selection.end.

[hugoh_UTC2, 2017-04-28 06:55:12]

https://codereview.chromium.org/2841093002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:467: return false;
On 2017/04/28 01:14:33, yosin_OOO_til_May_7 wrote:
> We also need to handle Range selection case.
> We should paint range selection if focused element in range, e.g.
> selection.start <= focused_element < selection.end.

The test I made for that case, see TEST_F(FrameSelectionTest,
RangeContainsFocus), passes. Is that test OK?

[yosin_UTC9, 2017-04-28 07:57:08]

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

[yosin_UTC9, 2017-04-28 07:57:09]

+tkent@, since I'm unavailable until May 8.

I think it is better to split this patch into two patches:
1. Introduce FrameSelection::HasFocus()
2. EventHandler changes for context menu

Because, 1 and 2 are mutuality exclusive and CL[1] and bug[2] requires
FrameSelection::HasFocus().


[1] http://crrev.com/2847663002: Don't show touch handles when selection hidden

https://codereview.chromium.org/2841093002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:428: bool
FrameSelection::IsHidden() {
I would like to call this function as |HasFocus()| or |IsActive()|.
Because of even if we don't paint selection, selection is still available.

https://codereview.chromium.org/2841093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:467: return false;
On 2017/04/28 at 06:55:12, hugoh_UTC2 wrote:
> On 2017/04/28 01:14:33, yosin_OOO_til_May_7 wrote:
> > We also need to handle Range selection case.
> > We should paint range selection if focused element in range, e.g.
> > selection.start <= focused_element < selection.end.
> 
> The test I made for that case, see TEST_F(FrameSelectionTest,
RangeContainsFocus), passes. Is that test OK?

Yes, it is what I expected.

[yosin_UTC9, 2017-04-28 09:02:48]

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

[hugoh_UTC2, 2017-04-28 15:53:30]

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

==========
Algorithm for deciding if a frame's selection is active

Background:
Crrev.com/464698 introduced "hiding" of unfocused selections
in text controls. Hiding avoids clearing the selection upon change
of focus.

Problem:
Now we only hide selections inside text controls.
Selections within content-editable elements must also be hidden.

Solution:
Generalize previous work into an algorithm that, given current
DOM and its activeElement, determines whether a frame's selection
is active. If the selection isn't active, we hide it.

When is a selection active? See the algorithm in IsActive()
for documentation and read its unit tests in FrameSelectionTest.

BUG=715889
==========

[Mostyn Bramley-Moore, 2017-05-02 10:44:40]

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

[commit-bot: I haz the power, 2017-05-02 10:44:53]

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-02 10:50:22]

xiaocheng@, if you have time, could you PTAL at this CL? I understand you are
stand-in for yosin@ during Golden Week.

[commit-bot: I haz the power, 2017-05-02 12:24:51]

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

[commit-bot: I haz the power, 2017-05-02 12:24:52]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-05-02 20:05:17]

Sorry for the delay.

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:436: const Element*
const focus_element = GetDocument().FocusedElement()
nit: s/focus_element/focused_element/g

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:440: return false;
Does LayoutTests/editing/selection/selection-crash.html reach here? If so,
please state that here.

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:447: do {
nit: A |for| loop seems to have better readability:

for (const Node* current = start; current; current =
current->ParentOrShadowHostNode()) {
  ...
}

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:448: if
(has_editable_style && !HasEditableStyle(*current)) {
I can't understand this loop body. There seem to be multiple factors mixed
together controlling the output:

1) Whether the selection is in text control or not
2) Whether the selection is editable or not
3) Whether the focused element |IsShadowInclduingInclusiveAncestorOf| the
selection
4) Whether the ancestor or shadow host chain of the selection (bounded by the
focused element if it's in the chain) crosses editing boundary

Can some checks be moved out of the loop, so that the loop body becomes lighter
and the relation between the factors and the output becomes clearer?

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.h (right):

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.h:133: bool IsActive()
const;
"Active" is a heavily overloaded term.

Judging from the unit tests related to range selection in contenteditable div,
it seems that this function only determines if the selection (caret or range)
should be painted or not. If so, could you make it clear?

[Xiaocheng, 2017-05-02 23:15:29]

I also observe a different behavior between this patch and Stable (M58):

1. Create a range selection in a disabled/readonly text control
2. Focus a button

M58: The selection is cleared, and hence, not painted
This patch: The selection is still painted

[hugoh_UTC2, 2017-05-03 07:18:55]

Thanks!

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:436: const Element*
const focus_element = GetDocument().FocusedElement()
On 2017/05/02 20:05:16, Xiaocheng wrote:
> nit: s/focus_element/focused_element/g

OK, I will change this.

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:440: return false;
On 2017/05/02 20:05:17, Xiaocheng wrote:
> Does LayoutTests/editing/selection/selection-crash.html reach here? If so,
> please state that here.

selection-crash.html doesn't crash right now but so far IsActive is only called
from LayoutSelection. Once others start to call IsActive, we would definitely
need this safety.

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:447: do {
On 2017/05/02 20:05:16, Xiaocheng wrote:
> nit: A |for| loop seems to have better readability:
> 
> for (const Node* current = start; current; current =
> current->ParentOrShadowHostNode()) {
>   ...
> }

Not sure I agree :) A for-loop would check |current| one extra time (we already
know it is true in the first round).

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:448: if
(has_editable_style && !HasEditableStyle(*current)) {
On 2017/05/02 20:05:16, Xiaocheng wrote:
> I can't understand this loop body. There seem to be multiple factors mixed
> together controlling the output:
> 
> 1) Whether the selection is in text control or not
> 2) Whether the selection is editable or not
> 3) Whether the focused element |IsShadowInclduingInclusiveAncestorOf| the
> selection
> 4) Whether the ancestor or shadow host chain of the selection (bounded by the
> focused element if it's in the chain) crosses editing boundary

Yes, we have many cases to think of. This logic is the simplest I could come up
with that covers them all.

> 
> Can some checks be moved out of the loop, so that the loop body becomes
lighter
> and the relation between the factors and the output becomes clearer?

Hmm. The reason for having the checks inside the loop body is that I wanna walk
the chain of parents only once.

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.h (right):

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.h:133: bool IsActive()
const;
On 2017/05/02 20:05:17, Xiaocheng wrote:
> "Active" is a heavily overloaded term.
> 
> Judging from the unit tests related to range selection in contenteditable div,
> it seems that this function only determines if the selection (caret or range)
> should be painted or not. If so, could you make it clear?

It is not only used for deciding if a selection is painted or not. As yosin@
pointed out, "even if we don't paint selection, selection is still available".

[Xiaocheng, 2017-05-03 15:03:01]

Sorry I'm still confused by the purpose of this patch. The meaning of "active"
isn't clear to me.

Is it just an alternative implementation of the "hiding" semantics introduced by
crrev.com/464698? If not, what are the other purposes?

Does this patch introduce different behavior compared to the current ToT? If so,
it should be split into two patch --- one for refactoring, and the other for
behavior change.

Could you also clarify about the different behaviors in #29?

Thanks!

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:447: do {
On 2017/05/03 at 07:18:55, hugoh_UTC2 wrote:
> On 2017/05/02 20:05:16, Xiaocheng wrote:
> > nit: A |for| loop seems to have better readability:
> > 
> > for (const Node* current = start; current; current =
> > current->ParentOrShadowHostNode()) {
> >   ...
> > }
> 
> Not sure I agree :) A for-loop would check |current| one extra time (we
already know it is true in the first round).

Compared to the loop body, an extra nullptr check should have almost no cost.

Anyway, this is minor.

[Xiaocheng, 2017-05-03 16:25:28]

Instead of an ambiguous "IsActive", I think we should have two new functions to
control different things.

1. FrameSelection::HasFocus. This function decides if the selection should
interact with user input (keyboard, mouse press, context menu, drag&drop, ...)
2. FrameSelection::IsHidden. This function controls whether the selection should
be painted or not.

The "IsActive" introduced in this patch seems to be a mix of these two.

Landing plan: at the first stage, these two functions should be added by
refactoring existing code without any behavior change:
- FS::HasFocus should use code moved from EditorKeyBindings.cpp
- FS::IsHidden should use code moved from FS::DidChangeFocus

Then we can start to refine the behaviors of these two functions.

[hugoh_UTC2, 2017-05-03 20:31:00]

On 2017/05/03, Xiaocheng wrote:
> I also observe a different behavior between this patch and Stable (M58):
> 1. Create a range selection in a disabled/readonly text control
> 2. Focus a button
> M58: The selection is cleared, and hence, not painted
> This patch: The selection is still painted

Thanks for catching this. I fixed this in PS7. (Needs a test).

On 2017/05/03, Xiaocheng wrote:
> Is it just an alternative implementation of the "hiding" semantics introduced
by
> crrev.com/464698? 

This is not a pure refactoring. With this new semantics, we also cover
contenteditable. ToT is broken so we can't just refactor it. We need this new
semantics to fix bug 715059 and others. 

On 2017/05/03 16:25:28, Xiaocheng wrote:
> Instead of an ambiguous "IsActive", I think we should have two new functions
to
> control different things.
> 
> 1. FrameSelection::HasFocus. This function decides if the selection should
> interact with user input (keyboard, mouse press, context menu, drag&drop, ...)
> 2. FrameSelection::IsHidden. This function controls whether the selection
should
> be painted or not.
> 
> The "IsActive" introduced in this patch seems to be a mix of these two.
> 
> Landing plan: at the first stage, these two functions should be added by
> refactoring existing code without any behavior change:
> - FS::HasFocus should use code moved from EditorKeyBindings.cpp
> - FS::IsHidden should use code moved from FS::DidChangeFocus
> 
> Then we can start to refine the behaviors of these two functions.

It is true that IsActive is a mix of both. Now, I think we only need one
function; if the selection is "active", that means it can *potentially* be
interacted with and *potentially* be painted. I see logic that decides if a
selection really can receive user interaction already spread out, for example to
EventHandler.cpp and to FrameView.cpp. I don't wanna make logical changes in
those places at this point. But long term, it sounds like a really good idea to
split the concepts. Right now, I just wanna fixup crrev.com/464698. Later, once
we have a green ToT, we could look into refactoring.

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:448: if
(has_editable_style && !HasEditableStyle(*current)) {
On 2017/05/03 07:18:55, hugoh_UTC2 wrote:
> On 2017/05/02 20:05:16, Xiaocheng wrote:
> > I can't understand this loop body. There seem to be multiple factors mixed
> > together controlling the output:
> > 
> > 1) Whether the selection is in text control or not
> > 2) Whether the selection is editable or not
> > 3) Whether the focused element |IsShadowInclduingInclusiveAncestorOf| the
> > selection
> > 4) Whether the ancestor or shadow host chain of the selection (bounded by
the
> > focused element if it's in the chain) crosses editing boundary
> 
> Yes, we have many cases to think of. This logic is the simplest I could come
up
> with that covers them all.
> 
> > 
> > Can some checks be moved out of the loop, so that the loop body becomes
> lighter
> > and the relation between the factors and the output becomes clearer?
> 
> Hmm. The reason for having the checks inside the loop body is that I wanna
walk
> the chain of parents only once.

I realized that EnclosingTextControl() and the check for text control could be
moved outside the loop body. See PS7.

[Xiaocheng, 2017-05-03 23:45:36]

Thanks for the update. I'm still skeptical about the semantics of IsActive(),
though...

The reason why we are working on this patch is that we need to clean up the
dirty hack introduced in crrev.com/464698. However, it is still a dirty hack to
introduce a mixed function with unclear semantics and apply it at multiple
places controlling different behaviors. As long as we introduce a hack, we have
to clean it up some day. So we can't really clean up a hack with another hack,
right?

Though sharing some similarities, IsHidden() and HasFocus() are different things
for different purposes, so we shouldn't make them mixed together at the
beginning.

For this patch, since IsActive() is only used by LayoutSelection, we should just
name it IsHidden so that its semantics are clear, and this patch can count as a
proper cleanup for crrev.com/464698.

We should introduce another function HasFocus() to handle user interaction. The
editing team can do it if you don't have the cycle :)

Btw, the impl of IsHidden seems fairly simple:

bool FrameSelection::IsHidden() const {
  if (selection is not range)
    return true;
  if (selection is not in text control)
    return false;
  return (the text control doesn't have focus);
}

ps. Not sure if we need special handling for disabled text controls... Chrome
allows selecting content of a disabled text control, which doesn't seem to
follow the spec. Firefox doesn't allow selecting content of a disabled text
control.

[hugoh_UTC2, 2017-05-04 12:33:40]

Patchset #8 (id:160001) has been deleted

[hugoh_UTC2, 2017-05-04 12:44:21]

On 2017/05/03 23:45:36, Xiaocheng wrote:
> For this patch, since IsActive() is only used by LayoutSelection, we should
just
> name it IsHidden so that its semantics are clear, and this patch can count as
a
> proper cleanup for crrev.com/464698.

I agree. I did that in PS4 but then yosin@ suggested (2017-04-28 07:57:09 UTC)
to call it IsActive instead. One point is that just because IsHidden == false,
that doesn't necessarily mean that the selection is painted.

> Btw, the impl of IsHidden seems fairly simple:
> 
> bool FrameSelection::IsHidden() const {
>   if (selection is not range)
>     return true;
>   if (selection is not in text control)
>     return false;
>   return (the text control doesn't have focus);
> }
> 

Above logic hides carets. We don't wanna hide this caret: <div
contenteditable>Hey|</div>. There are many cases to think of... I wrote the 19
new tests so I can keep track of the cases I have found so far.

> ps. Not sure if we need special handling for disabled text controls... Chrome
> allows selecting content of a disabled text control, which doesn't seem to
> follow the spec. Firefox doesn't allow selecting content of a disabled text
> control.

Many Chrome users depend on this, see
https://productforums.google.com/forum/#!topic/chrome/XvWpEBPFfj8 , so let's
keep it for now. I don't wanna introduce any visual differences to current
stable. In PS8 I added two tests that cover this.

[hugoh_UTC2, 2017-05-04 13:07:16]

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

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

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-04 13:07:24]

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

[commit-bot: I haz the power, 2017-05-04 13:07:25]

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.

[Mostyn Bramley-Moore, 2017-05-04 13:08:27]

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

[commit-bot: I haz the power, 2017-05-04 13:08:42]

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-04 14:14:28]

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

[commit-bot: I haz the power, 2017-05-04 14:14:29]

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_...)

[Mostyn Bramley-Moore, 2017-05-04 16:36:36]

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

[commit-bot: I haz the power, 2017-05-04 16:37:05]

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-04 18:02:59]

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

[commit-bot: I haz the power, 2017-05-04 18:03:00]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Xiaocheng, 2017-05-04 18:24:12]

On 2017/05/04 at 12:44:21, hugoh wrote:
> On 2017/05/03 23:45:36, Xiaocheng wrote:
> > For this patch, since IsActive() is only used by LayoutSelection, we should
just
> > name it IsHidden so that its semantics are clear, and this patch can count
as a
> > proper cleanup for crrev.com/464698.
> 
> I agree. I did that in PS4 but then yosin@ suggested (2017-04-28 07:57:09 UTC)
to call it IsActive instead. One point is that just because IsHidden == false,
that doesn't necessarily mean that the selection is painted.

That's because HasFocus() and IsHidden() are different things, and should be
controlled by different logic at the beginning. "Fixing" them with a mixed
function is going to introduce more cleanup work later.

Please make this patch clean such that it:
- Either cleanup the "hidden" logic introduced in LayoutSelection
- Or add FrameSelection::HasFocus() for fixing those bugs that assume coupling
between selection and focus. 

> 
> > Btw, the impl of IsHidden seems fairly simple:
> > 
> > bool FrameSelection::IsHidden() const {
> >   if (selection is not range)
> >     return true;
> >   if (selection is not in text control)
> >     return false;
> >   return (the text control doesn't have focus);
> > }
> > 
> 
> Above logic hides carets. We don't wanna hide this caret: <div
contenteditable>Hey|</div>. There are many cases to think of... I wrote the 19
new tests so I can keep track of the cases I have found so far.

LayoutSelection::Commit doesn't paint caret selection. Caret painting is done by
CaretDisplayItemClient::PaintCaret.

> 
> > ps. Not sure if we need special handling for disabled text controls...
Chrome
> > allows selecting content of a disabled text control, which doesn't seem to
> > follow the spec. Firefox doesn't allow selecting content of a disabled text
> > control.
> 
> Many Chrome users depend on this, see
https://productforums.google.com/forum/#!topic/chrome/XvWpEBPFfj8 , so let's
keep it for now. I don't wanna introduce any visual differences to current
stable. In PS8 I added two tests that cover this.

OK. It's reasonable to keep the current behavior.

[hugoh_UTC2, 2017-05-05 15:18:58]

Description was changed from

==========
Algorithm for deciding if a frame's selection is active

Background:
Crrev.com/464698 introduced "hiding" of unfocused selections
in text controls. Hiding avoids clearing the selection upon change
of focus.

Problem:
Now we only hide selections inside text controls.
Selections within content-editable elements must also be hidden.

Solution:
Generalize previous work into an algorithm that, given current
DOM and its activeElement, determines whether a frame's selection
is active. If the selection isn't active, we hide it.

When is a selection active? See the algorithm in IsActive()
for documentation and read its unit tests in FrameSelectionTest.

BUG=715889
==========

to

==========
Algorithm for deciding if a frame's selection should be hidden

Background:
Crrev.com/464698 introduced "hiding" of unfocused selections
in text controls. Hiding avoids clearing the selection upon change
of focus.

Problem:
Now we only hide selections inside text controls.
Selections within content-editable elements must also be hidden.

Solution:
Generalize previous work into an algorithm that, given current
DOM and its activeElement, determines whether a frame's selection
should be hidden.

See the algorithm in InHidden() for documentation and read its
unit tests in FrameSelectionTest.

BUG=715889
==========

[hugoh_UTC2, 2017-05-05 15:31:35]

On 2017/05/04 18:24:12, Xiaocheng wrote:
> On 2017/05/04 at 12:44:21, hugoh wrote:
> > On 2017/05/03 23:45:36, Xiaocheng wrote:
> > > For this patch, since IsActive() is only used by LayoutSelection, we
should
> just
> > > name it IsHidden so that its semantics are clear, and this patch can count
> as a
> > > proper cleanup for crrev.com/464698.
> > 
> > I agree. I did that in PS4 but then yosin@ suggested (2017-04-28 07:57:09
UTC)
> to call it IsActive instead. One point is that just because IsHidden == false,
> that doesn't necessarily mean that the selection is painted.
> 
> That's because HasFocus() and IsHidden() are different things, and should be
> controlled by different logic at the beginning. "Fixing" them with a mixed
> function is going to introduce more cleanup work later.

To decide if a selection "is hidden", we need to take focus into account. We
could factor out HasFocus() but then IsHidden() must use HasFocus(). I tried
this in PS10. WDYT? I put HasFocus() as a private method since no one is using
it yet.

If we go with HasFocus and IsHidden, let's rename SetFocused to
SetFrameIsFocused and IsFocused to FrameIsFocused to make FrameSelection's API
clearer? I can do that in a separate commit.

[Xiaocheng, 2017-05-05 18:13:35]

On 2017/05/05 at 15:31:35, hugoh wrote:
> On 2017/05/04 18:24:12, Xiaocheng wrote:
> > On 2017/05/04 at 12:44:21, hugoh wrote:
> > > On 2017/05/03 23:45:36, Xiaocheng wrote:
> > > > For this patch, since IsActive() is only used by LayoutSelection, we
should
> > just
> > > > name it IsHidden so that its semantics are clear, and this patch can
count
> > as a
> > > > proper cleanup for crrev.com/464698.
> > > 
> > > I agree. I did that in PS4 but then yosin@ suggested (2017-04-28 07:57:09
UTC)
> > to call it IsActive instead. One point is that just because IsHidden ==
false,
> > that doesn't necessarily mean that the selection is painted.
> > 
> > That's because HasFocus() and IsHidden() are different things, and should be
> > controlled by different logic at the beginning. "Fixing" them with a mixed
> > function is going to introduce more cleanup work later.
> 
> To decide if a selection "is hidden", we need to take focus into account. We
could factor out HasFocus() but then IsHidden() must use HasFocus(). I tried
this in PS10. WDYT?

I like the version in PS10. Thanks for making all these changes!

> I put HasFocus() as a private method since no one is using it yet.

You didn't. I prefer making it public to reduce work when we need to apply it to
fix other bugs.

> 
> If we go with HasFocus and IsHidden, let's rename SetFocused to
SetFrameIsFocused and IsFocused to FrameIsFocused to make FrameSelection's API
clearer? I can do that in a separate commit.

Agreed. We should make the function names non-ambiguous. And
- FS::HasFocus should be renamed as FS::SelectionHasFocus
- FS::IsFocusedAndActive should also be renamed as FS::FrameIsFocusedAndActive

Plese use FS::SelectionHasFocus in this patch, and add TODO(editing-dev) for the
other functions. Or TODO(hugoh) if you would like to take it :)

[Xiaocheng, 2017-05-05 18:32:02]

Whoops, forgot to publish drafts.

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:352:
TEST_F(FrameSelectionTest, CaretInShadowTree) {
Let's have two test cases:
- CaretInTextControl
- CaretInShadowTree (that is not a UA shadow tree under text control)

The reason is that we treat text controls and other shadow trees differently.

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:370:
TEST_F(FrameSelectionTest, RangeInShadowTree) {
Ditto.

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:478: // We use a
double click to create the selection [Berlin].
We can use FrameSelection::SetSelection to set selection in disabled text
control:

Element* inner_editor = ToTextControlElement(textarea)->InnerEditorElement();
Selection().SetSelection(SelectionInDOMTree::Builder().SelectAllChildren(*inner_editor).Build());

[Mostyn Bramley-Moore, 2017-05-07 21:42:39]

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

[commit-bot: I haz the power, 2017-05-07 21:42:46]

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-07 22:01:37]

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

[commit-bot: I haz the power, 2017-05-07 22:01:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yosin_UTC9, 2017-05-08 05:36:53]

Good shape. (^_^)b

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:429: const Node*
current = ComputeVisibleSelectionInDOMTreeDeprecated()
nit: Please use |ComputeVisibleSelectionInDOMTree()| instead of deprecated
version.

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:465: const Node* start
= ComputeVisibleSelectionInDOMTreeDeprecated()
nit: Please use |ComputeVisibleSelectionInDOMTree()| instead of deprecated
version.

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:739: // Here the
selection belongs to <divs>'s shadow tree and that tree has a
nit: s/divs/div/

[hugoh_UTC2, 2017-05-08 08:28:14]

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:352:
TEST_F(FrameSelectionTest, CaretInShadowTree) {
On 2017/05/05 18:32:02, Xiaocheng wrote:
> Let's have two test cases:
> - CaretInTextControl
> - CaretInShadowTree (that is not a UA shadow tree under text control)
> 
> The reason is that we treat text controls and other shadow trees differently.

You mean a caret inside a <div contenteditable> that's inside a shadow tree?
yosin@, is there a simpler/better "caret in shadow tree"-case?

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:478: // We use a
double click to create the selection [Berlin].
On 2017/05/05 18:32:02, Xiaocheng wrote:
> We can use FrameSelection::SetSelection to set selection in disabled text
> control:
> 
> Element* inner_editor = ToTextControlElement(textarea)->InnerEditorElement();
>
Selection().SetSelection(SelectionInDOMTree::Builder().SelectAllChildren(*inner_editor).Build());

Thanks. But let's use double click anyway since it is closer to what end users
do? Having some variation might also help us catch more bugs.

[yosin_UTC9, 2017-05-08 09:00:10]

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:352:
TEST_F(FrameSelectionTest, CaretInShadowTree) {
On 2017/05/08 at 08:28:14, hugoh_UTC2 wrote:
> On 2017/05/05 18:32:02, Xiaocheng wrote:
> > Let's have two test cases:
> > - CaretInTextControl
> > - CaretInShadowTree (that is not a UA shadow tree under text control)
> > 
> > The reason is that we treat text controls and other shadow trees
differently.
> 
> You mean a caret inside a <div contenteditable> that's inside a shadow tree?
yosin@, is there a simpler/better "caret in shadow tree"-case?

Following example can be simple(?):

SetBodyContent("<p id=host></p>");
ShadowRoot* shadow_root = SetShadowContent("<div contenteditable></div>");
Node* editable = shadow_root->firstChild();

VisibleUnitsTest.cpp has some usage SetShadowContent()

[hugoh_UTC2, 2017-05-08 09:08:13]

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:429: const Node*
current = ComputeVisibleSelectionInDOMTreeDeprecated()
On 2017/05/08 05:36:52, yosin_UTC9 wrote:
> nit: Please use |ComputeVisibleSelectionInDOMTree()| instead of deprecated
> version.

When I do, I get this crash:

$ out/PcDebug/webkit_unit_tests --gtest_filter=*FrameSelectionTest* 
...
[29332:29332:0508/094646.588352:1036716298934:FATAL:SelectionEditor.cpp(378)]
Check failed: GetDocument().Lifecycle().GetState() >=
DocumentLifecycle::kAfterPerformLayout (4 vs. 9)
#0 0x7fbfeb37d39b base::debug::StackTrace::StackTrace()
#1 0x7fbfeb37c09c base::debug::StackTrace::StackTrace()
#2 0x7fbfeb3ef8d3 logging::LogMessage::~LogMessage()
#3 0x7fbfe73d1e17 blink::SelectionEditor::UpdateCachedVisibleSelectionIfNeeded()
#4 0x7fbfe73d1bda blink::SelectionEditor::ComputeVisibleSelectionInDOMTree()
#5 0x7fbfe73a6501 blink::FrameSelection::ComputeVisibleSelectionInDOMTree()
#6 0x7fbfe73a8599 blink::FrameSelection::SelectionHasFocus()

[hugoh_UTC2, 2017-05-08 11:27:44]

Patchset #11 (id:240001) has been deleted

[hugoh_UTC2, 2017-05-08 11:32:06]

Patchset #11 (id:260001) has been deleted

[hugoh_UTC2, 2017-05-08 11:38:42]

Patchset #11 (id:280001) has been deleted

[hugoh_UTC2, 2017-05-08 11:40:03]

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:352:
TEST_F(FrameSelectionTest, CaretInShadowTree) {
On 2017/05/08 09:00:10, yosin_UTC9 wrote:
> On 2017/05/08 at 08:28:14, hugoh_UTC2 wrote:
> > On 2017/05/05 18:32:02, Xiaocheng wrote:
> > > Let's have two test cases:
> > > - CaretInTextControl
> > > - CaretInShadowTree (that is not a UA shadow tree under text control)
> > > 
> > > The reason is that we treat text controls and other shadow trees
> differently.
> > 
> > You mean a caret inside a <div contenteditable> that's inside a shadow tree?
> yosin@, is there a simpler/better "caret in shadow tree"-case?
> 
> Following example can be simple(?):
> 
> SetBodyContent("<p id=host></p>");
> ShadowRoot* shadow_root = SetShadowContent("<div contenteditable></div>");
> Node* editable = shadow_root->firstChild();
> 
> VisibleUnitsTest.cpp has some usage SetShadowContent()

Done. See PS11.

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:370:
TEST_F(FrameSelectionTest, RangeInShadowTree) {
On 2017/05/05 18:32:02, Xiaocheng wrote:
> Ditto.

Done. See PS11.

https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:739: // Here the
selection belongs to <divs>'s shadow tree and that tree has a
On 2017/05/08 05:36:52, yosin_UTC9 wrote:
> nit: s/divs/div/

Done. See PS11.

[hugoh_UTC2, 2017-05-08 11:46:18]

Description was changed from

==========
Algorithm for deciding if a frame's selection should be hidden

Background:
Crrev.com/464698 introduced "hiding" of unfocused selections
in text controls. Hiding avoids clearing the selection upon change
of focus.

Problem:
Now we only hide selections inside text controls.
Selections within content-editable elements must also be hidden.

Solution:
Generalize previous work into an algorithm that, given current
DOM and its activeElement, determines whether a frame's selection
should be hidden.

See the algorithm in InHidden() for documentation and read its
unit tests in FrameSelectionTest.

BUG=715889
==========

to

==========
Algorithm for deciding if a frame's selection should be hidden

Background:
Crrev.com/464698 introduced "hiding" of unfocused selections
in text controls. Hiding avoids clearing the selection upon change
of focus.

Problem:
Now we only hide selections inside text controls.
Selections within content-editable elements must also be hidden.

Solution:
Generalize previous work into an algorithm that, given current
DOM and its activeElement, determines whether a frame's selection
should be hidden.

See the algorithm in InHidden() for documentation and read its
unit tests in FrameSelectionTest.cpp.

BUG=715059, 715889
==========

[Mostyn Bramley-Moore, 2017-05-08 13:36:27]

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

[commit-bot: I haz the power, 2017-05-08 13:36:45]

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-08 15:41:14]

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

[commit-bot: I haz the power, 2017-05-08 15:41:15]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-05-08 22:08:30]

I'm OK with keeping the ComputeVisibleSelectionInDOMTreeDeprecated() calls in
this patch, since we have a standard cleanup process.

No other revision requested except in the unit tests.

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:369:
EXPECT_TRUE(Selection().IsHidden());  // Caret is now hidden.
nit: When documenting an EXPECT_FOO (or ASSERT_FOO), a better practice is to
print instead of to comment, so that we get more information when the test
fails:

EXPECT_TRUE(Selection().IsHidden()) << "Caret in shadow tree without focus
should be hidden";

Same applies to everything below.

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:390:
TEST_F(FrameSelectionTest, RangeInShadowTree) {
Sorry if I made a confusion. I was expecting a test case where we have a range
selection in a content editable div in a shadow tree:

TEST_F(FrameSelectionTest, EditableRangeInShadowTree) {
  SetBodyContent("<p id='host'></p>");
  ShadowRoot* root = SetShadowContent("<div id='ce' contenteditable>foo</div>");
  ...

  ce->focus();
  Selection().SelectAll();
  EXPECT(...)
  ...

  ce->blur();
  EXPECT(...)
  ...
}

Anyway, it's fine to keep this test case. Please also add the above test case.

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:526: const IntRect
elem_bounds = textarea->BoundsInViewport();
I still prefer setting selection directly here. A unit test is supposed to
depend on the fewest possible number of other components, so that it's easy to
identify the broken part when something goes wrong.

If we want to test mouse clicking behavior, a better place would be
SelectionControllerTest, EventHandlerTest or layout test.

[yosin_UTC9, 2017-05-09 01:29:37]

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:429: const Node*
current = ComputeVisibleSelectionInDOMTreeDeprecated()
I prefer to use |ComputeVisibleSelectionInDOMTree()| even if we have bug of
tracking this.
I don't want to see increasing of deprecated functions into new code.

If there are lots of call sites to make layout clean before calling this
functions,
we can update layout in this function with

  // TODO(editing-dev): Hoist updateStyleAndLayoutIgnorePendingStylesheets
  // to caller. See http://crbug.com/590369 for more details.
  GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets();

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:465: const Node* start
= ComputeVisibleSelectionInDOMTreeDeprecated()
We can use |ComputeVisibleSelectionInDOMTree()| since |SelectionHasFocus()|
updates layout.

[hugoh_UTC2, 2017-05-09 08:11:23]

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:429: const Node*
current = ComputeVisibleSelectionInDOMTreeDeprecated()
On 2017/05/09 01:29:37, yosin_UTC9 wrote:
> I prefer to use |ComputeVisibleSelectionInDOMTree()| even if we have bug of
> tracking this.
> I don't want to see increasing of deprecated functions into new code.
> 
> If there are lots of call sites to make layout clean before calling this
> functions,
> we can update layout in this function with
> 
>   // TODO(editing-dev): Hoist updateStyleAndLayoutIgnorePendingStylesheets
>   // to caller. See http://crbug.com/590369 for more details.
>   GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets();

Are you OK with hitting a DCHECK during unit testing?
See
https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
.
I mean, if it happens during unit testing, it may also happen during real
usage...

I guess the safest is to add
GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets(); here then?

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:465: const Node* start
= ComputeVisibleSelectionInDOMTreeDeprecated()
On 2017/05/09 01:29:37, yosin_UTC9 wrote:
> We can use |ComputeVisibleSelectionInDOMTree()| since |SelectionHasFocus()|
> updates layout.

Ah yes!

[yosin_UTC9, 2017-05-09 08:33:28]

On 2017/05/09 at 08:11:23, hugoh wrote:
>
https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):
> 
>
https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/FrameSelection.cpp:429: const Node*
current = ComputeVisibleSelectionInDOMTreeDeprecated()
> On 2017/05/09 01:29:37, yosin_UTC9 wrote:
> > I prefer to use |ComputeVisibleSelectionInDOMTree()| even if we have bug of
> > tracking this.
> > I don't want to see increasing of deprecated functions into new code.
> > 
> > If there are lots of call sites to make layout clean before calling this
> > functions,
> > we can update layout in this function with
> > 
> >   // TODO(editing-dev): Hoist updateStyleAndLayoutIgnorePendingStylesheets
> >   // to caller. See http://crbug.com/590369 for more details.
> >   GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets();
> 
> Are you OK with hitting a DCHECK during unit testing?
> See
https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
.
> I mean, if it happens during unit testing, it may also happen during real
usage...
> 
> I guess the safest is to add
GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets(); here then?
> 
Yes, please. 

Actually, this is equivalent to call
ComputeVisibleSelectionInDOMTreeDeprecated(), but we add update layout call for
 - Avoid to use deprecated function
 - Minimize patch size instead of hoisting update layout call.

[hugoh_UTC2, 2017-05-09 09:22:45]

@yosin, PTAL

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:429: const Node*
current = ComputeVisibleSelectionInDOMTreeDeprecated()
On 2017/05/09 08:11:23, hugoh_UTC2 wrote:
> On 2017/05/09 01:29:37, yosin_UTC9 wrote:
> > I prefer to use |ComputeVisibleSelectionInDOMTree()| even if we have bug of
> > tracking this.
> > I don't want to see increasing of deprecated functions into new code.
> > 
> > If there are lots of call sites to make layout clean before calling this
> > functions,
> > we can update layout in this function with
> > 
> >   // TODO(editing-dev): Hoist updateStyleAndLayoutIgnorePendingStylesheets
> >   // to caller. See http://crbug.com/590369 for more details.
> >   GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets();
> 
> Are you OK with hitting a DCHECK during unit testing?
> See
>
https://codereview.chromium.org/2841093002/diff/220001/third_party/WebKit/Sou...
> .
> I mean, if it happens during unit testing, it may also happen during real
> usage...
> 
> I guess the safest is to add
> GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets(); here then?

Done in PS12.

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:465: const Node* start
= ComputeVisibleSelectionInDOMTreeDeprecated()
On 2017/05/09 08:11:23, hugoh_UTC2 wrote:
> On 2017/05/09 01:29:37, yosin_UTC9 wrote:
> > We can use |ComputeVisibleSelectionInDOMTree()| since |SelectionHasFocus()|
> > updates layout.
> 
> Ah yes!

Done in PS12.

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:369:
EXPECT_TRUE(Selection().IsHidden());  // Caret is now hidden.
On 2017/05/08 22:08:29, Xiaocheng wrote:
> nit: When documenting an EXPECT_FOO (or ASSERT_FOO), a better practice is to
> print instead of to comment, so that we get more information when the test
> fails:
> 
> EXPECT_TRUE(Selection().IsHidden()) << "Caret in shadow tree without focus
> should be hidden";
> 
> Same applies to everything below.

Here I found it to messy to add a comment for every assert on IsHidden so I
tried to only comment the non-trivial lines.

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:390:
TEST_F(FrameSelectionTest, RangeInShadowTree) {
On 2017/05/08 22:08:29, Xiaocheng wrote:
> Sorry if I made a confusion. I was expecting a test case where we have a range
> selection in a content editable div in a shadow tree:
> 
> TEST_F(FrameSelectionTest, EditableRangeInShadowTree) {
>   SetBodyContent("<p id='host'></p>");
>   ShadowRoot* root = SetShadowContent("<div id='ce'
contenteditable>foo</div>");
>   ...
> 
>   ce->focus();
>   Selection().SelectAll();
>   EXPECT(...)
>   ...
> 
>   ce->blur();
>   EXPECT(...)
>   ...
> }
> 
> Anyway, it's fine to keep this test case. Please also add the above test case.

Done in PS12.

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:526: const IntRect
elem_bounds = textarea->BoundsInViewport();
On 2017/05/08 22:08:29, Xiaocheng wrote:
> I still prefer setting selection directly here. A unit test is supposed to
> depend on the fewest possible number of other components, so that it's easy to
> identify the broken part when something goes wrong.
> 
> If we want to test mouse clicking behavior, a better place would be
> SelectionControllerTest, EventHandlerTest or layout test.

I agree, but I think it is good to keep all tests of IsHidden() to this file.

[hugoh_UTC2, 2017-05-09 09:42:30]

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/2841093002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:526: const IntRect
elem_bounds = textarea->BoundsInViewport();
On 2017/05/09 09:22:45, hugoh_UTC2 wrote:
> On 2017/05/08 22:08:29, Xiaocheng wrote:
> > I still prefer setting selection directly here. A unit test is supposed to
> > depend on the fewest possible number of other components, so that it's easy
to
> > identify the broken part when something goes wrong.
> > 
> > If we want to test mouse clicking behavior, a better place would be
> > SelectionControllerTest, EventHandlerTest or layout test.
> 
> I agree, but I think it is good to keep all tests of IsHidden() to this file.

(I think it is good to use mouse clicks here since it adds a variation to the
testing of IsHidden(). As we don't test this exact user behavior elsewhere, I
think it OK to widen this unit test.)

[Xiaocheng, 2017-05-09 18:18:15]

lgtm

https://codereview.chromium.org/2841093002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2841093002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:429: //
TODO(editing-dev): Hoist updateStyleAndLayoutIgnorePendingStylesheets
nit: s/update/Update/

[hugoh_UTC2, 2017-05-09 19:19:40]

On 2017/05/09 18:18:15, Xiaocheng wrote:
> lgtm
> do you approve
>
https://codereview.chromium.org/2841093002/diff/320001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):
> 
>
https://codereview.chromium.org/2841093002/diff/320001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/FrameSelection.cpp:429: //
> TODO(editing-dev): Hoist updateStyleAndLayoutIgnorePendingStylesheets
> nit: s/update/Update/

Thank you! :) I will fix the nit once I get to the office tomorrow. yosin@, do
you approve too?

[yosin_UTC9, 2017-05-10 01:03:19]

lgtm

Thanks for working this!
Once this patch committed, we can fix related bugs.

[hugoh_UTC2, 2017-05-10 06:29:45]

The CQ bit was checked by hugoh@opera.com

[hugoh_UTC2, 2017-05-10 06:29:46]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org,
xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2841093002/#ps330001
(title: "Nit")

[commit-bot: I haz the power, 2017-05-10 06:30:32]

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-10 08:12:15]

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

[commit-bot: I haz the power, 2017-05-10 08:12:17]

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_...)

[yosin_UTC9, 2017-05-10 08:41:30]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-05-10 08:42:09]

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-10 11:32:43]

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

[commit-bot: I haz the power, 2017-05-10 11:32:45]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hugoh_UTC2, 2017-05-10 14:56:08]

The CQ bit was checked by hugoh@opera.com

[commit-bot: I haz the power, 2017-05-10 14:56:33]

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-10 16:40:03]

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

[commit-bot: I haz the power, 2017-05-10 16:40:04]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hugoh_UTC2, 2017-05-10 19:31:18]

The CQ bit was checked by hugoh@opera.com

[commit-bot: I haz the power, 2017-05-10 19:34:09]

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-10 21:38:43]

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

[commit-bot: I haz the power, 2017-05-10 21:38:45]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yosin_UTC9, 2017-05-11 00:57:21]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-05-11 00:58:30]

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-11 04:01:10]

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

[commit-bot: I haz the power, 2017-05-11 04:01:12]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yosin_UTC9, 2017-05-11 04:17:26]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-05-11 04:19:04]

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-11 05:28:33]

CQ is committing da patch.

Bot data: {"patchset_id": 330001, "attempt_start_ts": 1494476246392010,
"parent_rev": "011435e0b63669d86e6da8d54c35da4bc0b507f0", "commit_rev":
"8549ed4eaf9d95db494bd7028bf17110b26b6918"}

[commit-bot: I haz the power, 2017-05-11 05:28:44]

Description was changed from

==========
Algorithm for deciding if a frame's selection should be hidden

Background:
Crrev.com/464698 introduced "hiding" of unfocused selections
in text controls. Hiding avoids clearing the selection upon change
of focus.

Problem:
Now we only hide selections inside text controls.
Selections within content-editable elements must also be hidden.

Solution:
Generalize previous work into an algorithm that, given current
DOM and its activeElement, determines whether a frame's selection
should be hidden.

See the algorithm in InHidden() for documentation and read its
unit tests in FrameSelectionTest.cpp.

BUG=715059, 715889
==========

to

==========
Algorithm for deciding if a frame's selection should be hidden

Background:
Crrev.com/464698 introduced "hiding" of unfocused selections
in text controls. Hiding avoids clearing the selection upon change
of focus.

Problem:
Now we only hide selections inside text controls.
Selections within content-editable elements must also be hidden.

Solution:
Generalize previous work into an algorithm that, given current
DOM and its activeElement, determines whether a frame's selection
should be hidden.

See the algorithm in InHidden() for documentation and read its
unit tests in FrameSelectionTest.cpp.

BUG=715059, 715889

Review-Url: https://codereview.chromium.org/2841093002
Cr-Commit-Position: refs/heads/master@{#470822}
Committed:
https://chromium.googlesource.com/chromium/src/+/8549ed4eaf9d95db494bd7028bf1...
==========

[commit-bot: I haz the power, 2017-05-11 05:28:46]

Committed patchset #13 (id:330001) as
https://chromium.googlesource.com/chromium/src/+/8549ed4eaf9d95db494bd7028bf1...