WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
Committed
r121303
: <
http://trac.webkit.org/changeset/121303
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug