Reproducible case of backwards nextParagraph returning a position ahead of the input position

[Tim Horton]

Don't know how to write a standalone test for this yet, but:

<style>body { margin: 0; } iframe { border: none; }</style><meta name='viewport' content='initial-scale=1'><span id='the'>The</span> quick brown fox <span id='jumps'>jumps</span> over the lazy <span id='dog'>dog.</span>

And then select jumps:

getSelection().setBaseAndExtent(jumps, 0, jumps, 1)

If you take the selection start and then call positionOfNextBoundaryOfGranularity() with granularity=paragraph and direction=backwards, *twice*, you end up at the *end* of the document.

[Ryosuke Niwa]

Created attachment 365668 [details]
Bad repro

I can't reproduce this... Maybe nextParagraphBoundaryInDirection behaves differently from modify.

[Radar WebKit Bug Importer]

<rdar://problem/49135890>

[Ryosuke Niwa]

We should look into this. A bug like this can result in an infinite loop elsewhere.

[Tim Horton]

You should be able to reproduce with the API test in https://bugs.webkit.org/show_bug.cgi?id=196040 if you remove the silly check in moveByGranularity...

[Ryosuke Niwa]

Created attachment 366146 [details]
Fixes the bug

[Ryosuke Niwa]

Comment on attachment 366146 [details]
Fixes the bug

Wrong bug :(

[Tim Horton]

Aww, you had me excited

[Daniel Bates]

Hit this bug... I'm going to take a stab.

[Daniel Bates]

The issue is that WebCore::nextParagraphBoundaryInDirection() does actually handle the case where the specified position is not within a paragraph. It handle that case as if the position was on a paragraph boundary. The latter needs to be handled as well.

[Daniel Bates]

Created attachment 385645 [details]
Patch and unit test

[Daniel Bates]

Kinda wavering on the changes here...trying to figure out what's the most readable code. Left some duplication on the table to try to get something more readable. Maybe solution should be DRY-er.

[Daniel Bates]

Patch will not apply until bug #205142 lands.

[Daniel Bates]

Comment on attachment 385645 [details]
Patch and unit test

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/DocumentEditingContext.mm:631
> +    [webView synchronouslyLoadHTMLString:applyAhemStyle(@"<style>body { margin: 0; } iframe { border: none; }</style><meta name='viewport' content='initial-scale=1'><span id='the'>The</span> quick brown fox <span id='jumps'>jumps</span> over the lazy <span id='dog'>dog.</span>")];

Remove the <style> as  applyAhemStyle() adds it for us.

[Daniel Bates]

Created attachment 385779 [details]
Patch and unit test

[Wenson Hsieh]

Comment on attachment 385779 [details]
Patch and unit test

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

> Source/WebCore/ChangeLog:16
> +                        or backwards, respectively.

I think it would help to clarify what causes the last (third) scenario here — i.e. the last of the 3 return statements in nextParagraphBoundaryInDirection.

[Daniel Bates]

Comment on attachment 385779 [details]
Patch and unit test

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/DocumentEditingContext.mm:628
> +TEST(WebKit, DocumentEditingContextRequestLastTwoParagraphsWithCaretNotInParagraph)

No caret in this test. Will rename to DocumentEditingContextRequestLastTwoParagraphsWithSelectiontNotInAParagraph

[Daniel Bates]

(In reply to Wenson Hsieh from comment #15)
> Comment on attachment 385779 [details]
> Patch and unit test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385779&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +                        or backwards, respectively.
> 
> I think it would help to clarify what causes the last (third) scenario here
> — i.e. the last of the 3 return statements in
> nextParagraphBoundaryInDirection.

Nothing! There are only two cases: at boundary of paragraph and in paragraph. Somehow I wound up with a third case.

[Wenson Hsieh]

Comment on attachment 385779 [details]
Patch and unit test

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

>>> Source/WebCore/ChangeLog:16
>>> +                        or backwards, respectively.
>> 
>> I think it would help to clarify what causes the last (third) scenario here — i.e. the last of the 3 return statements in nextParagraphBoundaryInDirection.
> 
> Nothing! There are only two cases: at boundary of paragraph and in paragraph. Somehow I wound up with a third case.

Sounds like we can simplify the logic a bit, then!

[Daniel Bates]

Created attachment 385811 [details]
To land

[Daniel Bates]

Comment on attachment 385811 [details]
To land

Clearing flags on attachment: 385811

Committed r253578: <https://trac.webkit.org/changeset/253578>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.