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.
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).
(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.
(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?
(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)
Created attachment 100372 [details] Proposed fix
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
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.
(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).
(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.
Created attachment 100392 [details] Revised fix
Need to revise again following comments from Xiaomei; one second! (In reply to comment #10) > Created an attachment (id=100392) [details] > Revised fix
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.
Created attachment 100398 [details] Revised fix (ignore previous)
Created attachment 100399 [details] Revised fix (ignore previous) Sorry about that, forgot to mark as patch.
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:"?
Created attachment 100504 [details] Revised fix (updated layout test expectations)
Created attachment 102167 [details] Proposed fix with updated test cases
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.
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.
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.
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.
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.
Created attachment 102217 [details] Revised fix (with updated test cases) Fixed changelog entry
Created attachment 102219 [details] Revised fix (with updated test cases) Forgot to mark as review
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.
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.
Created attachment 102554 [details] Revised fix (with updated test cases) Extracted the duplicate code into a new function appendEndOfLinePosition and fixed indentation problem.
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.
Thanks Darin for the feedback; I'll resubmit with the changes.
Created attachment 102565 [details] Revised fix (with updated test cases) Made changes following Darin's suggestions.
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.
Is Xiaomei landing the patch on behalf of you? If not, you should ask for cq+ as well.
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.
(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?
I just unset the flag following a request from Xiaomei to wait until tomorrow morning.
(In reply to comment #35) > I just unset the flag following a request from Xiaomei to wait until tomorrow morning. Okay.
Comment on attachment 102571 [details] Revised fix (with updated test cases) Rejecting attachment 102571 [details] from commit-queue. vanlam@google.com 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.
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
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
Comment on attachment 102571 [details] Revised fix (with updated test cases) Clearing flags on attachment: 102571 Committed r92223: <http://trac.webkit.org/changeset/92223>
All reviewed patches have been landed. Closing bug.