Bug 48037 - Triple click does not select whole line for mixed contenteditable regions
Summary: Triple click does not select whole line for mixed contenteditable regions
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Benjamin (Ben) Kalman
URL:
Keywords:
Depends on: 48112 48658
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-20 20:35 PDT by Benjamin (Ben) Kalman
Modified: 2017-07-18 08:29 PDT (History)
8 users (show)

See Also:


Attachments
Patch (27.43 KB, patch)
2010-10-20 20:58 PDT, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (27.54 KB, patch)
2010-10-21 22:27 PDT, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (32.25 KB, patch)
2010-10-25 04:30 PDT, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (13.78 KB, patch)
2010-10-25 21:28 PDT, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (13.84 KB, patch)
2010-10-26 01:46 PDT, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
changes to visible_units & DeleteSelectionCommand (7.41 KB, patch)
2010-10-28 14:56 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (19.08 KB, patch)
2010-10-29 11:42 PDT, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (29.15 KB, patch)
2010-12-09 20:32 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin (Ben) Kalman 2010-10-20 20:35:29 PDT
Triple click does not select whole line for mixed contenteditable regions
Comment 1 Benjamin (Ben) Kalman 2010-10-20 20:58:44 PDT
Created attachment 71385 [details]
Patch
Comment 2 Ryosuke Niwa 2010-10-21 12:22:42 PDT
Comment on attachment 71385 [details]
Patch

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.
Comment 3 Benjamin (Ben) Kalman 2010-10-21 22:27:43 PDT
Created attachment 71526 [details]
Patch
Comment 4 Benjamin (Ben) Kalman 2010-10-21 22:44:41 PDT
Comment on attachment 71385 [details]
Patch

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.

There is a bug in webkit where copy-pasting some text into the entire of a contenteditable span inserts an extra <br> character at the end.  Easy repro is
"foo <span contenteditable>bar</span> baz"
Double click on foo, ctrl+c, double click on bar, ctrl+v.
The bug was being triggered by the copy-backslash-with-euc test, and for some reason this patch fixes it.

>> LayoutTests/ChangeLog:25
>> +        * 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.

Sounds good.  Are you suggesting I do this in a different patch, submit it, then come back to this one?

>> WebCore/editing/visible_units.cpp:-801
>> -            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.

