Utilize Position::IsEquivalent() in {Up,Down}streamIgnoringEditingBoundaries()

Utilize Position::IsEquivalent() in {Up,Down}streamIgnoringEditingBoundaries()

This patch utilized |Position::IsEquivalent()| in
|{Up,Down}streamIgnoringEditingBoundaries()| to avoid infinite loop.

In http://crbug.com/716093, |UpstreamIgnoringEditingBoundaries()| causes
infinite loop as:
- |MostBackwardCaretPosition(AfterChildren/DIV)| After/INPUT
- |MostBackwardCaretPosition(After/INPUT)| AfterChildren/DIV
for <div contenteditable>foo <input contenteditable="false"></div>

When I attempt to change |MostBackwardCaretPosition()| to return one of them,
some layout tests failed. Hence, to minimize change, this patch change loop
condition.

BUG=716093
TEST=run_webkit_unit_tests --gtest_filter=VisibleUnitsTest.ComputeInlineBoxPositionMixedEditabl

Review-Url: https://codereview.chromium.org/2875933004
Cr-Commit-Position: refs/heads/master@{#471697}
Committed: https://chromium.googlesource.com/chromium/src/+/9634436d854ad5431c48b8d3ddecd46050cbb74a

[yosin_UTC9, 2017-05-12 07:55:35]

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

[commit-bot: I haz the power, 2017-05-12 07:55:56]

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-05-12 08:39:44]

Description was changed from

==========
2017-05-12T16:49:43

BUG=716093

2017-05-12T16:40:24
==========

to

==========
Utilize Position::IsEquivalent() in {Up,Down}streamIgnoringEditingBoundaries()

This patch utilized |Position::IsEquivalent()| in
|{Up,Down}streamIgnoringEditingBoundaries()| to avoid infinite loop.

In http://crbug.com/716093, |UpstreamIgnoringEditingBoundaries()| causes
infinite loop as:
- |MostBackwardCaretPosition(AfterChildren/DIV)| After/INPUT
- |MostBackwardCaretPosition(After/INPUT)| AfterChildren/DIV
for <div contenteditable>foo <input contenteditable="false"></div>

When I attempt to change |MostBackwardCaretPosition()| to return one of them,
some layout tests failed. Hence, to minimize change, this patch change loop
condition.

BUG=716093
TEST=run_webkit_unit_tests
--gtest_filter=VisibleUnitsTest.ComputeInlineBoxPositionMixedEditabl
==========

[commit-bot: I haz the power, 2017-05-12 08:56:43]

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

[commit-bot: I haz the power, 2017-05-12 08:56:44]

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

[yosin_UTC9, 2017-05-12 09:23:31]

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

[yosin_UTC9, 2017-05-12 09:23:32]

PTAL

This patch is a preparation of fixing Release-Blocker bug.

[yosin_UTC9, 2017-05-12 09:24:20]

On 2017/05/12 at 09:23:32, yosin_UTC9 wrote:
> PTAL
> 
> This patch is a preparation of fixing Release-Blocker bug.

Correction. This patch fixes Release-Blocker bug.

[yosin_UTC9, 2017-05-12 09:27:19]

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

[commit-bot: I haz the power, 2017-05-12 09:27:41]

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-12 09:31:10]

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

[commit-bot: I haz the power, 2017-05-12 09:31:11]

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

[Xiaocheng, 2017-05-12 18:00:05]

Code change itself looks fine.

I'm wondering why contenteditable on <input> is not ignored.

https://codereview.chromium.org/2875933004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Position.cpp (right):

https://codereview.chromium.org/2875933004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Position.cpp:358: if (IsNull())
This should go to the previous patch.

[yoichio, 2017-05-15 01:41:53]

nlgtm

https://codereview.chromium.org/2875933004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Position.cpp (right):

https://codereview.chromium.org/2875933004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Position.cpp:358: if (IsNull())
I oppose this change from the math sense.
For example, Nan should not equal any number including
 Nan itself.
How about |while(position.IsValid() && position != lastposition)|
  in the caller?

[yosin_UTC9, 2017-05-15 04:23:28]

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

[commit-bot: I haz the power, 2017-05-15 04:23:43]

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-05-15 05:12:57]

PTAL

rebased.

https://codereview.chromium.org/2875933004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Position.cpp (right):

https://codereview.chromium.org/2875933004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Position.cpp:358: if (IsNull())
On 2017/05/12 at 18:00:05, Xiaocheng wrote:
> This should go to the previous patch.

