Fix a crash by AdvanceToNextMisspelling command (⌘-;) with empty INPUT

Fix a crash by AdvanceToNextMisspelling command (⌘-;) with empty INPUT

SpellChecker::advanceToNextMisspelling() should not choose VisiblePosition pointing
a different rootEditableElement.

BUG=584986
TEST=automated

Committed: https://crrev.com/f51e256f1250f2c84ba227ac796092b65a8ec0a6
Cr-Commit-Position: refs/heads/master@{#374337}

[tkent, 2016-02-09 04:10:59]

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

[tkent, 2016-02-09 04:11:00]

yosin@, would you review this please?

[yosin_UTC9, 2016-02-09 05:06:13]

|advanceToNextMisspelling()| is too big, we may want to break it into
functions...

https://codereview.chromium.org/1679173003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/html.css (right):

https://codereview.chromium.org/1679173003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/html.css:533: -webkit-user-select: none;
It is strange that we attempt to spell check "read-only" text,
input::-webkit-input-placeholder selector sets so.
We should fix |advanceToNextMisspelling()| not to attempt checking spell for
"read-only" text rather than make placeholder unselectable to make user created
INPUT element not to crash.

[tkent, 2016-02-09 05:09:26]

https://codereview.chromium.org/1679173003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/html.css (right):

https://codereview.chromium.org/1679173003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/html.css:533: -webkit-user-select: none;
On 2016/02/09 at 05:06:13, Yosi_UTC9 wrote:
> It is strange that we attempt to spell check "read-only" text,
input::-webkit-input-placeholder selector sets so.
> We should fix |advanceToNextMisspelling()| not to attempt checking spell for
"read-only" text rather than make placeholder unselectable to make user created
INPUT element not to crash.

I see. So, the following code in SpellChecker::advanceToNextMisspelling()
doesn't check editability.  I'll investigate it.

    // If spellingSearchRange starts in the middle of a word, advance to the
    // next word so we start checking at a word boundary. Going back by one char
    // and then forward by a word does the trick.
    if (startedWithSelection) {
    VisiblePosition oneBeforeStart =
previousPositionOf(createVisiblePosition(spellingSearchStart));
        if (oneBeforeStart.isNotNull())
            spellingSearchStart =
endOfWord(oneBeforeStart).toParentAnchoredPosition();
        // else we were already at the start of the editable node
    }

[tkent, 2016-02-09 05:59:41]

Description was changed from

==========
Fix a crash by AdvanceToNextMisspelling command (⌘-;) with empty INPUT

Internal placeholder element should be unselectable.

BUG=584986
TEST=automated
==========

to

==========
Fix a crash by AdvanceToNextMisspelling command (⌘-;) with empty INPUT

SpellChecker::advanceToNextMisspelling() should not choose VisiblePosition
pointing
a different rootEditableElement.

BUG=584986
TEST=automated
==========

[tkent, 2016-02-09 05:59:54]

Update the patch.  Please take another look.

[tkent, 2016-02-09 06:17:38]

On 2016/02/09 at 05:09:26, tkent wrote:
>
https://codereview.chromium.org/1679173003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/html.css (right):
> 
>
https://codereview.chromium.org/1679173003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/html.css:533: -webkit-user-select: none;
> On 2016/02/09 at 05:06:13, Yosi_UTC9 wrote:
> > It is strange that we attempt to spell check "read-only" text,
input::-webkit-input-placeholder selector sets so.
> > We should fix |advanceToNextMisspelling()| not to attempt checking spell for
"read-only" text rather than make placeholder unselectable to make user created
INPUT element not to crash.
> 
> I see. So, the following code in SpellChecker::advanceToNextMisspelling()
doesn't check editability.  I'll investigate it.
> 
>     // If spellingSearchRange starts in the middle of a word, advance to the
>     // next word so we start checking at a word boundary. Going back by one
char
>     // and then forward by a word does the trick.
>     if (startedWithSelection) {
>     VisiblePosition oneBeforeStart =
previousPositionOf(createVisiblePosition(spellingSearchStart));
>         if (oneBeforeStart.isNotNull())
>             spellingSearchStart =
endOfWord(oneBeforeStart).toParentAnchoredPosition();
>         // else we were already at the start of the editable node
>     }

Note that spellingSearchStart points 1 at the beginning of this code, then
it points 2 by previousPositionOf(). (Actually
previousVisuallyDistinctCandidateAlgorithm() does it.)


INPUT	0xcfab75c25a8 (focused)
		#shadow-root	0xcfab75c26a8
			DIV	0xcfab75c27e0 ID="placeholder" STYLE="display: block !important;
text-overflow: clip;"
2				#text	0xcfab75c2848 "placeholder"
1			DIV	0xcfab75c2778 ID="inner-editor" (editable)

[yosin_UTC9, 2016-02-09 07:13:19]

lgtm

Thanks!

[tkent, 2016-02-09 07:14:11]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2016-02-09 07:14:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1679173003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1679173003/20001

[commit-bot: I haz the power, 2016-02-09 08:30:28]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-09 08:31:17]

Description was changed from

==========
Fix a crash by AdvanceToNextMisspelling command (⌘-;) with empty INPUT

SpellChecker::advanceToNextMisspelling() should not choose VisiblePosition
pointing
a different rootEditableElement.

BUG=584986
TEST=automated
==========

to

==========
Fix a crash by AdvanceToNextMisspelling command (⌘-;) with empty INPUT

SpellChecker::advanceToNextMisspelling() should not choose VisiblePosition
pointing
a different rootEditableElement.

BUG=584986
TEST=automated

Committed: https://crrev.com/f51e256f1250f2c84ba227ac796092b65a8ec0a6
Cr-Commit-Position: refs/heads/master@{#374337}
==========

[commit-bot: I haz the power, 2016-02-09 08:31:18]

Patchset 2 (id:??) landed as
https://crrev.com/f51e256f1250f2c84ba227ac796092b65a8ec0a6
Cr-Commit-Position: refs/heads/master@{#374337}