Editing: Indent command: Do not insert BLOCKQUOTE under the root HTML element.

Editing: Indent command: Do not insert BLOCKQUOTE under the root HTML element.

- ApplyBlockElementCommand, which IndentOutdentCommand inherits from, shrinks
  selection to fit the body.

- startOfParagraph(), endOfParagraph(), mostBackwardCaretPosition(), and
  mostBackwardCaretPosition() make sure a position doesn't move across BODY
  boundary.

- Add NodeTraversal::contains() for the above bullet.
  FlatTreeTraversal already has contains().

- Add dumpFromRoot options to assert_selection().
  assert_selection() dumps children of the body if it exists by default.

BUG=657462

[tkent, 2017-03-17 02:13:37]

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

[commit-bot: I haz the power, 2017-03-17 02:14:04]

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

[tkent, 2017-03-17 02:30:44]

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

[commit-bot: I haz the power, 2017-03-17 02:31:07]

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-17 03:32:49]

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

[commit-bot: I haz the power, 2017-03-17 03:32:50]

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

[tkent, 2017-03-17 04:36:59]

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

[tkent, 2017-03-17 04:37:15]

Patchset #1 (id:1) has been deleted

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

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

[tkent, 2017-03-17 04:37:20]

Patchset #1 (id:20001) has been deleted

[tkent, 2017-03-17 04:38:07]

Description was changed from

==========
Editing: Indent command: Do not insert BLOCKQUOTE under the root HTML element.

- ApplyBlockElementCommand, which IndentOutdentCommand inherits from, shrinks
  selection to fit the body.

- startOfParagraph(), endOfParagraph(), mostBackwardCaretPosition(), and
  mostBackwardCaretPosition() make sure a position doesn't move across BODY
  boundary.

- Add dumpFromRoot options to assert_selection().

BUG=657462
==========

to

==========
Editing: Indent command: Do not insert BLOCKQUOTE under the root HTML element.

- ApplyBlockElementCommand, which IndentOutdentCommand inherits from, shrinks
  selection to fit the body.

- startOfParagraph(), endOfParagraph(), mostBackwardCaretPosition(), and
  mostBackwardCaretPosition() make sure a position doesn't move across BODY
  boundary.

- Add NodeTraversal::contains() for the above bullet.
  FlatTreeTraversal already has contains().

- Add dumpFromRoot options to assert_selection().

BUG=657462
==========

[tkent, 2017-03-17 04:38:38]

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

[tkent, 2017-03-17 04:38:39]

yosin@, would you review this please?
All tests should pass with this patch set.

[commit-bot: I haz the power, 2017-03-17 06:09:29]

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

[commit-bot: I haz the power, 2017-03-17 06:09:30]

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

[tkent, 2017-03-17 06:34:39]

Description was changed from

==========
Editing: Indent command: Do not insert BLOCKQUOTE under the root HTML element.

- ApplyBlockElementCommand, which IndentOutdentCommand inherits from, shrinks
  selection to fit the body.

- startOfParagraph(), endOfParagraph(), mostBackwardCaretPosition(), and
  mostBackwardCaretPosition() make sure a position doesn't move across BODY
  boundary.

- Add NodeTraversal::contains() for the above bullet.
  FlatTreeTraversal already has contains().

- Add dumpFromRoot options to assert_selection().

BUG=657462
==========

to

==========
Editing: Indent command: Do not insert BLOCKQUOTE under the root HTML element.

- ApplyBlockElementCommand, which IndentOutdentCommand inherits from, shrinks
  selection to fit the body.

- startOfParagraph(), endOfParagraph(), mostBackwardCaretPosition(), and
  mostBackwardCaretPosition() make sure a position doesn't move across BODY
  boundary.

- Add NodeTraversal::contains() for the above bullet.
  FlatTreeTraversal already has contains().

- Add dumpFromRoot options to assert_selection().
  assert_selection() dumps children of the body if it exists by default.

BUG=657462
==========

[yosin_UTC9, 2017-03-17 10:04:11]

https://codereview.chromium.org/2753193002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/editing/assert_selection.js (right):

https://codereview.chromium.org/2753193002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/assert_selection.js:25: //  dumpFromRoot:
A boolean. Default is false.
Could you move "assert_selection.js" change to another patch with a test in
"assert_selection.html"?

https://codereview.chromium.org/2753193002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/editing/execCommand/indent/indent_root.html
(right):

https://codereview.chromium.org/2753193002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/execCommand/indent/indent_root.html:22:
doc.execCommand("SelectAll")
nit: Let's use single-quote since other parts use single-quote.

https://codereview.chromium.org/2753193002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/execCommand/indent/indent_root.html:23:
doc.execCommand("Indent");
nit: Let's use single-quote since other parts use single-quote.

https://codereview.chromium.org/2753193002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/2753193002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1829: const HTMLElement*
body = startNode->document().body();
Could you move VisibleUnits.cpp change in another CL with units tests?

https://codereview.chromium.org/2753193002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1853: if
(startNodeIsInBody !=
There are three occurrences of this pattern. Is it worth to introduce local
function for this?

https://codereview.chromium.org/2753193002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
(right):

https://codereview.chromium.org/2753193002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:1955:
void CompositeEditCommand::shrinkEndingSelectionToFitBody() {
I would like this function as local function and functional-style, like:

EphemeralRange adjustRangeNotCrossingBody(const EphemeralRange& range, Element*
body) {
  const Position&  bodyStart = Positin::firstPositionInNode(body);
  const Position&  bodyEnd = Positin::lastPositionInNode(body);
  const Position& start = range.startPosition() < bodyStart ? bodyStart :
range.startPosition();
  const Position& end = range.endPositon() > bodyEnd ? bodyEnd :
range.endPostion();
  return EphemeralRange(start, end);
}

ApplyBlockElementCommand::doApply() {
  ...
  {
    const Position& start = endingSelection().visibleStart().deepEquivalent();
    const Position& end = endingSelection().visibleEnd().deepEquivalent();
    if (start.isConnected() && end.isConnected()) {
      const EphemeralRange range(start, end);
      const EphemeralRange adjustedRange = adjustRangeNotCrossingBody(range,
document().body());
      if (newRange != range) {
         setEndingSelection(SelectionInDOMTree::Builder()
            .setBaseAndExtent(newRange)
            .setIsDirecitonal(endingSelection().isDirectional())
            .build());
      }
    }
  }
...

[yosin_UTC9, 2017-03-17 10:06:13]

https://codereview.chromium.org/2753193002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NodeTraversal.h (right):

https://codereview.chromium.org/2753193002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NodeTraversal.h:125: return
container.contains(&other);
Since there is no unit test for ContainerNode::contains(). Could you move this
patch and add test "NodeTraversal.cpp"?

[tkent, 2017-03-27 05:34:11]

Offline discussion:
We concluded that we should not change VisibleUnits for this, and should adjust
positions/selections in the command layer instead. I filed crbug.com/705343.

I'm closing this issue. Fixing crbug.com/657462 with this way needs a few weeks,
I guess.