Bug 61346 – --webkit-visual-word: ctrl-arrow is not able to reach the boundary of line

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Xiaomei Ji]

for example: "abc def hij", the word breaks when press ctrl-right-arrow are "abc |def |hij",
press ctrl-right-arrow after the rightmost word break wont have any effect.

in windows native text system, the word breaks are "abc |def |hij|".

Firefox's behavior in windows needs some verification.

[Xiaomei Ji]

to be more accurate.
In LTR context, ctrl-right-arrow is not able to reach the visually rightmost of line (currently, ctrl-right-arrow only stops at the left boundary of a word).
In RTL context, ctrl-left-arrow is not able to reach the visually leftmost of line (currently, ctrl-left-arrow only stops at the right boundary of a word).

[Yair Yogev]

(In reply to comment #0)
> Firefox's behavior in windows needs some verification.

when i asked in the Mozilla support forum, 3 users reported reaching the boundary and one reported the opposite.
seems to be a bug in FF since:
1. we couldn't figure out what makes it act differently in different computers
2. in all other browsers/software Ctrl+arrow can always reach the boundary.

might be worth to note here that FF is also the only one among the browsers not stopping at line breaks.

[Xiaomei Ji]

(In reply to comment #2)
> (In reply to comment #0)
> > Firefox's behavior in windows needs some verification.
> 
> when i asked in the Mozilla support forum, 3 users reported reaching the boundary and one reported the opposite.
> seems to be a bug in FF since:
> 1. we couldn't figure out what makes it act differently in different computers
> 2. in all other browsers/software Ctrl+arrow can always reach the boundary.
> 
Thanks for the information!

> might be worth to note here that FF is also the only one among the browsers not stopping at line breaks.

What do you mean by "not stopping at line breaks"? example?

[Yair Yogev]

(In reply to comment #3)
> What do you mean by "not stopping at line breaks"? example?

as we discussed before-
in a two lined text area (for example), "|" representing caret stop positions when pressing Ctrl+Right from the beginning of the first line:
IE8 & Opera11:
|word |word|
|word |word|
while FF does for some:
|word |word
|word |word|
and FF for others apparently:
|word |word
|word |word


when i wrote line breaks i referred to the end of the first line, i don't know what is best here, all i know is that FF seems to be in minority by not stopping there.

but what we are talking about in this bug is (i think) the "text boundary":
the last position of the second line (aka eventually the caret should always reach the Ctrl+End position when Ctrl+Right is pressed long enough)

[Van Lam]

Created attachment 100372 [details]
Proposed fix

[Ryosuke Niwa]

Comment on attachment 100372 [details]
Proposed fix

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

I'm concerned that this code is really losing the original benefit of not doing linear search.  I feel like we can get a cleaner & more efficient code by collecting all word break in a visual position.

> Source/WebCore/ChangeLog:6
> +	Added the word boundary corresponding to the logical end-of-line to the
> +	word boundary vector; previously, this word boundary wasn't collected.

Nit: Tab.  Also, the description should appear after a blank line following "Reviewed by NOBODY" (followed by a blank line before "* editing/visible_units.cpp:").

> Source/WebCore/editing/visible_units.cpp:1347
> +            return Position(static_cast<Text*>(currentLTRBox->renderer()->node()), currentLTRBox->caretMaxOffset());

This isn't really the leftmost position, is it?  This is the leftmost position in the current LTR run, right?

> Source/WebCore/editing/visible_units.cpp:1447
> +    // Check if box is visually last in block; if so, append the end of line
> +    // position to the word break vector

This comment repeats the code.

> Source/WebCore/editing/visible_units.cpp:1448
> +    if (isLastVisualBoxInBlock(box, blockDirection)) {

Perhaps you want to call this function isBoxVisuallyLastInBlock?

> Source/WebCore/editing/visible_units.cpp:1482
> +        WordBoundaryEntry wordBoundaryEntry(endOfBlock, offsetOfEndOfBlock);
> +        orderedWordBoundaries.append(wordBoundaryEntry);

Nit: can't we just do orderedWordBoundaries.append(WordBoundaryEntry(endOfBlock, offsetOfEndOfBlock)); ?

> Source/WebCore/editing/visible_units.cpp:1517
> -    }        
> +    }

Please revert these whitespace changes.  It's bloating the patch needlessly.

> Source/WebCore/editing/visible_units.cpp:-1426
>      return VisiblePosition();
>  }
> -    

Ditto.

> Source/WebCore/editing/visible_units.cpp:1635
> +    // Check for empty div edge case.

This comment repeats what code says.

> Source/WebCore/editing/visible_units.cpp:1710
> -}
> +} // namespace WebCore

