[Webkit-unassigned] [Bug 107171] shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom should return false on Android

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=107171





--- Comment #18 from Ryosuke Niwa <rniwa at webkit.org>  2013-01-18 15:12:10 PST ---
(From update of attachment 183554)
View in context: https://bugs.webkit.org/attachment.cgi?id=183554&action=review

> LayoutTests/ChangeLog:12
> +        * editing/selection/script-tests/test-if-should-move-caret-to-horizontal-boundary-forced-false.js: Added.
> +        (checkSelection):
> +        (clickShouldResultInRange):
> +        * editing/selection/test-if-should-move-caret-to-horizontal-boundary-forced-false-expected.txt: Added.
> +        * editing/selection/test-if-should-move-caret-to-horizontal-boundary-forced-false.html: Added.

Please don't add a separate js file for the script tests. That's the old way. The new way is to put the script in the single html file.
Also, this test will have a different result on Windows as well as Chromium Windows so you'll need to add test expectations there as well.

> LayoutTests/editing/selection/script-tests/test-if-should-move-caret-to-horizontal-boundary-forced-false.js:40
> +function clickShouldResultInRange(x, y, offsetIfNormal, offsetIfForced) {
> +    if (window.eventSender) {
> +        clickAt(x, y);
> +        checkSelection(offsetIfNormal, offsetIfForced);
> +    }
> +}

A more idiomatic way of writing a test case like this is to add a helper function to verify things and call both of these functions inside assertEqual. e.g.
assertEqual("clickAt(pointImmediatelyAfterText); locationOfCaretInTextNode()", "0");

But this isn't really great because the output depends on the platform.
You might want your test to look something like:
evalAndLog(" clickAt(pointImmediatelyAfterText);");
debug("Caret location:" + caretLocation());

> LayoutTests/editing/selection/test-if-should-move-caret-to-horizontal-boundary-forced-false-expected.txt:10
> +PASS EditingBehavior::shouldMove[...] not forced to false. Selection is [anchorNode: [object Text](XX) anchorOffset: 0 focusNode: [object Text](XX) focusOffset: 0 isCollapsed: true]
> +PASS EditingBehavior::shouldMove[...] not forced to false. Selection is [anchorNode: [object Text](XX) anchorOffset: 0 focusNode: [object Text](XX) focusOffset: 0 isCollapsed: true]
> +PASS EditingBehavior::shouldMove[...] not forced to false. Selection is [anchorNode: [object Text](XX) anchorOffset: 2 focusNode: [object Text](XX) focusOffset: 2 isCollapsed: true]
> +PASS EditingBehavior::shouldMove[...] not forced to false. Selection is [anchorNode: [object Text](XX) anchorOffset: 2 focusNode: [object Text](XX) focusOffset: 2 isCollapsed: true]
> +PASS successfullyParsed is true

Looking at this output, I can't tell what it's testing and why those outputs are correct.
The test output should answer questions.
Also, it's not clear to me using testPassed/testFailed is the right thing to do in this test given that the output depends on the platform.

> LayoutTests/platform/chromium-android/TestExpectations:114
> +# On Android, EditingBehavior::shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom()
> +# always returns false. That breaks these tests.
> +# https://bugs.webkit.org/show_bug.cgi?id=105574
> +# This may be revisited after http://crbug.com/135959 is finished.
> +editing/selection/click-in-margins-inside-editable-div.html [ WontFix ]
> +editing/selection/click-in-padding-with-multiple-line-boxes.html [ WontFix ]
> +fast/writing-mode/flipped-blocks-hit-test-line-edges.html [ WontFix ]

It doesn't seem like a good idea to regress these tests. If anything the said chromium issue should be fixed first.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.