[Webkit-unassigned] [Bug 48037] Triple click does not select whole line for mixed contenteditable regions

[bugzilla-daemon at webkit.org]

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





--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org>  2010-10-21 12:22:42 PST ---
(From update of attachment 71385)
View in context: https://bugs.webkit.org/attachment.cgi?id=71385&action=review

> LayoutTests/ChangeLog:13
> +        * editing/pasteboard/copy-backslash-with-euc-expected.txt:

Please explain what's happening in this test.

> LayoutTests/ChangeLog:25
> +        * platform/chromium-win/editing/deleting/5390681-expected.txt:
> +        * platform/chromium-win/editing/deleting/delete-mixed-editable-content-001-expected.txt:
> +        * platform/gtk/editing/deleting/5390681-expected.txt:
> +        * platform/mac/editing/deleting/5390681-expected.txt:
> +        * platform/mac/editing/deleting/delete-mixed-editable-content-001-expected.txt:
> +        * platform/qt/editing/deleting/5390681-expected.txt:
> +        * platform/qt/editing/deleting/delete-mixed-editable-content-001-expected.txt:

Please consider converting these two tests to dump-as-markup or dump-as-text tests first because I don't think these two tests need to test the rendering results.  We're more interested in the correctness of the resultant DOM, which can be verified more easily in dump-as-markup / dump-as-text format.  You can also reduce the size of this patch once the conversion is done.  Note you should file a separate bug if you're doing the conversion.

> LayoutTests/editing/selection/triple-click-across-editability-boundaries.html:30
> +    editingTest();

Why do you need to add editingTest()?  editingTest is a special function called by runEditingTest() in the tests that use editing.js.  It's confusing that you're using the same name even though you're not using editing.js at all.  Moreover, I don't think you need to create a function at all.  You can just do all the tests inside of this if statement.

> LayoutTests/editing/selection/triple-click-across-editability-boundaries.html:36
> +    var elemIds = ["elem1", "elem2", "elem3"];
> +    for (var i in elemIds) {
> +        var elem = document.getElementById(elemIds[i]);

Please don't abbreviate element as elem.  And it'll be nice if elem1, elem2, and elem3 had more descriptive names.

> LayoutTests/editing/selection/triple-click-across-editability-boundaries.html:70
> +    var console = document.getElementById("console");
> +    var text = document.createTextNode(message);
> +    console.appendChild(text);

You can do all of this in one line:
document.getElementById("console").appendChild(document.createTextNode(message));
You can do it in two lines where you define console in a separate line.

> WebCore/editing/visible_units.cpp:-801
> -        if (n->isContentEditable() != startNode->isContentEditable())
> -            break;

Could you elaborate on why it is okay to remove this check?  It's not too obvious to me that editable boundaries are respected without this check.

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