(CANCELED) Ensure before/after is within the editable in moveParagraphWithClones

Ensure before/after is within the editable in moveParagraphWithClones

When the selection starts at the beginning of the editable host and
execCommand "FormatBlock" is executed, moveParagraphWithClones computed
the before the selection crossing the editing boundaries. This resulted
in isEditablePosition() to fail, and aborted the editing operation.

This patch ensures before and after the selection to stay within the
editable host.

BUG=656866

[kojii, 2016-11-29 07:02:27]

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

[commit-bot: I haz the power, 2016-11-29 07:03:05]

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

[kojii, 2016-11-29 07:32:43]

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

[commit-bot: I haz the power, 2016-11-29 07:33:09]

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, 2016-11-29 08:57:08]

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

[commit-bot: I haz the power, 2016-11-29 08:57:09]

Dry run: 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_...)

[kojii, 2016-11-29 09:04:53]

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

[commit-bot: I haz the power, 2016-11-29 09:05:13]

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

[kojii, 2016-11-29 09:36:08]

Description was changed from

==========
WIP: selection-656866

BUG=656866
==========

to

==========
Ensure before/after is within the editable in moveParagraphWithClones

When the selection starts at the beginning of the editable host and
execCommand "FormatBlock" is executed, moveParagraphWithClones computed
the before the selection crossing the editing boundaries. This resulted
in isEditablePosition() to fail, and aborted the editing operation.

This patch ensures before and after the selection to stay within the
editable host.

BUG=656866
==========

[kojii, 2016-11-29 10:02:27]

The CQ bit was unchecked by kojii@chromium.org

[kojii, 2016-11-29 10:41:48]

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

[kojii, 2016-11-29 10:41:48]

PTAL.

[yosin_UTC9, 2016-11-30 01:48:41]

C++ changes are looks fine.
Please make tests to utilize assert_selection() for ease of maintenance.

https://codereview.chromium.org/2532373002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/execCommand/format-block-contenteditable-false-expected.txt
(right):

https://codereview.chromium.org/2532373002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/execCommand/format-block-contenteditable-false-expected.txt:2:
| <h1>
Could you convert this test to utilize |assert_selection()|?
I suggest you to be in a separate patch.

https://codereview.chromium.org/2532373002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/execCommand/format_block/format-block-at-host-selected.html
(right):

https://codereview.chromium.org/2532373002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/execCommand/format_block/format-block-at-host-selected.html:26:
test(function () {
Could you use |assert_selection()| for easier to read?
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/La...

[yosin_UTC9, 2016-11-30 03:18:51]

https://codereview.chromium.org/2532373002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/execCommand/format-block-contenteditable-false-expected.txt
(right):

https://codereview.chromium.org/2532373002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/execCommand/format-block-contenteditable-false-expected.txt:2:
| <h1>
On 2016/11/30 at 01:48:40, Yosi_UTC9 wrote:
> Could you convert this test to utilize |assert_selection()|?
> I suggest you to be in a separate patch.

It helps us what this CL changes behavior.
You can create dependency patches to review same time and commit sequencally.
http://dev.chromium.org/developers/how-tos/get-the-code/working-with-branches

[kojii, 2016-11-30 03:49:20]

Tried a few hours without luck. If assert_selection() inserts the HTML into
iframe, the test does not fail without the fix.

[yosin_UTC9, 2016-11-30 04:33:23]

On 2016/11/30 at 03:49:20, kojii wrote:
> Tried a few hours without luck. If assert_selection() inserts the HTML into
iframe, the test does not fail without the fix.

How about using gTest? See core/editing/commands/*Test.cpp.
You can call CompositeEditCommand::moveParagraphWithClones() directly. It is
better than having too sensitive HTML markup.

[kojii, 2016-11-30 04:35:21]

Sorry, I'll move on. I need to take one more bug today.

[kojii, 2016-11-30 08:09:45]

On 2016/11/30 at 04:33:23, yosin wrote:
> On 2016/11/30 at 03:49:20, kojii wrote:
> > Tried a few hours without luck. If assert_selection() inserts the HTML into
iframe, the test does not fail without the fix.
> 
> How about using gTest? See core/editing/commands/*Test.cpp.
> You can call CompositeEditCommand::moveParagraphWithClones() directly. It is
better than having too sensitive HTML markup.

I don't get it. It just moves the sensitive HTML from HTML to cpp, no? And
what's wrong with sensitive HTML?

[yosin_UTC9, 2016-12-01 01:30:26]

On 2016/11/30 at 08:09:45, kojii wrote:
> On 2016/11/30 at 04:33:23, yosin wrote:
> > On 2016/11/30 at 03:49:20, kojii wrote:
> > > Tried a few hours without luck. If assert_selection() inserts the HTML
into iframe, the test does not fail without the fix.
> > 
> > How about using gTest? See core/editing/commands/*Test.cpp.
> > You can call CompositeEditCommand::moveParagraphWithClones() directly. It is
better than having too sensitive HTML markup.
> 
> I don't get it. It just moves the sensitive HTML from HTML to cpp, no? And
what's wrong with sensitive HTML?

Sorry for confusion. We usually do following for ClusterFuzz bug:

1. Minimize HTML as much as possible to identify root cause in content_shell
2. Make specific HTML to cause the issue in gTest; The HTML causing HTML is the
result of DOM mutation by command.

I use term "sensitive HTML" as "fragile HTML" which won't cause the issue due by
other part of change.

In this case, the root cause is moveParagraphWithClones rather than
"formatBlock" command. 
Other execCommand call moveParagraphWithClones can hit this issue.

So, I guess following HTML may cause the bug:
<div style="display:inline-block">abc</div><div contenteditable>def<br>ghi</div>

moveParagraphWithClones (["def", 0], [div, 1], ...)

[kojii, 2016-12-02 05:49:10]

yosin@ said he's going to take this over, so closing.