Let's not include these changes in one patch.

> LayoutTests/ChangeLog:5
> +	Modified two tests to test for visual word movement to end-of-line.

Nit: Tab

[Xiaomei Ji]

Comment on attachment 100372 [details]
Proposed fix

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

> Source/WebCore/ChangeLog:5
> +	Added the word boundary corresponding to the logical end-of-line to the

indent.

> Source/WebCore/editing/visible_units.cpp:1442
> +static void collectWordBreaksInBoxInsideBlockWithSameDirectionality(const InlineBox* box, WordBoundaryVector& orderedWordBoundaries, TextDirection blockDirection)

you do not need to pass blockDirection. You can use the box direction since the box direction == block direction in this function.

> Source/WebCore/editing/visible_units.cpp:1469
> +static void collectWordBreaksInBoxInsideBlockWithDifferntDirectionality(const InlineBox* box, WordBoundaryVector& orderedWordBoundaries, TextDirection blockDirection)

no need to pass blockDirection

> Source/WebCore/editing/visible_units.cpp:1637
> +        return VisiblePosition();

this and the corresponding test are covered in https://bugs.webkit.org/attachment.cgi?id=98417&action=review, so I think you can remove it from this patch.

> Source/WebCore/editing/visible_units.cpp:1680
> +        return VisiblePosition();

ditto

> LayoutTests/ChangeLog:5
> +	Modified two tests to test for visual word movement to end-of-line.

indent

> LayoutTests/editing/selection/move-by-word-visually-single-space-sigle-line-expected.txt:98
>  Test 20, RTL:

the html file seems changed, but why the test expectation is not change?
same for test 21.

[Xiaomei Ji]

