Bug 238621 – Moving forwards and backwards by sentences across a table results in different breaks in each direction.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Megan Gardner]

Created attachment 456260 [details]
Sample page showing issue.

Summary:
Moving by sentence across a table results in different breaks in each direction.

Steps To Reproduce:
1. Load up attached test page in Safari.
2. Click at the beginning of the table.
3. Press the ‘forward sentence’ button.
4. Note that the caret is after the table now
5. Press the ‘backward sentence’ button.
6. Note that it stops at the front of each cell.

Results:
The results should be the same in each direction.

Regression:
I’m not sure this has ever worked. But we are running into issues more now that we’re trying to get more context for autocorrection.

[Megan Gardner]

rdar://91115769

[Megan Gardner]

Created attachment 456290 [details]
Patch

[Darin Adler]

Comment on attachment 456290 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Writing a test while checking this against the bots.

Looking forward to significant test coverage to this under-tested code.

Besides the sentence move testing, we should test the backwards text iterator directly using the internals.rangeAsTextUsingBackwardsTextIterator function. I think we could make some really good coverage of all sorts of cases that way.

> Source/WebCore/editing/TextIterator.cpp:1370
> +    if (shouldEmitTabBeforeNode(*m_node)) {
> +        // It is important to emit tabs in tables, so we need to make sure to emit those explicitly instead of just defaulting to linefeed breaks in order to traverse it correctly.
> +        unsigned index = m_node->computeNodeIndex();
> +        emitCharacter('\t', *m_node->parentNode(), index + 1, index + 1);
> +    }

To match the logic in the forward iterator, this tab check should be done first, and we should use else and only do the newline if we didn’t already do the tab.

The comments in the code about newlines above apply to tabs as well, so the comment should be copied and pasted, or we should refactor to share code.

I’m not sure we need the new comment saying "It is important to emit tabs in tables". It seems really straightforward to say that if shouldEmitTabBeforeNode is true, we should emit a tab. Seems more that we need a comment if we did not do that!

> Source/WebCore/editing/TextIterator.cpp:1385
> +    if (shouldEmitTabBeforeNode(*m_node)) {
> +        // It is important to emit tabs in tables, so we need to make sure to emit those explicitly instead of just defaulting to linefeed breaks in order to traverse it correctly.
> +        emitCharacter('\t', *m_node, 0, 0);
> +    }

Ditto, all the same comments apply.

[Megan Gardner]

Comment on attachment 456290 [details]
Patch

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

>> Source/WebCore/editing/TextIterator.cpp:1370
>> +    }
> 
> To match the logic in the forward iterator, this tab check should be done first, and we should use else and only do the newline if we didn’t already do the tab.
> 
> The comments in the code about newlines above apply to tabs as well, so the comment should be copied and pasted, or we should refactor to share code.
> 
> I’m not sure we need the new comment saying "It is important to emit tabs in tables". It seems really straightforward to say that if shouldEmitTabBeforeNode is true, we should emit a tab. Seems more that we need a comment if we did not do that!

I'm not sure what you mean by "The comments in the code about newlines above apply to tabs as well, so the comment should be copied and pasted, or we should refactor to share code." can you elaborate/specify?

[Megan Gardner]

Created attachment 456294 [details]
Patch

[Darin Adler]

Comment on attachment 456290 [details]
Patch

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

>>> Source/WebCore/editing/TextIterator.cpp:1370
>>> +    }
>> 
>> To match the logic in the forward iterator, this tab check should be done first, and we should use else and only do the newline if we didn’t already do the tab.
>> 
>> The comments in the code about newlines above apply to tabs as well, so the comment should be copied and pasted, or we should refactor to share code.
>> 
>> I’m not sure we need the new comment saying "It is important to emit tabs in tables". It seems really straightforward to say that if shouldEmitTabBeforeNode is true, we should emit a tab. Seems more that we need a comment if we did not do that!
> 
> I'm not sure what you mean by "The comments in the code about newlines above apply to tabs as well, so the comment should be copied and pasted, or we should refactor to share code." can you elaborate/specify?

You copied the code, calling computeNodeIndex and emitCharacter, but not the comments that explain why we call computeNodeIndex ("Corresponds to the same check in TextIterator::exitNode") or gives context to emitCharacter ("The start of this emitted range is wrong [...]"). Need to copy the comments if copying the code.

[Megan Gardner]

Created attachment 456299 [details]
Patch

[Darin Adler]

Comment on attachment 456299 [details]
Patch

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

I think we should add a lot more backwards text iterator tests instead of only testing it indirectly like this. But maybe not in this patch I guess.

> Source/WebCore/editing/TextIterator.cpp:1356
> +    

Should drop that that blank line.

> Source/WebCore/editing/TextIterator.cpp:1364
> +        // A linefeed breaks words, sentences, and paragraphs.

I know you kept this comment cut down from the longer old one. But the code is now doing something so obvious I think we can leave even this out. Basically code now says "if we should emit a newline, emit a newline".

[Darin Adler]

Comment on attachment 456299 [details]
Patch

Looks like the change affected the result of editing/text-iterator/backwards-text-iterator-basic.html; that test probably needs to change.

[Megan Gardner]

Created attachment 456401 [details]
Patch

[Megan Gardner]

Created attachment 456677 [details]
Patch for landing

[EWS]

Committed r292381 (249246@main): <https://commits.webkit.org/249246@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456677 [details].