Bug 54535 - Stop calling node() and deprecatedEditingOffset() in comparePositions
: Stop calling node() and deprecatedEditingOffset() in comparePositions
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To: Ryosuke Niwa
:
Depends on: 66120 90011
Blocks: 52099 89159 89918
  Show dependency treegraph
 
Reported: 2011-02-16 00:10 PST by Ryosuke Niwa
Modified: 2012-06-26 17:01 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-02-16 00:10:13 PST
This is a cleanup.
Comment 1 Ryosuke Niwa 2011-02-16 00:12:46 PST
Created attachment 82597 [details]
work in progress
Comment 2 Ryosuke Niwa 2011-02-16 00:15:18 PST
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.
Comment 3 Ryosuke Niwa 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 Shinya Kawanaka 2012-06-06 01:38:13 PDT
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 Ryosuke Niwa 2012-06-06 12:26:03 PDT
(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 Shinya Kawanaka 2012-06-19 10:45:08 PDT
(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 Ryosuke Niwa 2012-06-26 16:49:55 PDT
Created attachment 149630 [details]
Fixes the bug
Comment 8 Enrica Casucci 2012-06-26 16:56:06 PDT
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?
Comment 9 Ryosuke Niwa 2012-06-26 16:56:33 PDT
Created attachment 149633 [details]
Updated for ToT
Comment 10 Ryosuke Niwa 2012-06-26 16:57:51 PDT
(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).
Comment 11 Enrica Casucci 2012-06-26 16:59:05 PDT
Comment on attachment 149633 [details]
Updated for ToT

Thanks for the reply.
Comment 12 Ryosuke Niwa 2012-06-26 17:01:00 PDT
Committed r121303: <http://trac.webkit.org/changeset/121303>