(In reply to comment #6)
> (From update of attachment 100372 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100372&action=review
> 
> I'm concerned that this code is really losing the original benefit of not doing linear search.  I feel like we can get a cleaner & more efficient code by collecting all word break in a visual position.

The word breaks are still visually ordered.
For example, "abc def" in LTR context,
previously, only word break "abc |def" and "|abc def" are collected, and they are collected visually right to left. 
Now, word break "abc def|" is collected at first, so the word breaks are still visually ordered as before.

The only difference is that:
for LTR block, the rightmost line boundary is collect as a word break in rightmost box. 
for RTL block, the leftmost line boundary is collect as a word break in leftmost box.

Since logicalEndOfLine might be costly, we are still working on finding a general solution to avoid using logicalEndOfLine to get the rightmost/leftmost line boundary position. Maybe by checking box's bidi-level (and only cover bidi-level 0, 1, 2, 3 for now).

[Xiaomei Ji]

(In reply to comment #7)
> (From update of attachment 100372 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100372&action=review

> 
> > Source/WebCore/editing/visible_units.cpp:1442
> > +static void collectWordBreaksInBoxInsideBlockWithSameDirectionality(const InlineBox* box, WordBoundaryVector& orderedWordBoundaries, TextDirection blockDirection)
> 
> you do not need to pass blockDirection. You can use the box direction since the box direction == block direction in this function.
> 

Ah, I see it is used in endOfBlockPosition() etc. callees.

[Van Lam]

Created attachment 100392 [details]
Revised fix

[Van Lam]

Need to revise again following comments from Xiaomei; one second!

(In reply to comment #10)
> Created an attachment (id=100392) [details]
> Revised fix

[Xiaomei Ji]

Comment on attachment 100372 [details]
Proposed fix

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

>> Source/WebCore/editing/visible_units.cpp:1347
>> +            return Position(static_cast<Text*>(currentLTRBox->renderer()->node()), currentLTRBox->caretMaxOffset());
> 
> This isn't really the leftmost position, is it?  This is the leftmost position in the current LTR run, right?

it is the leftmost position of the line since current run is the leftmost run in the line.

[Van Lam]

Created attachment 100398 [details]
Revised fix (ignore previous)

[Van Lam]

Created attachment 100399 [details]
Revised fix (ignore previous)

Sorry about that, forgot to mark as patch.

[Xiaomei Ji]

Comment on attachment 100399 [details]
Revised fix (ignore previous)

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

> LayoutTests/editing/selection/move-by-word-visually-single-space-sigle-line-expected.txt:-100
> -"abc def "[0], " rst uvw"[4], "hij opq"[3], "abc def "[7, 3]    FAIL expected: ["abc def "[ 0, ]" rst uvw"[ 4, ]"hij opq"[ 7,  3, ]"abc def "[ 7,  3]

the expectation seems does not match those in html file. same for test 21 and 31.
in the html file, position "rst uvw"[8] is added as a word boundary, then, why it is not showed as a word boundary in "FAIL expected:"?

[Van Lam]

Created attachment 100504 [details]
Revised fix (updated layout test expectations)

[Van Lam]

Created attachment 102167 [details]
Proposed fix with updated test cases

[Van Lam]

Created attachment 102185 [details]
Proposed fix with updated test cases

Test cases were split from 2 files to 4 between the time I updated and submitted the previous patch; here's my updated patch.

[Ryosuke Niwa]

Comment on attachment 102185 [details]
Proposed fix with updated test cases

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

> Source/WebCore/editing/visible_units.cpp:1188
> +static VisiblePosition leftmostPositionInRTLBoxInRTLBlock(const InlineBox* box)

You should probably add inline keyboard here.

> Source/WebCore/editing/visible_units.cpp:1218
> +        if (box->bidiLevel() == 1) {

I don't think we should be checking that the bidi level is 1.  If anything it should be checking that it's odd.  r- because of this.

> Source/WebCore/editing/visible_units.cpp:1257
> +    if (needToUseLogicalEndOfLine(box, blockDirection))
> +        return logicalEndOfLine(createPositionAvoidingIgnoredNode(box->renderer()->node(), box->caretMaxOffset()));
> +    
> +    return endOfBoxPosition(box, blockDirection);

I don't understand the difference between logicalEndOfLine and endOfBoxPosition.  We need to give them more descriptive name such that readers can make sense of this code without having to read the code in those two functions.

> Source/WebCore/editing/visible_units.cpp:1262
> +    return blockDirection == LTR ? !box->nextLeafChild() || box->nextLeafChild()->renderer()->isBR() : !box->prevLeafChild() || box->prevLeafChild()->renderer()->isBR();

Nit: I'd split this into two lines.

> Source/WebCore/editing/visible_units.cpp:1460
> +        endOfBlock.getInlineBoxAndOffset(boxOfEndOfBlock, offsetOfEndOfBlock);

positionIsInBox calls getInlineBoxAndOffset.  r- because of this.

> Source/WebCore/editing/visible_units.cpp:1485
> +        endOfBlock.getInlineBoxAndOffset(boxOfEndOfBlock, offsetOfEndOfBlock);

Ditto.

[Ryosuke Niwa]

Comment on attachment 102185 [details]
Proposed fix with updated test cases

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

> Source/WebCore/editing/visible_units.cpp:1245
> +static VisiblePosition endOfBoxPosition(const InlineBox* box, TextDirection blockDirection)

Nit: Did you mean endPositionInBox?  Also, I don't see much point in having this as a separate function.  It just bloats the code.  I'd much rather have this code right in endOfLinePosition so that I can see the optimization there.

[Xiaomei Ji]

In some cases, when we are not able to get the visual end of line correctly, we are using logicalEndOfLine to get the line boundary instead.

logicalEndOfLine() is doing the reverse of bidi run reordering, and its result is correct. But it might have performance hit.

needToUseLogicalEndOfLine() is introduced to avoid using logicalEndOfLine() for simple cases.

I think we might need to spend more time to check whether we are able to get the left/right boundary of line by checking the box and its previous/next boxes' bidi level if bidi levels are 0, 1, 2 3.

[Van Lam]

Created attachment 102216 [details]
Revised fix (with updated test cases)

Revised fix without optimizing (so I'm just calling logicalEndOfLine to compute the end-of-line position). This is correct but there may be a performance hit as Xiaomei mentioned; will profile and look into optimizations if necessary soon.

[Van Lam]

Created attachment 102217 [details]
Revised fix (with updated test cases)

Fixed changelog entry

[Van Lam]

Created attachment 102219 [details]
Revised fix (with updated test cases)

Forgot to mark as review

[Xiaomei Ji]

Comment on attachment 102219 [details]
Revised fix (with updated test cases)

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

> Source/WebCore/editing/visible_units.cpp:1402
> +    

the above 2 newly added code pieces are similar, maybe they could be combined as one function? and logical of isBoxVisuallyLastInLine can be moved inside the function as well.

[Ryosuke Niwa]

Comment on attachment 102219 [details]
Revised fix (with updated test cases)

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

> Source/WebCore/editing/visible_units.cpp:1179
> +                                   !box->prevLeafChild() || box->prevLeafChild()->renderer()->isBR();

Nit: We don't align second lines like this.  This line should be intended by 4 spaces in addition to the 4 spaces in front of "return".

>> Source/WebCore/editing/visible_units.cpp:1402
>> +    
> 
> the above 2 newly added code pieces are similar, maybe they could be combined as one function? and logical of isBoxVisuallyLastInLine can be moved inside the function as well.

I agree with Xiaomei here.  We should at least create a function for the code inside the if.  r- because of this code duplication.

[Van Lam]

Created attachment 102554 [details]
Revised fix (with updated test cases)

Extracted the duplicate code into a new function appendEndOfLinePosition and fixed indentation problem.

[Darin Adler]

Comment on attachment 102554 [details]
Revised fix (with updated test cases)

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

Not doing a review, but I do have a couple small formatting comments.

> Source/WebCore/editing/visible_units.cpp:1179
> +    return blockDirection == LTR ? !box->nextLeafChild() || box->nextLeafChild()->renderer()->isBR() :
> +        !box->prevLeafChild() || box->prevLeafChild()->renderer()->isBR();

Our normal formatting is to put the ":" on the new line, not on the end of the previous line.

> Source/WebCore/editing/visible_units.cpp:1371
> +    int offsetOfEndOfBlock;

Seems you should define this just before using it, not at the top of the function.

[Van Lam]

Thanks Darin for the feedback; I'll resubmit with the changes.

[Van Lam]

Created attachment 102565 [details]
Revised fix (with updated test cases)

Made changes following Darin's suggestions.

[Ryosuke Niwa]

Comment on attachment 102565 [details]
Revised fix (with updated test cases)

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

> Source/WebCore/editing/visible_units.cpp:1368
> +static void appendEndOfLinePosition(const InlineBox* box, WordBoundaryVector& orderedWordBoundaries)

appendPositionAtLogicalEndOfLine?  EndOfLinePosition sounds funny.

[Ryosuke Niwa]

Is Xiaomei landing the patch on behalf of you?  If not, you should ask for cq+ as well.

[Van Lam]

Created attachment 102571 [details]
Revised fix (with updated test cases)

Got r+ already from rniwa. Just changed function name from appendEndOfLinePosition to appendPositionAtLogicalEndOfLine and set request flag for commit-queue.

[Ryosuke Niwa]

(In reply to comment #33)
> Created an attachment (id=102571) [details]
> Revised fix (with updated test cases)
> 
> Got r+ already from rniwa. Just changed function name from appendEndOfLinePosition to appendPositionAtLogicalEndOfLine and set request flag for commit-queue.

The patch says no flags?

[Van Lam]

I just unset the flag following a request from Xiaomei to wait until tomorrow morning.

[Ryosuke Niwa]

(In reply to comment #35)
> I just unset the flag following a request from Xiaomei to wait until tomorrow morning.

Okay.

[WebKit Review Bot]

Comment on attachment 102571 [details]
Revised fix (with updated test cases)

Rejecting attachment 102571 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Xiaomei Ji]

Comment on attachment 102571 [details]
Revised fix (with updated test cases)

the only difference of this patch from the one r+-ed is the change of function name per review comment

[WebKit Review Bot]

Comment on attachment 102571 [details]
Revised fix (with updated test cases)

Rejecting attachment 102571 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 1

Last 500 characters of output:
s/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9289746

[WebKit Review Bot]

Comment on attachment 102571 [details]
Revised fix (with updated test cases)

Clearing flags on attachment: 102571

Committed r92223: <http://trac.webkit.org/changeset/92223>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.