Bug 36359 – Double clicking page's last editable inline element doesn't select a word.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hajime Morrita]

Created attachment 51144 [details]
a test case (also compatible with DRT)

Double clicking certain type of inline element  with contentEditable causes assertion failure.
See attached test-case for concrete example.

reproduce:
- open the test html
- double-click "here" text

expected:
- selection covers "here"
actual:
- assertion failed.

[Hajime Morrita]

Created attachment 51149 [details]
patch v0

[Hajime Morrita]

Created attachment 51390 [details]
v1; cleanup test result

[Enrica Casucci]

Looks good to me. I'm assuming you ran all the layout tests and they pass.

[Shinichiro Hamaji]

Comment on attachment 51390 [details]
v1; cleanup test result

> @@ -175,7 +175,7 @@ static VisiblePosition nextBoundary(const VisiblePosition& c, BoundarySearchFunc
>      Node *de = d->documentElement();
>      if (!de)
>          return VisiblePosition();
> -    Node *boundary = n->enclosingBlockFlowElement();
> +    Node* boundary = n;
>      if (!boundary)
>          return VisiblePosition();
>      bool isContentEditable = boundary->isContentEditable();

I'm not a good reviewer for this bug, but putting r- because I think we need some more work to fix the bug completely.

- I think we should remove "if (!boundary)" because it was already checked.
- With this patch, "here" is selected after the double click. Without this patch, it seems "here" isn't selected so I think you also fixed this issue and we need a test case for this.
- With the following HTML

<div><span id="target" contentEditable="true">here</span> double click</div>

"here" isn't selected when we double-click the target node. Is this related bug?

- (this is just a question) don't we also need to modify previousBoundary?

[Hajime Morrita]

Created attachment 51710 [details]
v2; remove redundant null-check

[Hajime Morrita]

Hi hamaji, thank you for reviewing!

> - I think we should remove "if (!boundary)" because it was already checked.
Fixed.

> - With this patch, "here" is selected after the double click. Without this
> patch, it seems "here" isn't selected so I think you also fixed this issue and
> we need a test case for this.
Selecting a word by double-click is an  expected behaviour.
In other word, current behaviour is wrong even without assertion failure.

> - With the following HTML
> 
> <div><span id="target" contentEditable="true">here</span> double click</div>
> 
> "here" isn't selected when we double-click the target node. Is this related
> bug?
Yes, as mentioned above. I'll change the summary line of this bug...

> 
> - (this is just a question) don't we also need to modify previousBoundary?
Yes, this cause a similar problem, which is filed on Bug 36360.
I'll fix that after this one is fixed.

[Adam Barth]

Another two-line editing patch that's been up for review for almost two months.  @enrica or @ojan, any interest in reviewing?

[Kent Tamura]

Comment on attachment 51710 [details]
v2; remove redundant null-check

This looks ok.
I'll set r+ tomorrow if no one make comments.

[Ojan Vafai]

Comment on attachment 51710 [details]
v2; remove redundant null-check

Sorry the review took so long.

Doesn't previousBoundary have the same bug? Would be good to understand why it doesn't or to fix it and add a testcase for that as well. r- for the previousBoundary issue.

LayoutTests/editing/selection/doubleclick-inline-last-contenteditable.html:45
 +  function flushLog()
This doesn't seem necessary since you only call log once in this test. You can move the logging logic into the log function.

LayoutTests/editing/selection/doubleclick-inline-last-contenteditable.html:29
 +      eventSender.leapForward(1);
These leapForward calls are not necessary to test double-click selecting a word. Just the mouseMoveTo, mouseDown and mouseUp calls are needed.

LayoutTests/editing/selection/doubleclick-inline-last-contenteditable.html:21
 +      pos.y += target.clientHeight/2;
nit: webkit style: there should be spaces around the /

[Hajime Morrita]

Created attachment 56673 [details]
took the feedback

[Hajime Morrita]

Hi Ojan, thank you for reviewing!

> Doesn't previousBoundary have the same bug? Would be good to understand why it doesn't or to fix it and add a testcase for that as well. r- for the previousBoundary issue.
Yes. Although I've filed the case to Bug 36360, It would be better to tackle these to same patch
because they've caused from almost same problem, as you mentioned.
So I updated the patch to tackle both.
I'll close Bug 36360 after this patch is landed.

> 
> LayoutTests/editing/selection/doubleclick-inline-last-contenteditable.html:45
>  +  function flushLog()
> This doesn't seem necessary since you only call log once in this test. You can move the logging logic into the log function.
Fixed.

> 
> LayoutTests/editing/selection/doubleclick-inline-last-contenteditable.html:29
>  +      eventSender.leapForward(1);
> These leapForward calls are not necessary to test double-click selecting a word. Just the mouseMoveTo, mouseDown and mouseUp calls are needed.
Fixed.

> 
> LayoutTests/editing/selection/doubleclick-inline-last-contenteditable.html:21
>  +      pos.y += target.clientHeight/2;
> nit: webkit style: there should be spaces around the /
Fixed.

[Ojan Vafai]

Comment on attachment 56673 [details]
took the feedback

LayoutTests/editing/selection/doubleclick-inline-first-contenteditable.html:14
 +      while (n) {
You only need this code if you have positioned elements. For this test, you can just directly use target.offsetLeft, target.clientLeft, etc. No while loop needed.

LayoutTests/editing/selection/doubleclick-inline-first-contenteditable.html:30
 +      eventSender.leapForward(50);
You shouldn't need this leapFoward for a doubleclick. Did you find it didn't work without it?

Can you combine the two tests into one using HTML like the following: <div><span id="target" contentEditable="true">first</span> double click <span id="target" contentEditable="true">last</span></div>

I tested that locally and it reproduced the bug for both cases.

WebCore/editing/visible_units.cpp:164
 +              // editing boundary. So we lookup editable node from the candidates.
I think this works correctly, but is a bit messy to fix it up after the fact. The best fix I can think of would be to modify TextIterator to have a TextIteratorBehavior that tells it to avoid crossing editing boundaries. I'm sure this won't be the only case where we'll want TextIterators to respect editing boundaries. That's a complicated change though. I'm OK with just adding FIXME here for now, but I'd like feedback from other reviewers. r- for now for the test changes and adding this fixme.

Enrica, Mitz, does this seem OK to you? Do you agree that long-term we should make TextIterator editing-boundary-aware?

[Ojan Vafai]

+darin in case he has thoughts on editing boundaries + TextIterator. I would guess that we have a lot of bugs with contentEditable inline elements where TextIterator crosses editing boundaries and shouldn't.

[Hajime Morrita]

Created attachment 56842 [details]
patch v4

[Hajime Morrita]

Comment on attachment 56842 [details]
patch v4

cancel v4 which misses some points

[Hajime Morrita]

Created attachment 56873 [details]
patch v5

[WebKit Review Bot]

Attachment 56873 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/editing/visible_units.cpp:81:  Declaration has space between type name and * in Document *d  [whitespace/declaration] [3]
WebCore/editing/visible_units.cpp:166:  Declaration has space between type name and * in Document *d  [whitespace/declaration] [3]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hajime Morrita]

Created attachment 56875 [details]
patch v6 - fix lint violation

[Hajime Morrita]

Hi Ojan, thank you for feedback!

> LayoutTests/editing/selection/doubleclick-inline-first-contenteditable.html:14
>  +      while (n) {
> You only need this code if you have positioned elements. For this test, you can just directly use target.offsetLeft, target.clientLeft, etc. No while loop needed.
Fixed.

> LayoutTests/editing/selection/doubleclick-inline-first-contenteditable.html:30
>  +      eventSender.leapForward(50);
> You shouldn't need this leapFoward for a doubleclick. Did you find it didn't work without it?
Fixed.
> 
> Can you combine the two tests into one using HTML like the following: <div><span id="target" contentEditable="true">first</span> double click <span id="target" contentEditable="true">last</span></div>
>
Fixed to combine 2 into 1.
 
> WebCore/editing/visible_units.cpp:164
>  +              // editing boundary. So we lookup editable node from the candidates.
> I think this works correctly, but is a bit messy to fix it up after the fact. The best fix I can think of would be to modify TextIterator to have a TextIteratorBehavior that tells it to avoid crossing editing boundaries. I'm sure this won't be the only case where we'll want TextIterators to respect editing boundaries. That's a complicated change though. I'm OK with just adding FIXME here for now, but I'd like feedback from other reviewers. r- for now for the test changes and adding this fixme.
It seems good idea to make TextIterator aware editing-boundary.
So I did it for SimplifiedBackwardsTextIterator which we are using in this case.

[Ojan Vafai]

Comment on attachment 56875 [details]
patch v6 - fix lint violation

WebCore/ChangeLog:16
 +          TextIteratorEndsAtEditingBoundary.
These last two sentences are a bit confusing. How about something like this (assuming this is correct)?

Also, added TextIteratorEndsAtEditingBoundary to BackwardsCharacterIterator, otherwise, the VisiblePosition returned by BackwardsCharacterIterator might cross an editing boundary.

WebCore/dom/Node.cpp:1522
 +      Node* documentElement = document()->documentElement();
I know the if-statement below works if documentElement is null, but I would prefer to be more explicit, both for readability and in case the code below changed. Can you add a null-check here like there was in the old code?

WebCore/editing/TextIterator.cpp:954
 +      ASSERT(m_behavior == TextIteratorDefaultBehavior || m_behavior == TextIteratorEndsAtEditingBoundary);
I like this assert. Can you add one like this to the start of the TextIterator constructor as well and put a FIXME to add support for TextIteratorEndsAtEditingBoundary to TextIterator?

WebCore/editing/visible_units.cpp:165
 +      Node* boundary = pos.node()->editingBoundary();
This can return null. We should return VisiblePositon() in that case to match the old code.

WebCore/editing/visible_units.cpp:80
 +      Node* boundary = pos.node()->editingBoundary();
This can return null. We should return VisiblePositon() in that case to match the old code.

WebCore/dom/Node.cpp:1517
 +  Node* Node::editingBoundary() const
I like that you moved this into a shared function, but I don't think this belongs in Node. Really, there shouldn't be editing code in Node. The code that's there now needs to move elsewhere some day. For now, this could just live in visible_units.cpp. There are a couple editing methods there and I'm not sure this will be needed elsewhere. If it turns out to be needed elsewhere, we might need to find a better home for it. The editing code in general needs a better home for a lot of code. 

WebCore/editing/TextIterator.cpp:1044
 +                  Node* parentNode = guardEditingBoundary(parentCrossingShadowBoundaries(m_node));
I think this code is correct, but it took me a good deal of looking at the code to figure out why. The key point is that we need to guard for crossing editing boundaries every time we set m_node. How about instead of this method we add something like the following:

void SimplifiedBackwardsTextIterator::setCurrentNode(Node* node)
{
    if (!m_node || !node)
        m_node = 0;

    if (m_behavior == TextIteratorEndsAtEditingBoundary
        && m_node->isContentEditable() != node->isContentEditable())
        m_node = 0;

    m_node = node;
}

Then this code could be:
Node* parentNode = guardEditingBoundary(parentCrossingShadowBoundaries(m_node));
if (!parentNode)
    break;
setCurrentNode(parentNode)
if (!m_node)
// NOTE: I'm not sure if we should break here, or if we should break right before the last line in the while loop.
    break;

And then below, it would just be setCurrentNode(next).

I'm not too familiar with this code, so tell me if this makes no sense. I just want something that's a bit more clear that we need to guard whenever we set m_node.

If you don't want to do all this, the safer, smaller change would be to avoid trying to mix these two checks into one. Instead, add a crossesEditingBoundary method that returns a bool. Then break if parentNode crosses an editing boundary. I realize it's equivalent, but it puts the crossesEditingBoundary call right before setting m_node that way.

WebCore/editing/TextIterator.cpp:1144
 +      if (!m_node || !node)
I don't feel like this should early return for !m_node (and node for that matter). It's just there so the next if-statement doesn't crash, right? Instead, the if-statement below should check m_node and node.

That way, you can make it so that all the places we set m_node actually call setCurrentNode instead.

[Ojan Vafai]

darin, hamaji, mitz, I'm not too familiar with the TextIterator code. You all have modified/reviewed it most recently. Mind taking a look to make sure the TextIterator changes are sane?

[Hajime Morrita]

Created attachment 57210 [details]
patch v7

[Hajime Morrita]

Hi Ojan, thank you for reviewing!

>  +          TextIteratorEndsAtEditingBoundary.
> These last two sentences are a bit confusing. How about something like this (assuming this is correct)?
> 
> Also, added TextIteratorEndsAtEditingBoundary to BackwardsCharacterIterator, otherwise, the VisiblePosition returned by BackwardsCharacterIterator might cross an editing boundary.
Ya, this is what I meant. I replaced with this one. Thank you for your suggestion!

> 
> WebCore/dom/Node.cpp:1522
>  +      Node* documentElement = document()->documentElement();
> I know the if-statement below works if documentElement is null, but I would prefer to be more explicit, both for readability and in case the code below changed. Can you add a null-check here like there was in the old code?
Fixed. The function itself was moved to Position::ascentEditingBoundary()

> 
> WebCore/editing/TextIterator.cpp:954
>  +      ASSERT(m_behavior == TextIteratorDefaultBehavior || m_behavior == TextIteratorEndsAtEditingBoundary);
> I like this assert. Can you add one like this to the start of the TextIterator constructor as well and put a FIXME to add support for TextIteratorEndsAtEditingBoundary to TextIterator?
Fixed.

> 
> WebCore/editing/visible_units.cpp:165
>  +      Node* boundary = pos.node()->editingBoundary();
> This can return null. We should return VisiblePositon() in that case to match the old code.
Fixed.

> 
> WebCore/editing/visible_units.cpp:80
>  +      Node* boundary = pos.node()->editingBoundary();
> This can return null. We should return VisiblePositon() in that case to match the old code.
Fixed.

> 
> WebCore/dom/Node.cpp:1517
>  +  Node* Node::editingBoundary() const
> I like that you moved this into a shared function, but I don't think this belongs in Node. Really, there shouldn't be editing code in Node. The code that's there now needs to move elsewhere some day. For now, this could just live in visible_units.cpp. There are a couple editing methods there and I'm not sure this will be needed elsewhere. If it turns out to be needed elsewhere, we might need to find a better home for it. The editing code in general needs a better home for a lot of code. 
Agreed that Node should not have editing related methods.
I moved this to Position class where there are many editing related method around.

> 
> WebCore/editing/TextIterator.cpp:1044
>  +                  Node* parentNode = guardEditingBoundary(parentCrossingShadowBoundaries(m_node));
> I think this code is correct, but it took me a good deal of looking at the code to figure out why. The key point is that we need to guard for crossing editing boundaries every time we set m_node. How about instead of this method we add something like the following:
Sounds nice. I introduced setCurrentNode().

> 
> WebCore/editing/TextIterator.cpp:1144
>  +      if (!m_node || !node)
> I don't feel like this should early return for !m_node (and node for that matter). It's just there so the next if-statement doesn't crash, right? Instead, the if-statement below should check m_node and node.
Fixed to make null checks separate.

[Ojan Vafai]

Comment on attachment 57210 [details]
patch v7

I'd like darin or mitz to comment since I'm not as familiar with this code.

Just a few nits left from me.

Does the SimplifiedBackwardsTextIterator on TextIterator.cpp line 195 need TextIteratorEndsAtEditingBoundary as well?

WebCore/editing/TextIterator.cpp:1152
 +       if (!node)
Indentation is off.

WebCore/editing/TextIterator.cpp:1154
 +      if (!m_node)
I didn't quite mean that you'd split up the null-checks here. What I meant was that if the m_node null-check were part of the if-statement on line 1156 instead, then you could use setCurrentNode for the initial setting of m_node in the constructor. Then, the only places where m_node is set would be in setCurrentNode and clearCurrentNode. So, specifically, something like this:

bool SimplifiedBackwardsTextIterator::setCurrentNode(Node* node)
{
    if (!node)
        return false;
    if (m_node && m_behavior == TextIteratorEndsAtEditingBoundary && willCrossEditingBoundary(node))
        return false;
    m_node = node;
    return true;
}

WebCore/editing/TextIterator.h:187
 +      bool willCrossEditingBoundary(Node* node) const;
Nit: I prefer the name crossesEditingBoundary

WebCore/dom/Position.h:145
 +      Node* ascentEditingBoundary() const;
This should probably be placed next to atEditingBoundary in the header file as well.

WebCore/dom/Position.cpp:335
 +      if (!m_anchorNode->document())
Just to clarify, I didn't mean that each null-check had to go on it's own line. It would be fine to combine 333 and 335 into one if-statement.

WebCore/dom/Position.cpp:331
 +  Node* Position::ascentEditingBoundary() const
How about parentEditingBoundary? I understand what you mean by "ascent", but I don't think I would get it from calling code without looking at this function to figure out what it does.

[Hajime Morrita]

Created attachment 57292 [details]
patch v8

[Hajime Morrita]

Hi Ojan, thank you for reviewing.
Although It might be better to take a look from others,
I updated the patch to follow last feedback anyway.

> Does the SimplifiedBackwardsTextIterator on TextIterator.cpp line 195 need TextIteratorEndsAtEditingBoundary as well?
It doesn't need such check because m_pastStartNode is used to stop m_node going next,
and m_node is already guarded by editing boundary in setCurrentNode().
The point is that previousInPostOrderCrossingShadowBoundaries() computes 
another kind of boundary what we should care other than editing-boundary.

> 
> WebCore/editing/TextIterator.cpp:1152
>  +       if (!node)
> Indentation is off.
Fixed

> 
> WebCore/editing/TextIterator.cpp:1154
>  +      if (!m_node)
> I didn't quite mean that you'd split up the null-checks here. 
Fixed to move it out of the method, and use it in the ctor.

> WebCore/editing/TextIterator.h:187
>  +      bool willCrossEditingBoundary(Node* node) const;
> Nit: I prefer the name crossesEditingBoundary
Fixed to rename crossesEditingBoundary().

> 
> WebCore/dom/Position.h:145
>  +      Node* ascentEditingBoundary() const;
> This should probably be placed next to atEditingBoundary in the header file as well.
Fixed.

> 
> WebCore/dom/Position.cpp:335
>  +      if (!m_anchorNode->document())
> Just to clarify, I didn't mean that each null-check had to go on it's own line. It would be fine to combine 333 and 335 into one if-statement.
Fixed to combine them.

> 
> WebCore/dom/Position.cpp:331
>  +  Node* Position::ascentEditingBoundary() const
> How about parentEditingBoundary? I understand what you mean by "ascent", but I don't think I would get it from calling code without looking at this function to figure out what it does.
Agreed, and fixed to rename parentEditingBoundary().

[Ojan Vafai]

darin, mitz, mind taking a look at this? This patch looks good to me, but I'm hesitant to r+ since I haven't hacked on TextIterator too much.

[Nikolas Zimmermann]

Comment on attachment 57292 [details]
patch v8

LayoutTests/editing/selection/doubleclick-inline-first-last-contenteditable-expected.txt:1
 +  here to double click, and there
Very confusing output.
Can you use the js-test-pre.js / js-test-post.js framework, and create a better test case?

That would also avoid having to write your custom log() function.
WebCore/editing/TextIterator.cpp:275
 +      // FIXME: should support TextIteratorEndsAtEditingBoundary
You should file a bug for that, and refer to it here.

WebCore/editing/TextIterator.cpp:950
 +      : m_behavior(TextIteratorDefaultBehavior), m_node(0), m_positionNode(0)
Each initialization belongs in its own line.

WebCore/editing/TextIterator.cpp:955
 +      : m_behavior(behavior), m_node(0), m_positionNode(0)
Ditto.

WebCore/editing/TextIterator.h:187
 +      bool crossesEditingBoundary(Node* node) const;
You can omit the "node" name here.

WebCore/editing/TextIterator.h:188
 +      bool setCurrentNode(Node* node);
Ditto.

[Hajime Morrita]

Created attachment 63698 [details]
Patch

[Hajime Morrita]

Thank you for reviewing,  zimmermann!
I updated the patch.

> LayoutTests/editing/selection/doubleclick-inline-first-last-contenteditable-expected.txt:1
>  +  here to double click, and there
> Very confusing output.
> Can you use the js-test-pre.js / js-test-post.js framework, and create a better test case?

I see. I rewrote it with script test facility.

> 
> That would also avoid having to write your custom log() function.
> WebCore/editing/TextIterator.cpp:275
>  +      // FIXME: should support TextIteratorEndsAtEditingBoundary
> You should file a bug for that, and refer to it here.

Filed at Bug 43609 and pointed it.

> 
> WebCore/editing/TextIterator.cpp:950
>  +      : m_behavior(TextIteratorDefaultBehavior), m_node(0), m_positionNode(0)
> Each initialization belongs in its own line.
Done

> 
> WebCore/editing/TextIterator.cpp:955
>  +      : m_behavior(behavior), m_node(0), m_positionNode(0)
> Ditto.
Done.

> 
> WebCore/editing/TextIterator.h:187
>  +      bool crossesEditingBoundary(Node* node) const;
> You can omit the "node" name here.
Done.

> 
> WebCore/editing/TextIterator.h:188
>  +      bool setCurrentNode(Node* node);
> Ditto.
Done.

[Ryosuke Niwa]

(In reply to comment #29)
> Created an attachment (id=63698) [details]
> Patch

Are you sure all of your change is well tested?  I'm afraid about changes in visible_units and TextIterator because I've been finding many bugs in those areas and I suspect that there aren't enough tests right now.

Your test seems to only test with mouse events, but your change will affect WebKit's behavior on keyboard events and window.getSelection().modify calls.  I'll really appreciate if you could add more tests.

[Ojan Vafai]

Comment on attachment 63698 [details]
Patch

> +    // http://webkit.org/b/43609: should support TextIteratorEndsAtEditingBoundary 

Please add "FIXME:" at the start of this comment.

[Hajime Morrita]

Committed r64974: <http://trac.webkit.org/changeset/64974>

(In reply to comment #31)
> Your test seems to only test with mouse events, but your change will affect WebKit's behavior on keyboard events and window.getSelection().modify calls.  I'll really appreciate if you could add more tests.
Hi Ryosuke, thank you for your advice!
I've added some more test that covers modify() and boundary detection across <divs>s, not only <span>s.