Bug 131475 - Autocorrection causes ASSERT when replacing alternative string
Summary: Autocorrection causes ASSERT when replacing alternative string
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 131296 131590 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-04-09 21:06 PDT by Myles C. Maxfield
Modified: 2014-04-14 10:00 PDT (History)
11 users (show)

See Also:

Attachments
Patch (6.70 KB, patch)
2014-04-09 21:12 PDT, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Patch (4.74 KB, patch)
2014-04-10 12:00 PDT, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Patch (4.84 KB, patch)
2014-04-10 15:05 PDT, Myles C. Maxfield 		rniwa: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-04-09 21:06:44 PDT 
Autocorrection causes ASSERT when replacing alternative string
Comment 1 Myles C. Maxfield 2014-04-09 21:12:19 PDT 
Created attachment 229022 [details]
Patch
Comment 2 Myles C. Maxfield 2014-04-09 21:17:54 PDT 
<rdar://problem/16284891>
Comment 3 Myles C. Maxfield 2014-04-09 21:18:45 PDT 
This test is currently broken; I'm not sure how to bring up the autocorrect popup. I'll figure that out tomorrow and then update the test.
Comment 4 Myles C. Maxfield 2014-04-10 12:00:31 PDT 
Created attachment 229063 [details]
Patch
Comment 5 Myles C. Maxfield 2014-04-10 12:01:10 PDT 
This can be tested automatically once https://bugs.webkit.org/show_bug.cgi?id=131502 gets fixed.
Comment 6 Ryosuke Niwa 2014-04-10 13:08:59 PDT 
Comment on attachment 229063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229063&action=review

> Source/WebCore/editing/AlternativeTextController.cpp:278
> -    int paragraphStartIndex = TextIterator::rangeLength(Range::create(*m_frame.document(), m_frame.document(), 0, paragraphRangeContainingCorrection.get()->startContainer(), paragraphRangeContainingCorrection.get()->startOffset()).get());
> +    // Find the root container for the node, so we can create a range from that point
> +    Node* rootNode = paragraphRangeContainingCorrection.get()->startContainer();
> +    ContainerNode* rootContainer = isContainerNode(*rootNode) ? toContainerNode(rootNode) : rootNode->parentNode();
> +    while (rootContainer->parentNode())
> +        rootContainer = rootContainer->parentNode();

A better way to do this will be obtaining the tree scope's rootNode.

> ManualTests/autocorrection/autocorrection-accept-crash.html:4
> +<head>
> +</head>

We don't need head.

> ManualTests/autocorrection/autocorrection-accept-crash.html:12
> +<p>
> +<ol>
> +<li>Type "word loadp" in the first box.</li>
> +<li>When the suggestion popup appears, click in the second box.</li>
> +</ol>
> +The test is successful if there is no crash.
> +</p>

p can't contain ol.  Also, please describe what we're testing.

> ManualTests/autocorrection/autocorrection-accept-crash.html:18
> +<p>
> +<input id="t" type="text" spellCheck="true">
> +</p>
> +<p>
> +<textarea id="a"></textarea>
> +</p>

I don't think we need all these p's.
Comment 7 Myles C. Maxfield 2014-04-10 15:05:22 PDT 
Created attachment 229081 [details]
Patch
Comment 8 Ryosuke Niwa 2014-04-11 13:59:01 PDT 
Comment on attachment 229081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229081&action=review

> ManualTests/autocorrection/autocorrection-accept-crash.html:18
> +<ol>
> +<li>Type "word loadp" in the first box.</li>
> +<li>When the suggestion popup appears, click in the second box.</li>
> +</ol>

This instruction should come first.
Comment 9 Ryosuke Niwa 2014-04-11 13:59:28 PDT 
Comment on attachment 229081 [details]
Patch

r=me assuming the test is fixed.
Comment 10 Ryosuke Niwa 2014-04-11 14:00:11 PDT 
Comment on attachment 229081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229081&action=review

> Source/WebCore/editing/AlternativeTextController.cpp:275
> +    int paragraphStartIndex = TextIterator::rangeLength(Range::create(*m_frame.document(), &rootNode, 0, paragraphRangeContainingCorrection.get()->startContainer(), paragraphRangeContainingCorrection.get()->startOffset()).get());

You can use rootNode.document().
Comment 11 Myles C. Maxfield 2014-04-11 14:37:40 PDT 
http://trac.webkit.org/changeset/167151
Comment 12 David Kilzer (:ddkilzer) 2014-04-11 15:16:30 PDT 
(In reply to comment #11)
> http://trac.webkit.org/changeset/167151

Build fix in r167160:  <http://trac.webkit.org/changeset/167160>
Comment 13 Alexey Proskuryakov 2014-04-14 09:59:57 PDT 
*** Bug 131296 has been marked as a duplicate of this bug. ***
Comment 14 Alexey Proskuryakov 2014-04-14 10:00:09 PDT 
*** Bug 131590 has been marked as a duplicate of this bug. ***