Bug 54534

Summary:

RTL lineboundary left/right is reversed when cursor is at start of RTL container

Product:

WebKit

Reporter:

Benjamin (Ben) Kalman <kalman>

Component:

HTML Editing

Assignee:

Benjamin (Ben) Kalman <kalman>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, commit-queue, eric, mitz, rniwa, webkit.review.bot, xji

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

Attachments:

Description	Flags
Test case	none
Patch	none
Patch	none
Patch	none
Patch for landing	none

Benjamin (Ben) Kalman

Reported 2011-02-15 23:59:43 PST

As described in the summary. Test case attached.

Attachments
Test case (1.22 KB, text/html) 2011-02-16 00:00 PST, Benjamin (Ben) Kalman	no flags	Details
Patch (8.68 KB, patch) 2011-02-16 19:56 PST, Benjamin (Ben) Kalman	no flags	Details Formatted Diff Diff
Patch (7.43 KB, patch) 2011-02-16 22:00 PST, Benjamin (Ben) Kalman	no flags	Details Formatted Diff Diff
Patch (7.99 KB, patch) 2011-02-16 22:56 PST, Benjamin (Ben) Kalman	no flags	Details Formatted Diff Diff
Patch for landing (7.99 KB, patch) 2011-02-16 23:27 PST, Benjamin (Ben) Kalman	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Benjamin (Ben) Kalman

Comment 1 2011-02-16 00:00:13 PST

Created attachment 82596 [details] Test case

Benjamin (Ben) Kalman

Comment 2 2011-02-16 19:56:33 PST

Created attachment 82744 [details] Patch

WebKit Review Bot

Comment 3 2011-02-16 19:59:23 PST

Attachment 82744 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 LayoutTests/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ryosuke Niwa

Comment 4 2011-02-16 20:19:50 PST

Comment on attachment 82744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82744&action=review The change looks fine but r- because you should address at least some of the style issues below: > LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:2 > + I would personally put <body> than a blank line > LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:9 > + > +<pre id="console"></pre> > + Are these blank lines needed? > LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:14 > + function log(text) { > + document.getElementById("console").appendChild(document.createTextNode(text + "\n")); > + } > + I don't think we need to indent all these functions. It just bloats the patch/file size. > LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:31 > + assertEquals("", getSelection().toString(), "extend left from left"); Isn't it better to check the offset values here? For example, this wouldn't detect when the caret moved to the opposite side, right? > LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:51 > + for (var i = 0; i < testContainers.length; i++) { > + runTestForContainer(testContainers[i]); > + } No curly brackets around one line statements. > LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:56 > + while (testContainers.length > 0) { > + document.body.removeChild(testContainers[0]); > + } Ditto. > Source/WebCore/editing/SelectionController.cpp:421 > - if (directionOfEnclosingBlock() == LTR) > - pos = pos.next(true); > - else > - pos = pos.previous(true); > + pos = (directionOfEnclosingBlock() == LTR) ? pos.next(true) : pos.previous(true); I'm not sure if we should use ternary expression here. It seems like we use explicit if/else expressions everywhere else in this file.

Ryosuke Niwa

Comment 5 2011-02-16 20:20:40 PST

Comment on attachment 82744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82744&action=review > Source/WebCore/editing/SelectionController.cpp:570 > - if (directionOfEnclosingBlock() == LTR) > - pos = pos.previous(true); > - else > - pos = pos.next(true); > + pos = (directionOfEnclosingBlock() == LTR) ? pos.previous(true) : pos.next(true); Ditto about ternary expression.

Benjamin (Ben) Kalman

Comment 6 2011-02-16 21:51:30 PST