Oops, I missed rebasing.

>How about |while(position.IsValid() && position != lastposition)| in the
caller?
Talked offline.

In this patch, we use Position::IsEqualivalent() as replacement of operator==()
to
minimize patch size.

We'll get rid of null-Position usage in future.

[yoichio, 2017-05-15 06:32:35]

lgtm

[yosin_UTC9, 2017-05-15 06:40:49]

On 2017/05/12 at 18:00:05, xiaochengh wrote:
> Code change itself looks fine.
> 
> I'm wondering why contenteditable on <input> is not ignored.
To do so, we need to change lots of places, e.g.

@@ -2974,11 +2974,13 @@ static PositionTemplate<Strategy>
MostBackwardCaretPosition(
     // node, to avoid the expense of computing hasEditableStyle().
     if (current_node != last_node) {
       // Don't change editability.
-      bool current_editable = HasEditableStyle(*current_node);
-      if (start_editable != current_editable) {
-        if (rule == kCannotCrossEditingBoundary)
-          break;
-        boundary_crossed = true;
+      if (!isHTMLInputElement(*current_node)) {
+        bool current_editable = HasEditableStyle(*current_node);
+        if (start_editable != current_editable) {
+          if (rule == kCannotCrossEditingBoundary)
+            break;
+          boundary_crossed = true;
+        }
       }
       last_node = current_node;
     }

I attempt to above for both MostBackwardCaretPosition() and
MostForwardCaretPosition(),
but this bug isn't fixed. :-<

I also attempt of change hasEditableStyle() to handle INPUT element and I found
lost of layout tests are failed.

When we revise visible position canonicalization, it may be good to consider
ignoring contenteditable attribute for UA shadows, INPUT, SELECT, TEXTAREA.
But, I guess usage of contenteditable attribute on UA shadows is rare case.

[commit-bot: I haz the power, 2017-05-15 06:48:30]

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

[commit-bot: I haz the power, 2017-05-15 06:48:31]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-05-15 08:22:27]

The CQ bit was checked by yosin@chromium.org

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

CQ is trying da patch.

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

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

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494836547089260,
"parent_rev": "5ffddc43be38516f89c6c631872285324778eb39", "commit_rev":
"9634436d854ad5431c48b8d3ddecd46050cbb74a"}

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

Description was changed from

==========
Utilize Position::IsEquivalent() in {Up,Down}streamIgnoringEditingBoundaries()

This patch utilized |Position::IsEquivalent()| in
|{Up,Down}streamIgnoringEditingBoundaries()| to avoid infinite loop.

In http://crbug.com/716093, |UpstreamIgnoringEditingBoundaries()| causes
infinite loop as:
- |MostBackwardCaretPosition(AfterChildren/DIV)| After/INPUT
- |MostBackwardCaretPosition(After/INPUT)| AfterChildren/DIV
for <div contenteditable>foo <input contenteditable="false"></div>

When I attempt to change |MostBackwardCaretPosition()| to return one of them,
some layout tests failed. Hence, to minimize change, this patch change loop
condition.

BUG=716093
TEST=run_webkit_unit_tests
--gtest_filter=VisibleUnitsTest.ComputeInlineBoxPositionMixedEditabl
==========

to

==========
Utilize Position::IsEquivalent() in {Up,Down}streamIgnoringEditingBoundaries()

This patch utilized |Position::IsEquivalent()| in
|{Up,Down}streamIgnoringEditingBoundaries()| to avoid infinite loop.

In http://crbug.com/716093, |UpstreamIgnoringEditingBoundaries()| causes
infinite loop as:
- |MostBackwardCaretPosition(AfterChildren/DIV)| After/INPUT
- |MostBackwardCaretPosition(After/INPUT)| AfterChildren/DIV
for <div contenteditable>foo <input contenteditable="false"></div>

When I attempt to change |MostBackwardCaretPosition()| to return one of them,
some layout tests failed. Hence, to minimize change, this patch change loop
condition.

BUG=716093
TEST=run_webkit_unit_tests
--gtest_filter=VisibleUnitsTest.ComputeInlineBoxPositionMixedEditabl

Review-Url: https://codereview.chromium.org/2875933004
Cr-Commit-Position: refs/heads/master@{#471697}
Committed:
https://chromium.googlesource.com/chromium/src/+/9634436d854ad5431c48b8d3ddec...
==========

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

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9634436d854ad5431c48b8d3ddec...