[Webkit-unassigned] [Bug 36359] Double clicking page's last editable inline element doesn't select a word.

[bugzilla-daemon at webkit.org]

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


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #56875|review?                     |review-
               Flag|                            |




--- Comment #20 from Ojan Vafai <ojan at chromium.org>  2010-05-26 17:58:08 PST ---
(From update of attachment 56875)
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.

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