Bug 54535 - Stop calling node() and deprecatedEditingOffset() in comparePositions
: Stop calling node() and deprecatedEditingOffset() in comparePositions
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
:
: 66120 90011
: 52099 89159 89918
  Show dependency treegraph
 
Reported: 2011-02-16 00:10 PST by
Modified: 2012-06-26 17:01 PST (History)


Attachments
work in progress (5.59 KB, patch)
2011-02-16 00:12 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
Fixes the bug (13.34 KB, patch)
2012-06-26 16:49 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
Updated for ToT (13.34 KB, patch)
2012-06-26 16:56 PST, Ryosuke Niwa
enrica: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-02-16 00:10:13 PST
This is a cleanup.
------- Comment #1 From 2011-02-16 00:12:46 PST -------
Created an attachment (id=82597) [details]
work in progress
------- Comment #2 From 2011-02-16 00:15:18 PST -------
(From update of attachment 82597 [details])
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.
------- Comment #3 From 2011-02-16 00:24:54 PST -------
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.
------- Comment #4 From 2012-06-06 01:38:13 PST -------
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.
------- Comment #5 From 2012-06-06 12:26:03 PST -------
(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.
------- Comment #6 From 2012-06-19 10:45:08 PST -------
(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.
------- Comment #7 From 2012-06-26 16:49:55 PST -------
Created an attachment (id=149630) [details]
Fixes the bug
------- Comment #8 From 2012-06-26 16:56:06 PST -------
(From update of attachment 149630 [details])
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?
------- Comment #9 From 2012-06-26 16:56:33 PST -------
Created an attachment (id=149633) [details]
Updated for ToT
------- Comment #10 From 2012-06-26 16:57:51 PST -------
(In reply to comment #8)
> (From update of attachment 149630 [details] [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).
------- Comment #11 From 2012-06-26 16:59:05 PST -------
(From update of attachment 149633 [details])
Thanks for the reply.
------- Comment #12 From 2012-06-26 17:01:00 PST -------
Committed r121303: <http://trac.webkit.org/changeset/121303>