Bug 54535 – Stop calling node() and deprecatedEditingOffset() in comparePositions

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

This is a cleanup.

[Ryosuke Niwa]

Created attachment 82597 [details]
work in progress

[Ryosuke Niwa]

Comment on attachment 82597 [details]
work in progress

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

> Source/WebCore/dom/CharacterData.cpp:156
> -    if (document()->frame())
> -        document()->frame()->selection()->textWillBeReplaced(this, offsetOfReplacedData, oldLength, newLength);
>      RefPtr<StringImpl> oldData = m_data;
>      m_data = newData;
> +    if (document()->frame())
> +        document()->frame()->selection()->textWillBeReplaced(this, offsetOfReplacedData, oldLength, newLength);

This change is required because computeOffsetInContainerNode() auto-corrects the offset and causes VisibleSelection::setWithoutValidation to set m_baseIsFirst incorrectly when new offset is larger than the maximum offset in the oldData.

[Ryosuke Niwa]

This patch can't be completed at the moment because it regresses editing/deleting/25322-2.html.  In this test, we have an image followed by a br as in:
<div><img src=..><br></div>

We then place the caret at the end of the div, and press delete key.  Because of canononicalization, we end up getting [br, 0] internally for this selection in TypingCommand::deleteKeyPressed.  The problem is when we extend the selection backwards by SelectionController::modify.  The extent in this case is [img, 1] and comparePositions with my patch says they're identical to [br, 0] and VisibleSelection::setBaseAndExtentToDeepEquivalents sets m_baseIsFirst true.  However, this causes all sort of assumptions to break down in DeleteSelectionCommand.

I'm not sure what the correct fix is.  It seems like DeleteSelectionCommand needs to handle these special cases but it's also odd that SelectionController replies on Position comparison functions to determine which positions comes first when they should really be comparing positions visually.

[Shinya Kawanaka]

I'm now thinking "position after img" and "position before br" is different in some meaning. When inserting something between img and br, those position becomes different apparently.

If we can distinguish them, it will be able to fix editing/deleting/25322-2.html case. It might regress other tests though... But I believe it's worth considering.

[Ryosuke Niwa]

(In reply to comment #4)
> I'm now thinking "position after img" and "position before br" is different in some meaning. When inserting something between img and br, those position becomes different apparently.

I don't think that makes sense. They're identical DOM positions. We should figure out why we have to treat them differently and fix that problem.

> If we can distinguish them, it will be able to fix editing/deleting/25322-2.html case. It might regress other tests though... But I believe it's worth considering.

But then we're introducing yet-another quirks into the editing code. It's like fixing one problem by introducing another problem.

[Shinya Kawanaka]

(In reply to comment #5)
> (In reply to comment #4)
> > I'm now thinking "position after img" and "position before br" is different in some meaning. When inserting something between img and br, those position becomes different apparently.
> 
> I don't think that makes sense. They're identical DOM positions. We should figure out why we have to treat them differently and fix that problem.

As we (shinyak and rniwa) talked on 18 Jun 2012, they are the same DOM positions, but when inserting some elements between <br> and <img>, they become another position.
Maybe we should be careful about choosing the anchor node. Even though (br, after node) and (img, before node) are the theoretically same, there is a case that one is apparently more suitable than the other.

[Ryosuke Niwa]

Created attachment 149630 [details]
Fixes the bug

[Enrica Casucci]

Comment on attachment 149630 [details]
Fixes the bug

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

The change looks good, but I'm puzzled by one of the rebased tests.

> Source/WebCore/ChangeLog:8
> +        Replaced depreactedNode() and deprecatedEditingOffset() by containerNode() and computeOffsetInContainerNode()

typo depreactedNode should be deprecatedNode

> LayoutTests/editing/inserting/font-size-clears-from-typing-style-expected.txt:-2
> -| <div>

Is it ok for this div to disappear?

[Ryosuke Niwa]

Created attachment 149633 [details]
Updated for ToT

[Ryosuke Niwa]

(In reply to comment #8)
> (From update of attachment 149630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149630&action=review
>
> > LayoutTests/editing/inserting/font-size-clears-from-typing-style-expected.txt:-2
> > -| <div>
> 
> Is it ok for this div to disappear?

Yes, there's a div around it with contenteditable attribute, and that div isn't doing anything (i.e. there's no paragraph before/after it).

[Enrica Casucci]

Comment on attachment 149633 [details]
Updated for ToT

Thanks for the reply.

[Ryosuke Niwa]

Committed r121303: <http://trac.webkit.org/changeset/121303>