Triple click does not select whole line for mixed contenteditable regions

• Component: HTML Editing
• Status: ASSIGNED ---

[Benjamin (Ben) Kalman]

Triple click does not select whole line for mixed contenteditable regions

[Benjamin (Ben) Kalman]

Created attachment 71385 [details]
Patch

[Ryosuke Niwa]

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.

[Benjamin (Ben) Kalman]

Created attachment 71526 [details]
Patch

[Benjamin (Ben) Kalman]

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).

[Ojan Vafai]

(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.

[Ryosuke Niwa]

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.

[Benjamin (Ben) Kalman]

Created attachment 71730 [details]
Patch

[Benjamin (Ben) Kalman]

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?

[Ryosuke Niwa]

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?

[Benjamin (Ben) Kalman]

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.

[Benjamin (Ben) Kalman]

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

[Ryosuke Niwa]

(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.

[Benjamin (Ben) Kalman]

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).

[Benjamin (Ben) Kalman]

Created attachment 71839 [details]
Patch

[Benjamin (Ben) Kalman]

Turns out the enum change means that I don't need to update the other tests after all...

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Benjamin (Ben) Kalman]

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?

[Ryosuke Niwa]

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.

[Benjamin (Ben) Kalman]

Created attachment 71850 [details]
Patch

[Benjamin (Ben) Kalman]

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.

[Benjamin (Ben) Kalman]

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 ?

[Tony Chang]

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.

[Ryosuke Niwa]

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.

[Ojan Vafai]

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).

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Kalman,

Can I assign this bug to you?

[Benjamin (Ben) Kalman]

yep

[Ryosuke Niwa]

Created attachment 72238 [details]
changes to visible_units & DeleteSelectionCommand

[Benjamin (Ben) Kalman]

Created attachment 72357 [details]
Patch

[Ryosuke Niwa]

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.

[Darin Adler]

(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.

[Ryosuke Niwa]

Could someone review kalman's patch?

[Benjamin (Ben) Kalman]

I have a new version of the patch that I'll upload soon.  Will ping the bug when it's done.

[Benjamin (Ben) Kalman]

Created attachment 76159 [details]
Patch

[Benjamin (Ben) Kalman]

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.