Comment on attachment 82744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82744&action=review >> LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:2 >> + > > I would personally put <body> than a blank line but then i need a body at the end. it's much of a muchness and blank lines are less noisy than <body> tags. >> LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:9 >> + > > Are these blank lines needed? no... but i think it's neater to separate the test divs from the log? >> LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:31 >> + assertEquals("", getSelection().toString(), "extend left from left"); > > Isn't it better to check the offset values here? For example, this wouldn't detect when the caret moved to the opposite side, right? I suppose not, but that sort of bug would surely be picked up in lots of other layout tests i.e. it's equivalent to a modify("extend") behaving like modify("move"). Doing it this way gives a nice FAIL output statement though, in assertEquals. >> Source/WebCore/editing/SelectionController.cpp:421 >> + pos = (directionOfEnclosingBlock() == LTR) ? pos.next(true) : pos.previous(true); > > I'm not sure if we should use ternary expression here. It seems like we use explicit if/else expressions everywhere else in this file. I like ternary ifs in situations like this, it's cleaner and semantically more correct i.e. "assign a value to pos; the value depends on whether it's LTR or RTL" is functional, whereas using if/else is procedural. That's somewhat of a more general philosophic point though, and you're right that this makes it unlike the rest of the file, so I don't mind changing it (back) to using if/else. That said, this whole file could be significantly less bulky if all the assignments were changed to be like this. Maybe save 50 lines? There are other examples too, like if (directionOfEnclosingBlock() == LTR) baseIsStart = true; else baseIsStart = false; break; (line 348) could/should just be baseIsStart = directionOfEnclosingBlock() == LTR;

Benjamin (Ben) Kalman

Comment 7 2011-02-16 22:00:57 PST

Created attachment 82756 [details] Patch

Ryosuke Niwa

Comment 8 2011-02-16 22:15:07 PST

(In reply to comment #6) > but then i need a body at the end. it's much of a muchness and blank lines are less noisy than <body> tags. ok. > >> LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:9 > >> + > > > > Are these blank lines needed? > > no... but i think it's neater to separate the test divs from the log? I don't think so. It just adds noise especially a blank line immediately after DOCTYPE. > >> LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:31 > >> + assertEquals("", getSelection().toString(), "extend left from left"); > > > > Isn't it better to check the offset values here? For example, this wouldn't detect when the caret moved to the opposite side, right? > > I suppose not, but that sort of bug would surely be picked up in lots of other layout tests i.e. it's equivalent to a modify("extend") behaving like modify("move"). Doing it this way gives a nice FAIL output statement though, in assertEquals. I'm not sure. For example, if the selection was changed from [text, 0] to [div, 0] where we have div > text, then toString returns the same result. I don't think the verification we have is strong enough. We should make sure to catch any small regression there could be. > That's somewhat of a more general philosophic point though, and you're right that this makes it unlike the rest of the file, so I don't mind changing it (back) to using if/else. Right. We should at least do that in a separate patch but I'm not sure if everyone agrees with you. I remember someone objected it when I proposed that change before. View in context: https://bugs.webkit.org/attachment.cgi?id=82756&action=review > LayoutTests/ChangeLog:8 > + Add regression test. You should explain what kind of a test you're adding here.

Benjamin (Ben) Kalman

Comment 9 2011-02-16 22:56:48 PST

Created attachment 82757 [details] Patch

Ryosuke Niwa

Comment 10 2011-02-16 23:04:22 PST

Comment on attachment 82757 [details] Patch Thanks for fixing this bug!

Benjamin (Ben) Kalman

Comment 11 2011-02-16 23:27:57 PST

Created attachment 82760 [details] Patch for landing

WebKit Commit Bot

Comment 12 2011-02-17 01:02:45 PST

Comment on attachment 82760 [details] Patch for landing Clearing flags on attachment: 82760 Committed r78799: <http://trac.webkit.org/changeset/78799>

WebKit Commit Bot

Comment 13 2011-02-17 01:02:50 PST

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 14 2011-02-17 03:47:28 PST

http://trac.webkit.org/changeset/78799 might have broken GTK Linux 32-bit Debug

Note You need to log in before you can comment on or make changes to this bug.