Select All present even all selectable text has been selected

Select All present even all selectable text has been selected

When you right click in an empty text box "Select All" isn't grayed out. Also if you
were to use select all to select all text and then right click you "Select All"
would not be grayed out even though all text is already selected. This will also be
used in fixing Clank bug: crbug.com/615435

BUG=696134
Review-Url: https://codereview.chromium.org/2712603007
Cr-Commit-Position: refs/heads/master@{#457617}
Committed: https://chromium.googlesource.com/chromium/src/+/c9178af4426d3a2fc543bd77e929cea2562d8d4c

[amaralp, 2017-02-25 04:33:51]

Description was changed from

==========
Select All present even all selectable text has been selected

BUG=
==========

to

==========
Select All present even all selectable text has been selected

When you right click in an empty text box "Select All" isn't grayed out. Also if
you were to use select all to select all text and then right click you "Select
All" would not be grayed out even though all text is already selected. This will
also be used in fixing Clank bug: crbug.com/615435

BUG=696134
==========

[amaralp, 2017-02-25 04:33:51]

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

[amaralp, 2017-02-25 04:34:03]

Description was changed from

==========
Select All present even all selectable text has been selected

When you right click in an empty text box "Select All" isn't grayed out. Also if
you were to use select all to select all text and then right click you "Select
All" would not be grayed out even though all text is already selected. This will
also be used in fixing Clank bug: crbug.com/615435

BUG=696134
==========

to

==========
Select All present even all selectable text has been selected

When you right click in an empty text box "Select All" isn't grayed out. Also if
you were to use select all to select all text and then right click you "Select
All"
would not be grayed out even though all text is already selected. This will also
be
used in fixing Clank bug: crbug.com/615435

BUG=696134
==========

[amaralp, 2017-02-25 04:34:10]

Description was changed from

==========
Select All present even all selectable text has been selected

When you right click in an empty text box "Select All" isn't grayed out. Also if
you were to use select all to select all text and then right click you "Select
All"
would not be grayed out even though all text is already selected. This will also
be
used in fixing Clank bug: crbug.com/615435

BUG=696134
==========

to

==========
Select All present even all selectable text has been selected

When you right click in an empty text box "Select All" isn't grayed out. Also if
you
were to use select all to select all text and then right click you "Select All"
would not be grayed out even though all text is already selected. This will also
be
used in fixing Clank bug: crbug.com/615435

BUG=696134
==========

[amaralp, 2017-02-25 04:38:43]

PTAL, currently my implementation of |FrameSelection::canSelectAll()| always
returns true. I
think the problem may be related to the comment: 

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/P...

Any idea on how to fix this? Thanks!

[yosin_UTC9, 2017-02-27 03:46:40]

https://codereview.chromium.org/2712603007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2712603007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:629: Node*
FrameSelection::selectAllRoot() {
Since this function returns root node for "SelectAll", can we call this function
as |computeRootNodeForSelectAll()| or another name to describe what this
function does?

https://codereview.chromium.org/2712603007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:666: if
(isHTMLSelectElement(document().focusedElement())) {
Since document.focusedElement() can be null, please add null check for it.

https://codereview.chromium.org/2712603007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:686: bool
FrameSelection::canSelectAll() {
It is better to implement in |Editor| class rather than here?

https://codereview.chromium.org/2712603007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:687: if
(isHTMLSelectElement(document().focusedElement())) {
Since document.focusedElement() can be null, please add null check for it.

https://codereview.chromium.org/2712603007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:690: if
(selectElement->canSelectAll()) {
nit: We don't need to have braces.

[amaralp, 2017-02-27 05:02:18]

https://codereview.chromium.org/2712603007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2712603007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:629: Node*
FrameSelection::selectAllRoot() {
On 2017/02/27 at 03:46:40, yosin_UTC9 wrote:
> Since this function returns root node for "SelectAll", can we call this
function as |computeRootNodeForSelectAll()| or another name to describe what
this function does?

Renamed to |computeRootNodeForSelectAll()|.

https://codereview.chromium.org/2712603007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:666: if
(isHTMLSelectElement(document().focusedElement())) {
On 2017/02/27 at 03:46:40, yosin_UTC9 wrote:
> Since document.focusedElement() can be null, please add null check for it.

|isHTMLSelectElement()| already does the null check. Also this is how it was
done prior to my changes.
Do you still want the null check?

https://codereview.chromium.org/2712603007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:686: bool
FrameSelection::canSelectAll() {
On 2017/02/27 at 03:46:40, yosin_UTC9 wrote:
> It is better to implement in |Editor| class rather than here?

Moved it to |Editor|.

https://codereview.chromium.org/2712603007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:687: if
(isHTMLSelectElement(document().focusedElement())) {
On 2017/02/27 at 03:46:40, yosin_UTC9 wrote:
> Since document.focusedElement() can be null, please add null check for it.

|isHTMLSelectElement()| already does the null check.

https://codereview.chromium.org/2712603007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:690: if
(selectElement->canSelectAll()) {
On 2017/02/27 at 03:46:40, yosin_UTC9 wrote:
> nit: We don't need to have braces.

Done.

[yosin_UTC9, 2017-02-27 05:11:12]

lgtm

Thanks for quick work!

[amaralp, 2017-02-27 05:23:41]

On 2017/02/27 at 05:11:12, yosin wrote:
> lgtm
> 
> Thanks for quick work!

Thank you for the quick replies!

I think you may have missed my original question. My current implementation of
|Editor::canSelectAll()|
always returns true because of the way I am comparing |Position|'s in the return
statement . I think
the problem may be related to the comment: 

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/P...

Should I be using |Position::compareTo()| instead of the |==|?

[yosin_UTC9, 2017-02-27 06:21:56]

On 2017/02/27 at 05:23:41, amaralp wrote:
> On 2017/02/27 at 05:11:12, yosin wrote:
> > lgtm
> > 
> > Thanks for quick work!
> 
> Thank you for the quick replies!
> 
> I think you may have missed my original question. My current implementation of
|Editor::canSelectAll()|
> always returns true because of the way I am comparing |Position|'s in the
return statement . I think
> the problem may be related to the comment: 
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/P...
> 
> Should I be using |Position::compareTo()| instead of the |==|?

Sorry, I missed your question.
The FIXME is wrong. [div, 0] != [img, 0].
|Position::comapreTo()| == |comparePositions()| should return same result of
|Position::operator==|.

I'm not sure what HTML makes |root| to null. Try to insert |DCHECK(root)| before
L659 of |FrameSelection::selectAll()| then run layout test?

  DCHECK(root);
  if (!root || editingIgnoresContent(*root))
    return;

BTW, we may want to have a test for |ContextMenuClientImpl::showContextMenu()
changes| to check |data.editFlags|

[amaralp, 2017-02-27 19:17:33]

> Sorry, I missed your question.
> The FIXME is wrong. [div, 0] != [img, 0].
> |Position::comapreTo()| == |comparePositions()| should return same result of
|Position::operator==|.
> 
How would I check if two positions would be the same visibly? For example if I
have a caret selection in an
empty text box the selection's base has value 'DIV id="inner-editor"
(editable)@offsetInAnchor[0]' but |Position::firstPositionInNode(selectAllRoot)|
gives 'DIV id="inner-editor" (editable)@beforeChildren'. Both these positions
refer to the same location but they are not equal when using ==. Another
situation is where I have selected all the text in a text box. Then the
selection has base '#text "some text"@offsetInAnchor[0]' while
|firstPostionInNode(selectAllRoot)| still gives 'DIV id="inner-editor"
(editable)@beforeChildren'. Is there a way to check for this type of |Position| 
equality?

[yosin_UTC9, 2017-02-28 02:30:59]

On 2017/02/27 at 19:17:33, amaralp wrote:
> > Sorry, I missed your question.
> > The FIXME is wrong. [div, 0] != [img, 0].
> > |Position::comapreTo()| == |comparePositions()| should return same result of
|Position::operator==|.
> > 
> How would I check if two positions would be the same visibly? For example if I
have a caret selection in an
> empty text box the selection's base has value 'DIV id="inner-editor"
(editable)@offsetInAnchor[0]' but |Position::firstPositionInNode(selectAllRoot)|
> gives 'DIV id="inner-editor" (editable)@beforeChildren'. Both these positions
refer to the same location but they are not equal when using ==. Another
> situation is where I have selected all the text in a text box. Then the
selection has base '#text "some text"@offsetInAnchor[0]' while
> |firstPostionInNode(selectAllRoot)| still gives 'DIV id="inner-editor"
(editable)@beforeChildren'. Is there a way to check for this type of |Position| 
> equality?

Thanks for clarification.
How about using Position::toOffsetInAnchor(), aka normalize Position?
This converts {Before,After}{Anchor,Children} position to OffsetInAnchor
position.

[amaralp, 2017-02-28 02:49:56]

> Thanks for clarification.
> How about using Position::toOffsetInAnchor(), aka normalize Position?
> This converts {Before,After}{Anchor,Children} position to OffsetInAnchor
position.

Thank you for the suggestion. It fixed the problem when the text box is empty,
but not when you have highlighted all the text inside a textbox.
The problem is that when you highlight text the |anchorNode| becomes '#text
"some text"' but |firstPostionInNode(selectAllRoot)| has it's
|anchorNode| as 'DIV id="inner-editor" (editable)'.

[amaralp, 2017-03-01 19:00:30]

amaralp@chromium.org changed reviewers:
+ aelias@chromium.org

[amaralp, 2017-03-02 00:05:46]

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

[commit-bot: I haz the power, 2017-03-02 00:06:27]

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-03-02 01:07:44]

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

[commit-bot: I haz the power, 2017-03-02 01:07:45]

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

[amaralp, 2017-03-02 01:41:11]

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

[commit-bot: I haz the power, 2017-03-02 01:41:58]

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-03-02 02:15:10]

On 2017/02/28 at 02:49:56, amaralp wrote:
> > Thanks for clarification.
> > How about using Position::toOffsetInAnchor(), aka normalize Position?
> > This converts {Before,After}{Anchor,Children} position to OffsetInAnchor
position.
> 
> Thank you for the suggestion. It fixed the problem when the text box is empty,
but not when you have highlighted all the text inside a textbox.
> The problem is that when you highlight text the |anchorNode| becomes '#text
"some text"' but |firstPostionInNode(selectAllRoot)| has it's
> |anchorNode| as 'DIV id="inner-editor" (editable)'.

not lgtm

Sorry, I was wrong. This should be done by
Document::queryCommandEnable("selectAll").
Context menu can check returned boolean from
Document::queryCommandEnable("selectAll").

In current implementation, "selectAll" command is always enabled.
It seems "Select All" menu item is always on desktop application, checked on Mac
and Win.
Do we really want this behavior?


In src/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:

static const EditorInternalCommand* internalCommand(const String& commandName) {
  static const EditorInternalCommand kEditorCommands[] = {
...
  {WebEditingCommandType::SelectAll, 
   executeSelectAll, 
   supported, 
   enabled,
   stateNone, 
   valueNull, 
   notTextInsertion, 
   doNotAllowExecutionWhenDisabled},

Another question is what should we do context menu showed on
user-select:none[1]?
 - disable "select all"
 - enable "select all" then select all nodes except for inclusive descendants of
user-select:none elements.

I'll put this into crbug.com/696134

[1] https://drafts.csswg.org/css-ui-4/#user-interaction

[amaralp, 2017-03-02 03:10:26]

On 2017/03/02 at 02:15:10, yosin wrote:
> On 2017/02/28 at 02:49:56, amaralp wrote:
> > > Thanks for clarification.
> > > How about using Position::toOffsetInAnchor(), aka normalize Position?
> > > This converts {Before,After}{Anchor,Children} position to OffsetInAnchor
position.
> > 
> > Thank you for the suggestion. It fixed the problem when the text box is
empty, but not when you have highlighted all the text inside a textbox.
> > The problem is that when you highlight text the |anchorNode| becomes '#text
"some text"' but |firstPostionInNode(selectAllRoot)| has it's
> > |anchorNode| as 'DIV id="inner-editor" (editable)'.
> 
> not lgtm
> 
> Sorry, I was wrong. This should be done by
Document::queryCommandEnable("selectAll").
> Context menu can check returned boolean from
Document::queryCommandEnable("selectAll").
> 
You want |ContextMenuClientImpl::showContextMenu()| to use queryCommandEnable()?
All the other editing flags (Cut, Copy, Paste, etc.) are set according to the
corresponding |Editor| functions (|Editor::canCut()|, |Editor::canCopy()|,
etc.).
So it seems a little awkward to do it differently for select all. Also
queryCommandEnable()
will eventually call |Editor:;canSelectAll()| anyways.

> In current implementation, "selectAll" command is always enabled.
> It seems "Select All" menu item is always on desktop application, checked on
Mac and Win.
> Do we really want this behavior?
> 
Android native textboxes only enable select all when there is additional text to
select and
so we'd need to support this for Android so why not for all platforms? Also as
you've noted
in the bug report Firefox has it disabled if it is empty.

> 
> In src/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:
> 
> static const EditorInternalCommand* internalCommand(const String& commandName)
{
>   static const EditorInternalCommand kEditorCommands[] = {
> ...
>   {WebEditingCommandType::SelectAll, 
>    executeSelectAll, 
>    supported, 
>    enabled,
>    stateNone, 
>    valueNull, 
>    notTextInsertion, 
>    doNotAllowExecutionWhenDisabled},
> 
> Another question is what should we do context menu showed on
user-select:none[1]?
>  - disable "select all"
>  - enable "select all" then select all nodes except for inclusive descendants
of user-select:none elements.
> 
Wouldn't this be an issue regardless of this CL?

> I'll put this into crbug.com/696134
> 
> [1] https://drafts.csswg.org/css-ui-4/#user-interaction

[amaralp, 2017-03-02 03:46:35]

> > Another question is what should we do context menu showed on
user-select:none[1]?
> >  - disable "select all"
> >  - enable "select all" then select all nodes except for inclusive
descendants of user-select:none elements.
> > 
> Wouldn't this be an issue regardless of this CL?
> 

I misunderstood what you meant. aelias@ and I discussed this in person and came
to the conclusion that enabling
"select all" in the case where there is a user-select:none is fine.

[aelias_OOO_until_Jul13, 2017-03-02 04:01:51]

On 2017/03/02 at 03:10:26, amaralp wrote:
> > In current implementation, "selectAll" command is always enabled.
> > It seems "Select All" menu item is always on desktop application, checked on
Mac and Win.
> > Do we really want this behavior?
> > 
> Android native textboxes only enable select all when there is additional text
to select and
> so we'd need to support this for Android so why not for all platforms? Also as
you've noted
> in the bug report Firefox has it disabled if it is empty.

It's a platform convention on Android that we need to meet to reach parity with
native TextView.  On Android, there is limited screen real estate so hiding
"Select All" when there is nothing to select amounts to a significant benefit. 
On desktop platforms, I don't think it matters much either way given how
cluttered the menu is.  I do think it's slightly better to gray it out on
desktop if it would do nothing, and don't understand the counterargument for
keeping it always active (but if you insist on this, we could always fork the
behavior per platform).

On 2017/03/02 at 03:46:35, amaralp wrote:
> > > Another question is what should we do context menu showed on
user-select:none[1]?
> > >  - disable "select all"
> > >  - enable "select all" then select all nodes except for inclusive
descendants of user-select:none elements.
> > > 
> > Wouldn't this be an issue regardless of this CL?
> > 
> 
> I misunderstood what you meant. aelias@ and I discussed this in person and
came to the conclusion that enabling
> "select all" in the case where there is a user-select:none is fine.

Right, we're interested in hiding "Select All" in specific common cases on
Android (empty textboxes, especially), as a usability improvement given the
limited screen real estate.  I don't think we need to go out of our way to
disable it in other cases: it's fine to have a biased towards showing the menu
option in cases where it would require extra code/complexity to determine that
it would have no effect.

[yosin_UTC9, 2017-03-02 04:29:05]

On 2017/03/02 at 03:10:26, amaralp wrote:
> On 2017/03/02 at 02:15:10, yosin wrote:
> > On 2017/02/28 at 02:49:56, amaralp wrote:
> > > > Thanks for clarification.
> > > > How about using Position::toOffsetInAnchor(), aka normalize Position?
> > > > This converts {Before,After}{Anchor,Children} position to OffsetInAnchor
position.
> > > 
> > > Thank you for the suggestion. It fixed the problem when the text box is
empty, but not when you have highlighted all the text inside a textbox.
> > > The problem is that when you highlight text the |anchorNode| becomes
'#text "some text"' but |firstPostionInNode(selectAllRoot)| has it's
> > > |anchorNode| as 'DIV id="inner-editor" (editable)'.
> > 
> > not lgtm
> > 
> > Sorry, I was wrong. This should be done by
Document::queryCommandEnable("selectAll").
> > Context menu can check returned boolean from
Document::queryCommandEnable("selectAll").
> > 
> You want |ContextMenuClientImpl::showContextMenu()| to use
queryCommandEnable()?
> All the other editing flags (Cut, Copy, Paste, etc.) are set according to the
> corresponding |Editor| functions (|Editor::canCut()|, |Editor::canCopy()|,
etc.).
> So it seems a little awkward to do it differently for select all. Also
queryCommandEnable()
> will eventually call |Editor:;canSelectAll()| anyways.

This is opposite.

We want not to expose |Editor| to other modules. These editor reference will be
replaced to
document.queryEnabledCommand("copy") etc, actual Document::queryCommandEnabled()
calls them[1].

[1] http://crbug.com/672405


> > In current implementation, "selectAll" command is always enabled.
> > It seems "Select All" menu item is always on desktop application, checked on
Mac and Win.
> > Do we really want this behavior?
> > 
> Android native textboxes only enable select all when there is additional text
to select and
> so we'd need to support this for Android so why not for all platforms? Also as
you've noted
> in the bug report Firefox has it disabled if it is empty.
> 
> > 
> > In src/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:
> > 
> > static const EditorInternalCommand* internalCommand(const String&
commandName) {
> >   static const EditorInternalCommand kEditorCommands[] = {
> > ...
> >   {WebEditingCommandType::SelectAll, 
> >    executeSelectAll, 
> >    supported, 
> >    enabled,
> >    stateNone, 
> >    valueNull, 
> >    notTextInsertion, 
> >    doNotAllowExecutionWhenDisabled},
> > 
> > Another question is what should we do context menu showed on
user-select:none[1]?
> >  - disable "select all"
> >  - enable "select all" then select all nodes except for inclusive
descendants of user-select:none elements.
> > 
> Wouldn't this be an issue regardless of this CL?
> 
> > I'll put this into crbug.com/696134
> > 
> > [1] https://drafts.csswg.org/css-ui-4/#user-interaction

[yosin_UTC9, 2017-03-02 04:35:59]

On 2017/03/02 at 04:01:51, aelias wrote:
> On 2017/03/02 at 03:10:26, amaralp wrote:
> > > In current implementation, "selectAll" command is always enabled.
> > > It seems "Select All" menu item is always on desktop application, checked
on Mac and Win.
> > > Do we really want this behavior?
> > > 
> > Android native textboxes only enable select all when there is additional
text to select and
> > so we'd need to support this for Android so why not for all platforms? Also
as you've noted
> > in the bug report Firefox has it disabled if it is empty.
> 
> It's a platform convention on Android that we need to meet to reach parity
with native TextView.  On Android, there is limited screen real estate so hiding
"Select All" when there is nothing to select amounts to a significant benefit. 
On desktop platforms, I don't think it matters much either way given how
cluttered the menu is.  I do think it's slightly better to gray it out on
desktop if it would do nothing, and don't understand the counterargument for
keeping it always active (but if you insist on this, we could always fork the
behavior per platform).
> 
> On 2017/03/02 at 03:46:35, amaralp wrote:
> > > > Another question is what should we do context menu showed on
user-select:none[1]?
> > > >  - disable "select all"
> > > >  - enable "select all" then select all nodes except for inclusive
descendants of user-select:none elements.
> > > > 
> > > Wouldn't this be an issue regardless of this CL?
> > > 
> > 
> > I misunderstood what you meant. aelias@ and I discussed this in person and
came to the conclusion that enabling
> > "select all" in the case where there is a user-select:none is fine.
> 
> Right, we're interested in hiding "Select All" in specific common cases on
Android (empty textboxes, especially), as a usability improvement given the
limited screen real estate.  I don't think we need to go out of our way to
disable it in other cases: it's fine to have a biased towards showing the menu
option in cases where it would require extra code/complexity to determine that
it would have no effect.

Thanks for clarification. My disagreement is implementation of this patch. I
agree to change enable/disable of "select all".
It is good to start with text control only to avoid user-select issue.

enableSelectAll() function can be:

bool enableSelectAll(...) {
  if (selection.isNone()) return true;
  if (auto* textControl = enclosingTextControl(selection.start())) {
    check empty of text control
    optional: check fully selected from text control API.
  }
  // TODO: Support contentEditable; maybe we can use TextIterator with object
character enabled to catch IMG, VIDOE, etc.
  // TODO: Address user-select handling.
  return true;
}

[amaralp, 2017-03-03 22:59:40]

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

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[amaralp, 2017-03-03 23:25:58]

I've implemented enabledSelectAll() as you instructed. As for the TODO's the
most important one for (crbug.com/615435) would be to have enabledSelectAll()
return false for an empty contentEditable.

[yosin_UTC9, 2017-03-06 04:54:22]

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

https://codereview.chromium.org/2712603007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1964:
frame.selection().computeVisibleSelectionInDOMTreeDeprecated();
Please don't use deprecated version in new code.
You could write:

// TODO(editing-dev): The use of updateStyleAndLayoutIgnorePendingStylesheets
// needs to be audited.  See http://crbug.com/590369 for more details.
frame.document().updateStyleAndLayoutIgnorePendingStylesheets();
const VisbileSelection& selection =
frame.selection().computeVisibleSelectionInDOMTree();

[amaralp, 2017-03-06 22:48:30]

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

[commit-bot: I haz the power, 2017-03-06 22:49: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-03-06 22:59:27]

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

[commit-bot: I haz the power, 2017-03-06 22:59:27]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[amaralp, 2017-03-06 23:18:28]

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

[commit-bot: I haz the power, 2017-03-06 23:19: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-03-07 00:25:59]

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

[commit-bot: I haz the power, 2017-03-07 00:26:00]

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

[yosin_UTC9, 2017-03-07 02:13:11]

On 2017/03/06 at 04:54:22, yosin_UTC9 wrote:
>
https://codereview.chromium.org/2712603007/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
(right):
> 
>
https://codereview.chromium.org/2712603007/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1964:
frame.selection().computeVisibleSelectionInDOMTreeDeprecated();
> Please don't use deprecated version in new code.
> You could write:
> 
> // TODO(editing-dev): The use of updateStyleAndLayoutIgnorePendingStylesheets
> // needs to be audited.  See http://crbug.com/590369 for more details.
> frame.document().updateStyleAndLayoutIgnorePendingStylesheets();
> const VisbileSelection& selection =
frame.selection().computeVisibleSelectionInDOMTree();

Could you add layout test for Document#questCommandEnabled('selectAll')?
We would like to use it for regression and run on other browsers.
Thanks

[amaralp, 2017-03-09 02:03:39]

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

[commit-bot: I haz the power, 2017-03-09 02:04:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[amaralp, 2017-03-09 02:22:50]

On 2017/03/07 at 02:13:11, yosin wrote:
> On 2017/03/06 at 04:54:22, yosin_UTC9 wrote:
> >
https://codereview.chromium.org/2712603007/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
(right):
> > 
> >
https://codereview.chromium.org/2712603007/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1964:
frame.selection().computeVisibleSelectionInDOMTreeDeprecated();
> > Please don't use deprecated version in new code.
> > You could write:
> > 
> > // TODO(editing-dev): The use of
updateStyleAndLayoutIgnorePendingStylesheets
> > // needs to be audited.  See http://crbug.com/590369 for more details.
> > frame.document().updateStyleAndLayoutIgnorePendingStylesheets();
> > const VisbileSelection& selection =
frame.selection().computeVisibleSelectionInDOMTree();
> 
> Could you add layout test for Document#questCommandEnabled('selectAll')?
> We would like to use it for regression and run on other browsers.
> Thanks

Added a test. What should be done for the |ExceptionState| that
Document#queryCommandEnabled() expects in ContextMenuClientImpl.cpp?

[commit-bot: I haz the power, 2017-03-09 02:58:38]

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

[commit-bot: I haz the power, 2017-03-09 02:58:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-03-09 03:29:06]

On 2017/03/09 at 02:22:50, amaralp wrote:
> On 2017/03/07 at 02:13:11, yosin wrote:
> > On 2017/03/06 at 04:54:22, yosin_UTC9 wrote:
> > >
https://codereview.chromium.org/2712603007/diff/120001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
(right):
> > > 
> > >
https://codereview.chromium.org/2712603007/diff/120001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1964:
frame.selection().computeVisibleSelectionInDOMTreeDeprecated();
> > > Please don't use deprecated version in new code.
> > > You could write:
> > > 
> > > // TODO(editing-dev): The use of
updateStyleAndLayoutIgnorePendingStylesheets
> > > // needs to be audited.  See http://crbug.com/590369 for more details.
> > > frame.document().updateStyleAndLayoutIgnorePendingStylesheets();
> > > const VisbileSelection& selection =
frame.selection().computeVisibleSelectionInDOMTree();
> > 
> > Could you add layout test for Document#questCommandEnabled('selectAll')?
> > We would like to use it for regression and run on other browsers.
> > Thanks
> 
> Added a test. What should be done for the |ExceptionState| that
Document#queryCommandEnabled() expects in ContextMenuClientImpl.cpp?

|ASSERT_NO_EXCEPTION| is enough, since we don't expect queryCommandEnabled()
failed on "selectAll", at this time. ;-)
If cluster fuzz find the case, we can tailor it.

Could you also add test for context menu to avoid regression in context menu
implementation?
For editing, your layout test is OK.

[yosin_UTC9, 2017-03-09 03:33:07]

https://codereview.chromium.org/2712603007/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/editing/selection/select_all/select_all_empty_textarea.html
(right):

https://codereview.chromium.org/2712603007/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/selection/select_all/select_all_empty_textarea.html:4:
<textarea id="textarea"></textarea>
We also want to have test for INPUT element.
And contenteditable for record current behavior to emphasize contentEditable
doesn't handle empty text yet.

https://codereview.chromium.org/2712603007/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/selection/select_all/select_all_empty_textarea.html:11:
textarea.appendChild(document.createTextNode("x"));
nit: textarea.value = 'x' is simpler.

[amaralp, 2017-03-13 21:09:41]

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

[commit-bot: I haz the power, 2017-03-13 21:10:55]

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-03-13 21:14:20]

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

[commit-bot: I haz the power, 2017-03-13 21:14:21]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[amaralp, 2017-03-13 22:03:37]

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

[commit-bot: I haz the power, 2017-03-13 22:04:19]

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-03-13 23:15:39]

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

[commit-bot: I haz the power, 2017-03-13 23:15:40]

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

[amaralp, 2017-03-14 00:45:27]

> |ASSERT_NO_EXCEPTION| is enough, since we don't expect queryCommandEnabled()
failed on "selectAll", at this time. ;-)
> If cluster fuzz find the case, we can tailor it.
> 
There already is a failure in
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/svg/custo....

> Could you also add test for context menu to avoid regression in context menu
implementation?
Done.

https://codereview.chromium.org/2712603007/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/editing/selection/select_all/select_all_empty_textarea.html
(right):

https://codereview.chromium.org/2712603007/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/selection/select_all/select_all_empty_textarea.html:4:
<textarea id="textarea"></textarea>
On 2017/03/09 at 03:33:07, yosin_UTC9 wrote:
> We also want to have test for INPUT element.
> And contenteditable for record current behavior to emphasize contentEditable
doesn't handle empty text yet.

Done.

https://codereview.chromium.org/2712603007/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/selection/select_all/select_all_empty_textarea.html:11:
textarea.appendChild(document.createTextNode("x"));
On 2017/03/09 at 03:33:07, yosin_UTC9 wrote:
> nit: textarea.value = 'x' is simpler.

Done.

[yosin_UTC9, 2017-03-14 02:05:30]

On 2017/03/14 at 00:45:27, amaralp wrote:
> > |ASSERT_NO_EXCEPTION| is enough, since we don't expect queryCommandEnabled()
failed on "selectAll", at this time. ;-)
> > If cluster fuzz find the case, we can tailor it.
> > 
> There already is a failure in
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/svg/custo....

Since execCommand doesn't work on non-HTML document, we should disable editing
menu items.
Given that, we should use document type before computing enable/disable of menu
items.
In addition to SVG, we have XML tree view, XSLT output, text, image, video,
source code, etc.

Easy way is:

bool ContextMenuClientImpl::showContextMenu(...) {
  ...
  if (document.isHTMLDocument() || document.isXHTMLDocument()) {
   ... using Document#queryCommandEnabled
  } else {
   ... disable editing commands
  }
}

Document::queryCommandEnabled() implementation

bool Document::queryCommandEnabled(const String& commandName,
                                   ExceptionState& exceptionState) {
  if (!isHTMLDocument() && !isXHTMLDocument()) {
    exceptionState.throwDOMException(
        InvalidStateError,
        "queryCommandEnabled is only supported on HTML documents.");
    return false;
  }

  return command(this, commandName).isEnabled();
}

[amaralp, 2017-03-14 20:39:09]

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

[commit-bot: I haz the power, 2017-03-14 20:40:02]

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-03-14 22:18:11]

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

[commit-bot: I haz the power, 2017-03-14 22:18:13]

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

[amaralp, 2017-03-15 22:26:59]

On 2017/03/14 at 02:05:30, yosin wrote:
> On 2017/03/14 at 00:45:27, amaralp wrote:
> > > |ASSERT_NO_EXCEPTION| is enough, since we don't expect
queryCommandEnabled() failed on "selectAll", at this time. ;-)
> > > If cluster fuzz find the case, we can tailor it.
> > > 
> > There already is a failure in
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/svg/custo....
> 
> Since execCommand doesn't work on non-HTML document, we should disable editing
menu items.
> Given that, we should use document type before computing enable/disable of
menu items.
> In addition to SVG, we have XML tree view, XSLT output, text, image, video,
source code, etc.
> 
> Easy way is:
> 
> bool ContextMenuClientImpl::showContextMenu(...) {
>   ...
>   if (document.isHTMLDocument() || document.isXHTMLDocument()) {
>    ... using Document#queryCommandEnabled
>   } else {
>    ... disable editing commands
>   }
> }
> 
> Document::queryCommandEnabled() implementation
> 
> bool Document::queryCommandEnabled(const String& commandName,
>                                    ExceptionState& exceptionState) {
>   if (!isHTMLDocument() && !isXHTMLDocument()) {
>     exceptionState.throwDOMException(
>         InvalidStateError,
>         "queryCommandEnabled is only supported on HTML documents.");
>     return false;
>   }
> 
>   return command(this, commandName).isEnabled();
> }

Done.

[yosin_UTC9, 2017-03-16 01:21:35]

lgtm w/ nits

Thanks for long iteration!

https://codereview.chromium.org/2712603007/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2712603007/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11463: };
nit: Could you add |DISALLOW_COPY_AND_ASSIGN(ContextMenuWebFrameClient)|?

https://codereview.chromium.org/2712603007/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11495:
EXPECT_TRUE(testSelectAll("<div contenteditable></div>"));
nit: Could you add a TODO comment that we actually expect this is false.

[amaralp, 2017-03-16 03:06:45]

On 2017/03/16 at 01:21:35, yosin wrote:
> lgtm w/ nits
> 
> Thanks for long iteration!
> 
Thanks for your helpful feedback!
>
https://codereview.chromium.org/2712603007/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):
> 
>
https://codereview.chromium.org/2712603007/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11463: };
> nit: Could you add |DISALLOW_COPY_AND_ASSIGN(ContextMenuWebFrameClient)|?
> 
Done.
>
https://codereview.chromium.org/2712603007/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11495:
EXPECT_TRUE(testSelectAll("<div contenteditable></div>"));
> nit: Could you add a TODO comment that we actually expect this is false.
Done.

aelias@, could you review the Source/web parts? I still need OWNERS for that.

[aelias_OOO_until_Jul13, 2017-03-16 03:11:38]

Source/web lgtm

[amaralp, 2017-03-16 04:53:26]

The CQ bit was checked by amaralp@chromium.org

[amaralp, 2017-03-16 04:53:26]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2712603007/#ps260001
(title: "yosin's nits")

[commit-bot: I haz the power, 2017-03-16 04:53:54]

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-03-16 05:27:38]

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

[commit-bot: I haz the power, 2017-03-16 05:27:40]

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

[amaralp, 2017-03-17 00:03:22]

The CQ bit was checked by amaralp@chromium.org

[commit-bot: I haz the power, 2017-03-17 00:03:49]

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-03-17 00:09:36]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1489709002298930,
"parent_rev": "125ad86aa910bde12f4dd3338fe5d97553f3979d", "commit_rev":
"c9178af4426d3a2fc543bd77e929cea2562d8d4c"}

[commit-bot: I haz the power, 2017-03-17 00:11:03]

Description was changed from

==========
Select All present even all selectable text has been selected

When you right click in an empty text box "Select All" isn't grayed out. Also if
you
were to use select all to select all text and then right click you "Select All"
would not be grayed out even though all text is already selected. This will also
be
used in fixing Clank bug: crbug.com/615435

BUG=696134
==========

to

==========
Select All present even all selectable text has been selected

When you right click in an empty text box "Select All" isn't grayed out. Also if
you
were to use select all to select all text and then right click you "Select All"
would not be grayed out even though all text is already selected. This will also
be
used in fixing Clank bug: crbug.com/615435

BUG=696134

Review-Url: https://codereview.chromium.org/2712603007
Cr-Commit-Position: refs/heads/master@{#457617}
Committed:
https://chromium.googlesource.com/chromium/src/+/c9178af4426d3a2fc543bd77e929...
==========

[commit-bot: I haz the power, 2017-03-17 00:11:04]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/c9178af4426d3a2fc543bd77e929...