I'm not sure what you mean.  In what way should editable boundaries be respected?
Elaboration: it's this check that incorrectly stops the selection when a non-contenteditable region is encountered inside a contenteditable one.  I say "incorrectly" because both IE and FF select across them.  From what I can tell there is sanity checking elsewhere (VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries).
Comment 5 Ojan Vafai 2010-10-22 08:05:15 PDT
(In reply to comment #4)
> Sounds good.  Are you suggesting I do this in a different patch, submit it, then come back to this one?

Correct. The standard way to do code cleanup in webkit (including converting tests to dump-as-markup) is to do the cleanup patch first then do your actual change. The overhead ends up being worth it because it makes each patch much more clear and easy to verify correctness.
Comment 6 Ryosuke Niwa 2010-10-22 13:11:24 PDT
Comment on attachment 71385 [details]
Patch

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

>>> WebCore/editing/visible_units.cpp:-801
>>> -            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.
> 
> I'm not sure what you mean.  In what way should editable boundaries be respected?
> Elaboration: it's this check that incorrectly stops the selection when a non-contenteditable region is encountered inside a contenteditable one.  I say "incorrectly" because both IE and FF select across them.  From what I can tell there is sanity checking elsewhere (VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries).

startOfParagraph is used everywhere in editing and removing this check would allow editing commands to operate across editing boundaries.  e.g. Suppose we have <div>hello <span contenteditable>world</span></div>, and "world" is selected.  When we indent, we shouldn't be indenting "hello".

You're probably trying to fix the behavior of startOfParagraph when called in VisibleSelection::setStartAndEndFromBaseAndExtentRespectingGranularity.  That's very specific use of startOfParagraph and even for that we need to verify that it won't break other things.  Now, setStartAndEndFromBaseAndExtentRespectingGranularity is called by validate with granularity != CharacterGranularity in expandUsingGranularity.  There are 4 calls to expandUsingGranularity, and all of them seem to be user-initiated so they're fine.  The call from expandSelectionToGranularity seems fine as well because they're executing commands to extend selection.

So we can probably modify the behavior of setStartAndEndFromBaseAndExtentRespectingGranularity(ParagraphGranularity) without breaking anything. And if we're changing the behavior of startOfParagraph and endOfParagraph for these two functions, then we probably want to add an argument to these functions to ignore editing boundaries.  See Position::CanCrossEditingBoundary.

> WebCore/editing/visible_units.cpp:816
> +        // FIXME: We avoid returning a position where the renderer can't accept the caret.
> +        // We also do this in endOfParagraph, and might want to do it in other cases too.
> +        if (r->isText() && r->caretMaxRenderedOffset() > 0) {

Are you sure this won't skip unicode-bidi control characters, zero-width space, etc... ?  I need Xiaomei's or Dan's confirmation on this.
Comment 7 Benjamin (Ben) Kalman 2010-10-25 04:30:23 PDT
Created attachment 71730 [details]
Patch
Comment 8 Benjamin (Ben) Kalman 2010-10-25 04:43:44 PDT
Comment on attachment 71385 [details]
Patch

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

>>>> WebCore/editing/visible_units.cpp:-801
>>>> -            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.
>> 
>> I'm not sure what you mean.  In what way should editable boundaries be respected?
>> Elaboration: it's this check that incorrectly stops the selection when a non-contenteditable region is encountered inside a contenteditable one.  I say "incorrectly" because both IE and FF select across them.  From what I can tell there is sanity checking elsewhere (VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries).
> 
> startOfParagraph is used everywhere in editing and removing this check would allow editing commands to operate across editing boundaries.  e.g. Suppose we have <div>hello <span contenteditable>world</span></div>, and "world" is selected.  When we indent, we shouldn't be indenting "hello".
> 
> You're probably trying to fix the behavior of startOfParagraph when called in VisibleSelection::setStartAndEndFromBaseAndExtentRespectingGranularity.  That's very specific use of startOfParagraph and even for that we need to verify that it won't break other things.  Now, setStartAndEndFromBaseAndExtentRespectingGranularity is called by validate with granularity != CharacterGranularity in expandUsingGranularity.  There are 4 calls to expandUsingGranularity, and all of them seem to be user-initiated so they're fine.  The call from expandSelectionToGranularity seems fine as well because they're executing commands to extend selection.
> 
> So we can probably modify the behavior of setStartAndEndFromBaseAndExtentRespectingGranularity(ParagraphGranularity) without breaking anything. And if we're changing the behavior of startOfParagraph and endOfParagraph for these two functions, then we probably want to add an argument to these functions to ignore editing boundaries.  See Position::CanCrossEditingBoundary.

Ah, thank you.

I've added the argument to start/endOfParagraph, and a token test (for the indentation) which did indeed crash after removing the check.  It seems a little... messy... but perhaps that's inevitable.

>> WebCore/editing/visible_units.cpp:816
>> +        if (r->isText() && r->caretMaxRenderedOffset() > 0) {
> 
> Are you sure this won't skip unicode-bidi control characters, zero-width space, etc... ?  I need Xiaomei's or Dan's confirmation on this.

No idea -- would this case be different to the same check in endOfParagraph?
Comment 9 Ryosuke Niwa 2010-10-25 19:41:06 PDT
Comment on attachment 71730 [details]
Patch

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

On second thought, I'm not even sure if this bug is valid.  We normally disallow selections that cross editing boundaries.  Why should be allow selection be extended across editing boundaries?  This patch seems to introduce a serious inconsistency into the editing code.

> LayoutTests/platform/chromium-linux/editing/deleting/5390681-expected.txt:16
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x584
> +      RenderBlock {P} at (0,0) size 784x40
> +        RenderText {#text} at (0,0) size 768x39
> +          text run at (0,0) width 565: "This tests for a bug where expansion for smart delete would not consider editable boundaries. "
> +          text run at (565,0) width 177: "Only 'foo' should be deleted. "
> +          text run at (742,0) width 26: "You"
> +          text run at (0,20) width 101: "should see ' bar'."
> +      RenderBlock {DIV} at (0,56) size 784x20
> +        RenderInline {SPAN} at (0,0) size 20x19
> +          RenderText {#text} at (0,0) size 20x19
> +            text run at (0,0) width 20: "bar"
> +caret: position 0 of child 2 {DIV} of body

Wait, why are you adding this?

> LayoutTests/platform/chromium-linux/editing/deleting/delete-mixed-editable-content-001-expected.txt:27
> +EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of SPAN > SPAN > DIV > BODY > HTML > #document to 1 of SPAN > SPAN > DIV > BODY > HTML > #document
> +EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
> +EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of SPAN > SPAN > DIV > BODY > HTML > #document to 0 of SPAN > SPAN > DIV > BODY > HTML > #document toDOMRange:range from 0 of #text > SPAN > SPAN > DIV > BODY > HTML > #document to 5 of #text > SPAN > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> +EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
> +EDITING DELEGATE: shouldDeleteDOMRange:range from 0 of #text > SPAN > SPAN > DIV > BODY > HTML > #document to 5 of #text > SPAN > SPAN > DIV > BODY > HTML > #document
> +EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of SPAN > SPAN > DIV > BODY > HTML > #document to 0 of SPAN > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> +EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
> +EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x584
> +      RenderBlock {DIV} at (0,0) size 204x24 [border: (2px solid #FF0000)]
> +        RenderInline {SPAN} at (0,0) size 40x19
> +          RenderText {#text} at (0,0) size 0x0
> +          RenderInline {SPAN} at (0,0) size 40x19
> +            RenderInline {SPAN} at (0,0) size 40x19
> +              RenderText {#text} at (2,2) size 40x19
> +                text run at (2,2) width 40: "12345"
> +          RenderText {#text} at (0,0) size 0x0
> +          RenderInline {SPAN} at (0,0) size 0x0
> +          RenderText {#text} at (0,0) size 0x0
> +        RenderText {#text} at (0,0) size 0x0
> +caret: position 0 of child 3 {SPAN} of child 1 {SPAN} of child 0 {DIV} of body

Same comment for this one.  Perhaps, you've forgot to remove these?
Comment 10 Benjamin (Ben) Kalman 2010-10-25 19:54:54 PDT
Why not?  Arguably in

<div contenteditable>foo <span contenteditable="false">bar</span> baz</div>

the "bar" is just some uneditable inline entity within an editable block, so I don't see why the shouldn't cross it.  Another way of saying the same thing, I suppose, is "are foo/bar/baz in the same paragraph", the answer to which I assumed was yes.

As some possibly interesting trivia:
 - IE (9 -- haven't tested the others) selects everything, and even allows you to move the "bar" around inside the containing div.
 - Firefox selects everything.
 - Opera selects everything.

i.e. WebKit is the exception.
Comment 11 Benjamin (Ben) Kalman 2010-10-25 19:56:02 PDT
Comment on attachment 71730 [details]
Patch

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

>> LayoutTests/platform/chromium-linux/editing/deleting/5390681-expected.txt:16
>> +caret: position 0 of child 2 {DIV} of body
> 
> Wait, why are you adding this?

Nah just haven't merged yet :-)

>> LayoutTests/platform/chromium-linux/editing/deleting/delete-mixed-editable-content-001-expected.txt:27
>> +caret: position 0 of child 3 {SPAN} of child 1 {SPAN} of child 0 {DIV} of body
> 
> Same comment for this one.  Perhaps, you've forgot to remove these?

ditto
Comment 12 Ryosuke Niwa 2010-10-25 20:16:39 PDT
(In reply to comment #10)
> <div contenteditable>foo <span contenteditable="false">bar</span> baz</div>
> 
> the "bar" is just some uneditable inline entity within an editable block, so I don't see why the shouldn't cross it.  Another way of saying the same thing, I suppose, is "are foo/bar/baz in the same paragraph", the answer to which I assumed was yes.

In this case, new behavior seems to be desirable.  But how about
foo <div contenteditable>bar</div> baz
or
<div contenteditable>foo</div> bar <div contenteditable>baz</div>
With your patch, we extend the selection to foo & baz in these case as well.  I'm not sure if we should allow such weird selections.
Comment 13 Benjamin (Ben) Kalman 2010-10-25 20:32:51 PDT
I don't think so?  end/startOfParagraph still don't select outside their containing block elements so the editibility wouldn't really play a part.  Something that perhaps would be surprising is triple clicking on "bar" where

<div>foo <span contenteditable>bar</span> baz</div>

but I believe that the selection is restricted to just "baz" in VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries

(Note that Firefox does select outside "bar" but its handling of contenteditable spans seems to be rather buggy; IE9 only selects "bar", the same as WebKit both before and after the patch).

More trivia: I also tried this all on Opera, and it behaves very similarly to Firefox (but so far not buggy).
Comment 14 Benjamin (Ben) Kalman 2010-10-25 21:28:28 PDT
Created attachment 71839 [details]
Patch
Comment 15 Benjamin (Ben) Kalman 2010-10-25 21:30:27 PDT
Turns out the enum change means that I don't need to update the other tests after all...
Comment 16 Ryosuke Niwa 2010-10-25 21:37:06 PDT
(In reply to comment #12)
> I don't think so?  end/startOfParagraph still don't select outside their containing block elements so the editibility wouldn't really play a part.  Something that perhaps would be surprising is triple clicking on "bar" where

oops, I meant

foo <span contenteditable>bar</span> baz

and

<span contenteditable>foo</span> bar <span contenteditable>baz</span>

> <div>foo <span contenteditable>bar</span> baz</div>
> 
> but I believe that the selection is restricted to just "baz" in VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries

I see.  That makes sense.
Comment 17 Ryosuke Niwa 2010-10-25 21:50:07 PDT
Comment on attachment 71839 [details]
Patch

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

> LayoutTests/editing/selection/triple-click-across-editability-boundaries.html:37
> +    layoutTestController.dumpAsText();
> +    var testElementIds = ["test1Element", "test2Element", "test3Element"];
> +    for (var i in testElementIds) {
> +        var element = document.getElementById(testElementIds[i]);
> +        log("Results for " + element.id + ":\n");
> +        tripleClickAndLog(element, "left");
> +        tripleClickAndLog(element, "middle");
> +        tripleClickAndLog(element, "right");
> +    }

You probably want to rewrite this as a script-test.  See http://trac.webkit.org/changeset/68670 for an example.

> LayoutTests/editing/selection/triple-click-across-editability-boundaries.html:58
> +    eventSender.mouseDown();
> +    eventSender.mouseUp();
> +    eventSender.mouseDown();
> +    eventSender.mouseUp();
> +    eventSender.mouseDown();
> +    eventSender.mouseUp();

This might be too fast.  You may need to add some delays between each downs & ups.

> WebCore/editing/visible_units.cpp:817
> +        // FIXME: We avoid returning a position where the renderer can't accept the caret.
> +        // We also do this in endOfParagraph, and might want to do it in other cases too.

Why FIXME?  Isn't new code correct?  I don't think we should add FIXME to indicate that we should fix other places.  And I note that there are other places in editing where we skip empty text nodes.  If you've found other code that may require skipping empty text nodes, then you should add FIXME to those places and not here.

> WebCore/editing/visible_units.cpp:-877
>          // FIXME: We avoid returning a position where the renderer can't accept the caret.
> -        // We should probably do this in other cases such as startOfParagraph.

You should get rid of this FIXME entirely since we now do the same in startOfParagraph.

> WebCore/editing/visible_units.h:37
> +enum EditingBoundaryCrossingRule { CanCrossEditingBoundary, CannotCrossEditingBoundary };

Mn... you probably don't want to redefine enum here.
Comment 18 Ryosuke Niwa 2010-10-25 21:55:49 PDT
Comment on attachment 71839 [details]
Patch

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

> LayoutTests/editing/execCommand/indent-mixed-editability.html:1
> +<script src="../../resources/dump-as-markup.js"></script>

Please add <!DOCTYPE html> to force the standard mode.

> LayoutTests/editing/selection/triple-click-across-editability-boundaries.html:1
> +<html>

Please add <!DOCTYPE html> to force the standard mode.  But this isn't relevant once you convert this test to a script test.
Comment 19 Benjamin (Ben) Kalman 2010-10-25 22:38:21 PDT
Comment on attachment 71839 [details]
Patch

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

>> LayoutTests/editing/selection/triple-click-across-editability-boundaries.html:58
>> +    eventSender.mouseUp();
> 
> This might be too fast.  You may need to add some delays between each downs & ups.

Yeah I copied it from the other triple click test, seems to work?

>> WebCore/editing/visible_units.cpp:817
>> +        // We also do this in endOfParagraph, and might want to do it in other cases too.
> 
> Why FIXME?  Isn't new code correct?  I don't think we should add FIXME to indicate that we should fix other places.  And I note that there are other places in editing where we skip empty text nodes.  If you've found other code that may require skipping empty text nodes, then you should add FIXME to those places and not here.

Just copied the existing FIXME -- I don't know why that FIXME was added (http://trac.webkit.org/changeset/8786).
I took the "such as startOfParagraph" to mean that there might be other cases too besides startOfParagraph, but I will delete it if you want.  I'll definitely delete the one here though.

>> WebCore/editing/visible_units.h:37
>> +enum EditingBoundaryCrossingRule { CanCrossEditingBoundary, CannotCrossEditingBoundary };
> 
> Mn... you probably don't want to redefine enum here.

Yeah I was wondering about that.  So Position::EditingBoundaryCrossingRule etc everywhere?  Or define a new similarly-named enum?
Comment 20 Ryosuke Niwa 2010-10-26 00:01:42 PDT
Comment on attachment 71839 [details]
Patch

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

The patch looks much better now.  We probably want some reviewers come in & take a look at your patch once the problems I have addressed here have been fixed.

>>> LayoutTests/editing/selection/triple-click-across-editability-boundaries.html:58
>>> +    eventSender.mouseUp();
>> 
>> This might be too fast.  You may need to add some delays between each downs & ups.
> 
> Yeah I copied it from the other triple click test, seems to work?

Mn... could you verify that the other test hasn't been flaky?

>>> WebCore/editing/visible_units.cpp:817
>>> +        // We also do this in endOfParagraph, and might want to do it in other cases too.
>> 
>> Why FIXME?  Isn't new code correct?  I don't think we should add FIXME to indicate that we should fix other places.  And I note that there are other places in editing where we skip empty text nodes.  If you've found other code that may require skipping empty text nodes, then you should add FIXME to those places and not here.
> 
> Just copied the existing FIXME -- I don't know why that FIXME was added (http://trac.webkit.org/changeset/8786).
> I took the "such as startOfParagraph" to mean that there might be other cases too besides startOfParagraph, but I will delete it if you want.  I'll definitely delete the one here though.

That's very mysterious.  Darin might be able to elaborate on this issue since he reviewed the patch (r8786).

>>> WebCore/editing/visible_units.h:37
>>> +enum EditingBoundaryCrossingRule { CanCrossEditingBoundary, CannotCrossEditingBoundary };
>> 
>> Mn... you probably don't want to redefine enum here.
> 
> Yeah I was wondering about that.  So Position::EditingBoundaryCrossingRule etc everywhere?  Or define a new similarly-named enum?

You probably want to do Positon::EditingBoundaryCrossingRule everywhere or promote EditingBoundaryCrossingRule into WebCore namespace in Position.h.  Regardless, We shouldn't be defining two identical enums in Position and VisiblePosition.
Comment 21 Benjamin (Ben) Kalman 2010-10-26 01:46:12 PDT
Created attachment 71850 [details]
Patch
Comment 22 Benjamin (Ben) Kalman 2010-10-26 01:48:04 PDT
Comment on attachment 71839 [details]
Patch

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

>> LayoutTests/editing/selection/triple-click-across-editability-boundaries.html:37
>> +    }
> 
> You probably want to rewrite this as a script-test.  See http://trac.webkit.org/changeset/68670 for an example.

Ok done.  It's longer though... maybe I'm doing something wrong...

>>>> LayoutTests/editing/selection/triple-click-across-editability-boundaries.html:58
>>>> +    eventSender.mouseUp();
>>> 
>>> This might be too fast.  You may need to add some delays between each downs & ups.
>> 
>> Yeah I copied it from the other triple click test, seems to work?
> 
> Mn... could you verify that the other test hasn't been flaky?

You're right; it actually doesn't work without a delay (speaking of which, is there a more standard way to do a delay in tests?).
The other test has been disabled in a few places; I can have a look at this in another patch.
Comment 23 Benjamin (Ben) Kalman 2010-10-26 01:49:23 PDT
Comment on attachment 71850 [details]
Patch

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

> LayoutTests/editing/selection/script-tests/triple-click-across-editability-boundaries.js:70
> +    //testTripleClick("bar baz rab", "middle", test3);

This is interesting, possibly a bug in VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries ?
Comment 24 Tony Chang 2010-10-26 09:43:25 PDT
Comment on attachment 71850 [details]
Patch

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

don't have time for a full review now, will look again in a few hours

> LayoutTests/editing/selection/script-tests/triple-click-across-editability-boundaries.js:45
> +function pause(millis) {
> +    var startDate = new Date();
> +    while ((new Date()) - startDate < millis) {}

You should use eventSender.leapForward so the test isn't slow.
Comment 25 Ryosuke Niwa 2010-10-26 10:07:50 PDT
Comment on attachment 71850 [details]
Patch

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

> LayoutTests/editing/selection/script-tests/triple-click-across-editability-boundaries.js:51
> +    var test1 = createElementWithInnerHTML("span",
> +        "foo <span contenteditable>bar</span> baz");

You can fit all of this in one line.  In WebKit, we don't limit ourselves to 80 columns.

>> LayoutTests/editing/selection/script-tests/triple-click-across-editability-boundaries.js:70
>> +    //testTripleClick("bar baz rab", "middle", test3);
> 
> This is interesting, possibly a bug in VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries ?

This is a quite serious bug.  We should fix this bug before checking in this patch.
Comment 26 Ojan Vafai 2010-10-26 10:57:18 PDT
Comment on attachment 71850 [details]
Patch

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

I agree that we should be able to select non-editable regions inside an editable region using these APIs. The user can do it using the mouse. The important thing is to be careful to make sure we don't select part of a non-editable region. It needs to be atomic. Can you add to your test the cases where the non-editable span is at the beginning/end of the paragraph?

r- for the test case

FWIW, I agree that script-tests are unfortunate. Feel free to fix https://bugs.webkit.org/show_bug.cgi?id=48344. :)

>>> LayoutTests/editing/selection/script-tests/triple-click-across-editability-boundaries.js:70
>>> +    //testTripleClick("bar baz rab", "middle", test3);
>> 
>> This is interesting, possibly a bug in VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries ?
> 
> This is a quite serious bug.  We should fix this bug before checking in this patch.

It's not clear to me what the actual result here is. The standard behavior here is to put a FIXME, have it spit out the FAIL line and check in the expectations with the FAIL line. That's better that commented out code because it keeps the codepath tested (e.g. if someone ends up changing this in the future).
Comment 27 Ryosuke Niwa 2010-10-26 12:08:18 PDT
(In reply to comment #26)
> >>> LayoutTests/editing/selection/script-tests/triple-click-across-editability-boundaries.js:70
> >>> +    //testTripleClick("bar baz rab", "middle", test3);
> >> 
> >> This is interesting, possibly a bug in VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries ?
> > 
> > This is a quite serious bug.  We should fix this bug before checking in this patch.
> 
> It's not clear to me what the actual result here is. The standard behavior here is to put a FIXME, have it spit out the FAIL line and check in the expectations with the FAIL line. That's better that commented out code because it keeps the codepath tested (e.g. if someone ends up changing this in the future).

Right, that's the general approach. But we shouldn't check-in this particular patch unless the bug in adjustSelectionToAvoidCrossingEditingBoundaries is fixed because we're generating a selection across an editing boundary. The bug is exposed precisely because startOfParagraph crosses editing boundary with this change.  Before this patch, the problem never showed up because startOfParagraph didn't cross any editing boundaries.
Comment 28 Ryosuke Niwa 2010-10-28 00:19:12 PDT
Kalman,

Can I assign this bug to you?
Comment 29 Benjamin (Ben) Kalman 2010-10-28 10:16:25 PDT
yep
Comment 30 Ryosuke Niwa 2010-10-28 14:56:11 PDT
Created attachment 72238 [details]
changes to visible_units & DeleteSelectionCommand
Comment 31 Benjamin (Ben) Kalman 2010-10-29 11:42:37 PDT
Created attachment 72357 [details]
Patch
Comment 32 Ryosuke Niwa 2010-10-29 11:48:37 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=72357&action=review

What happened to the change in DeleteSelectionCommand?

> LayoutTests/editing/selection/script-tests/triple-click-across-editability-boundaries.js:58
> +    // FIXME: currently this is [bar].
> +    testTripleClick("foo bar baz", "middle", test2);
> +    testTripleClick("foo bar baz", "right", test2);

Are you planning to fix this before landing this patch?

> WebCore/WebCore.exp.in:420
> -__ZN7WebCore16isEndOfParagraphERKNS_15VisiblePositionE
> +__ZN7WebCore16isEndOfParagraphERKNS_15VisiblePositionENS_8Position27EditingBoundaryCrossingRuleE

You need to do the same in WebCore.order.  I'm actually surprised that isEndOfParagraph / isStartOfParagraph are exported.  Are they really used in WebKit layer?  If not, we can get rid of these declarations entirely.
Comment 33 Darin Adler 2010-10-29 12:20:13 PDT
(In reply to comment #32)
> You need to do the same in WebCore.order.

The order files do *not* need to be updated. Those are generated by engineers at Apple who run some special tools to do so; there is no practical way to keep them up to date as we add functions and change what calls what. So there is no need to make changes.
Comment 34 Ryosuke Niwa 2010-12-08 16:08:52 PST
Could someone review kalman's patch?
Comment 35 Benjamin (Ben) Kalman 2010-12-08 16:14:30 PST
I have a new version of the patch that I'll upload soon.  Will ping the bug when it's done.
Comment 36 Benjamin (Ben) Kalman 2010-12-09 20:32:48 PST
Created attachment 76159 [details]
Patch
Comment 37 Benjamin (Ben) Kalman 2010-12-09 20:37:01 PST
This patch needed to change quite a lot to pass most of the cases I put in the layout test.  As such, it's a pretty invasive change so I don't really expect to get it submitted as-is any time soon.

(There are also a few tests that will need to be rebaselined, and another test that currently fails an assertion -- long story about that one).

So this is kind of... just for the record, while I go off and get some more experience with webkit.