RESOLVED FIXED Bug 54535
Stop calling node() and deprecatedEditingOffset() in comparePositions
https://bugs.webkit.org/show_bug.cgi?id=54535
Summary Stop calling node() and deprecatedEditingOffset() in comparePositions
Ryosuke Niwa
Reported 2011-02-16 00:10:13 PST
This is a cleanup.
Attachments
work in progress (5.59 KB, patch)
2011-02-16 00:12 PST, Ryosuke Niwa
no flags
Fixes the bug (13.34 KB, patch)
2012-06-26 16:49 PDT, Ryosuke Niwa
no flags
Updated for ToT (13.34 KB, patch)
2012-06-26 16:56 PDT, Ryosuke Niwa
enrica: review+
Ryosuke Niwa
Comment 1 2011-02-16 00:12:46 PST
Created attachment 82597 [details] work in progress
Ryosuke Niwa
Comment 2 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.
Ryosuke Niwa
Comment 3 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.
Shinya Kawanaka
Comment 4 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.
Ryosuke Niwa
Comment 5 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.
Shinya Kawanaka
Comment 6 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.
Ryosuke Niwa
Comment 7 2012-06-26 16:49:55 PDT
Created attachment 149630 [details] Fixes the bug
Enrica Casucci
Comment 8 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?
Ryosuke Niwa
Comment 9 2012-06-26 16:56:33 PDT
Created attachment 149633 [details] Updated for ToT
Ryosuke Niwa
Comment 10 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).
Enrica Casucci
Comment 11 2012-06-26 16:59:05 PDT
Comment on attachment 149633 [details] Updated for ToT Thanks for the reply.
Ryosuke Niwa
Comment 12 2012-06-26 17:01:00 PDT
Note You need to log in before you can comment on or make changes to this bug.