Bug 205378

Summary: TextManipulationController should respect new token orders
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, mifenton, thorton, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
WIP2
none
Implements the new behavior wenson_hsieh: review+

Ryosuke Niwa
Reported 2019-12-17 23:25:27 PST
TextManipulationController::completeManipulation should use the new order of tokens.
Attachments
WIP (10.31 KB, patch)
2019-12-17 23:26 PST, Ryosuke Niwa
no flags
WIP2 (15.63 KB, patch)
2019-12-19 13:21 PST, Ryosuke Niwa
no flags
Implements the new behavior (26.30 KB, patch)
2019-12-20 01:16 PST, Ryosuke Niwa
wenson_hsieh: review+
Ryosuke Niwa
Comment 1 2019-12-17 23:26:25 PST
Ryosuke Niwa
Comment 2 2019-12-19 13:21:29 PST
Ryosuke Niwa
Comment 3 2019-12-20 01:16:24 PST
Created attachment 386188 [details] Implements the new behavior
Ryosuke Niwa
Comment 4 2019-12-20 01:20:39 PST
Wenson Hsieh
Comment 5 2019-12-20 17:19:10 PST
Comment on attachment 386188 [details] Implements the new behavior View in context: https://bugs.webkit.org/attachment.cgi?id=386188&action=review > Source/WebCore/editing/TextManipulationController.cpp:279 > + RefPtr<Node> parent; I found it confusing that `!parent` means that we should append the child node to a separate insertion position instead, but I can't really think of an alternative that doesn't end up making this more complicated :/ Maybe add a comment to clarify this? > Source/WebCore/editing/TextManipulationController.cpp:369 > + if (i == currentElementStack.size() && i == currentAncestors.size()) { > + insertions.append(NodeInsertion { currentElementStack.size() ? currentElementStack.last().ptr() : nullptr, contentNode.releaseNonNull() }); > + } else { Nit - no braces?
Ryosuke Niwa
Comment 6 2019-12-20 17:20:18 PST
(In reply to Wenson Hsieh from comment #5) > Comment on attachment 386188 [details] > Implements the new behavior > > View in context: > https://bugs.webkit.org/attachment.cgi?id=386188&action=review > > > Source/WebCore/editing/TextManipulationController.cpp:279 > > + RefPtr<Node> parent; > > I found it confusing that `!parent` means that we should append the child > node to a separate insertion position instead, but I can't really think of > an alternative that doesn't end up making this more complicated :/ > > Maybe add a comment to clarify this? Good point. Maybe rename it to parentIfDifferentFromCommonAncestor? > > Source/WebCore/editing/TextManipulationController.cpp:369 > > + if (i == currentElementStack.size() && i == currentAncestors.size()) { > > + insertions.append(NodeInsertion { currentElementStack.size() ? currentElementStack.last().ptr() : nullptr, contentNode.releaseNonNull() }); > > + } else { > > Nit - no braces? Will fix.
Wenson Hsieh
Comment 7 2019-12-20 17:21:05 PST
Comment on attachment 386188 [details] Implements the new behavior View in context: https://bugs.webkit.org/attachment.cgi?id=386188&action=review >>> Source/WebCore/editing/TextManipulationController.cpp:279 >>> + RefPtr<Node> parent; >> >> I found it confusing that `!parent` means that we should append the child node to a separate insertion position instead, but I can't really think of an alternative that doesn't end up making this more complicated :/ >> >> Maybe add a comment to clarify this? > > Good point. Maybe rename it to parentIfDifferentFromCommonAncestor? Yeah, I think that's a clearer name, given the behavior.
Ryosuke Niwa
Comment 8 2019-12-20 18:09:52 PST
Note You need to log in before you can comment on or make changes to this bug.