<rdar://61830589> 0 com.apple.WebCore 0x00000001b14059aa WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool) + 11514 1 com.apple.WebCore 0x00000001b13f27e7 WebCore::ApplyStyleCommand::applyBlockStyle(WebCore::EditingStyle&) + 4423 2 com.apple.WebCore 0x00000001b01b5c5d WebCore::CompositeEditCommand::apply() + 397 3 com.apple.WebCore 0x00000001b142aee1 WebCore::Editor::applyParagraphStyle(WebCore::StyleProperties*, WebCore::EditAction) + 433 4 com.apple.WebCore 0x00000001b145b783 WebCore::executeApplyParagraphStyle(WebCore::Frame&, WebCore::EditorCommandSource, WebCore::EditAction, WebCore::CSSPropertyID, WTF::String const&) + 131 5 com.apple.WebCore 0x00000001b1455726 WebCore::executeJustifyLeft(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 54 6 com.apple.WebCore 0x00000001b021c2b1 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 81 7 com.apple.WebCore 0x00000001b05f21ea WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 426
Test case: <style> q { -webkit-user-select: none; -webkit-border-image: url() } label { direction: rtl; } </style> <body><label contenteditable="true" style="-webkit-appearance: button"><q id=q>a</q> <script> window.getSelection().selectAllChildren(q); document.execCommand("justifyLeft", false); </script> Root cause: 1. In processing “selectAllChildren(q)”, because Q is user-select:none, the selection is moved to #text “\n”. 2. In processing justifyLeft command for element Q, we create and insert DIV and BR. 3. Then we call moveParagraphs to move #text “\n” into firstPositionInNode(DIV). 4. However, in finding firstPositionInNode(DIV), because DIV is also user-select:none, and no other candidate is found, we pass an empty position to moveParagraphs. 5. The null anchorNode in the empty position is dereferenced later in the function. BODY 0x60c000088780 (renderer 0x612000068bc0) LABEL 0x60c000088840 (renderer 0x612000068d40) STYLE=-webkit-appearance: button Q 0x60c000088900 (renderer 0x6110000d7400) #text 0x60800004e420 "a" * DIV 0x60c0000a21c0 (renderer 0x61200006b740) BR 0x60c0000a2280 (renderer 0x6110000dab00) #text 0x60800004e4a0 "\n" SCRIPT 0x610000032740 (renderer 0x0) #text 0x60800004e520 "\n window.getSelection().selectAllChildren(q);\n document.execCommand("justifyLeft", false);\n"
There are two options for the patch: 1. Check for empty position in moveParagraphs and returns early. However, it is a little bit late because at this point we have already inserted DIV and BR. Besides, we would return the newly created DIV element back to the caller. 2. Check the node where DIV is being inserted to see if it is userSelect:None and return early. However, I am not sure if this is equivalent to checking the candidate later after DIV and BR are inserted.
> 1. In processing “selectAllChildren(q)”, because Q is user-select:none, the > selection is moved to #text “\n”. > 2. In processing justifyLeft command for element Q, we create and insert DIV > and BR. It's a little strange that the DIV and BR are inserted into Q, even though Q is not selected. My naive take is that we should have tried to left-justify only the selected "\n" (which is basically a no-op).
Yeah, the code insert DIV to upstreamStart, instead of the selected position (paragraph start). Position upstreamStart = visibleParagraphStart.deepEquivalent().upstream(); ... insertNewDefaultParagraphElementAt(upstreamStart); (In reply to Geoffrey Garen from comment #3) > It's a little strange that the DIV and BR are inserted into Q, even though Q > is not selected. My naive take is that we should have tried to left-justify > only the selected "\n" (which is basically a no-op).
Created attachment 398111 [details] Patch
Please see the patch for two patch options. Geoff also brought up a good point: should upstream() skip the user-select:none elements? If a node is not selectable, does it makes sense to return its position? (In reply to Jack from comment #5) > Created attachment 398111 [details] > Patch
Created attachment 398163 [details] Patch
Function moveParagraphContentsToNewBlockIfNecessary checks if upstreamStart is editable or not. Added check for user-select:none at the same line. (In reply to Jack from comment #7) > Created attachment 398163 [details] > Patch
Comment on attachment 398163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398163&action=review > Source/WebCore/editing/CompositeEditCommand.cpp:1204 > - if (!isEditablePosition(upstreamStart)) > + if (!isEditablePosition(upstreamStart) || Position::nodeIsUserSelectNone(upstreamStart.containerNode())) > return nullptr; I'm curious what happen is you change isEditablePosition() to check nodeIsUserSelectNone(). That seems like a cleaner fix to me, if it passes tests, since we have other code that checks isEditablePosition. I don't love the idea of adding a check for nodeIsUserSelectNone() only in moveParagraphContentsToNewBlockIfNecessary(). That seems like an arbitrary difference from other editing code.
Comment on attachment 398111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398111&action=review > Source/WebCore/editing/CompositeEditCommand.cpp:1399 > + if (destination.isNull() || startOfParagraphToMove == destination) > return; I don't really mind this null check. It seems like this function is already pretty lenient, since it allows a caller to specify startOfParagraphToMove == destination. Still, it seems like a bug for a caller to ask to move to an invalid destination. Surely the caller would have preferred to pick a valid destination, if it had known? I guess I think we should try adding the nodeIsUserSelectNone() check to isEditablePosition(), and see if that passes tests. If it does pass tests, let's go with that. Otherwise, let's add this null check. Mechanically, what causes a user-select: none element to be a null VisiblePosition? Which other piece of code is checking nodeIsUserSelectNone() and concluding that the position is not visible? That might help us understand whether isEditablePosition() should change to match.
(In reply to Geoffrey Garen from comment #10) > Comment on attachment 398111 [details] > Patch > > Mechanically, what causes a user-select: none element to be a null > VisiblePosition? Which other piece of code is checking > nodeIsUserSelectNone() and concluding that the position is not visible? That > might help us understand whether isEditablePosition() should change to match. NodeIsUserSelectNone is only called in isCandidate(), except in EventHandler::updateSelectionForMouseDownDispatchingSelectStart, probably as a patch as well. In isCandidate(), node is checked for editable style by calling hasEditableStyle, which uses the same function computeEditability, as isEditablePosition. However, in function isCandidate(), different nodes can be used to check editable and select styles -- m_anchorNode is always used for editable style, and for select style, deprecatedNode() or its parentNode() can be checked. Therefore, the behavior may be different if we check m_anchorNode for both editable and select styles.
(In reply to Jack from comment #11) > (In reply to Geoffrey Garen from comment #10) > > Comment on attachment 398111 [details] > > Patch > > > > > Mechanically, what causes a user-select: none element to be a null > > VisiblePosition? Which other piece of code is checking > > nodeIsUserSelectNone() and concluding that the position is not visible? That > > might help us understand whether isEditablePosition() should change to match. > > NodeIsUserSelectNone is only called in isCandidate() In this test case, which position ends up being null? Is it upstreamStart? Or firstPositionInNode(newBlock.ptr())? Or something else?
(In reply to Geoffrey Garen from comment #12) > (In reply to Jack from comment #11) > In this test case, which position ends up being null? Is it upstreamStart? > Or firstPositionInNode(newBlock.ptr())? Or something else? firstPositionInNode(newBlock.ptr()) returns empty position. upstreamStart is the #text in Q. newBlock is DIV in Q.
> upstreamStart is the #text in Q. If Position::isCandidate rejects nodes that are user-select: none, and the #text in Q is user-select: none (inherited from Q), how did upstreamStart get set to the #text in Q to begin with? How is it even possible to construct such a Position? What is visibleParagraphStart?
(In reply to Geoffrey Garen from comment #14) > > upstreamStart is the #text in Q. > > If Position::isCandidate rejects nodes that are user-select: none, and the > #text in Q is user-select: none (inherited from Q), how did upstreamStart > get set to the #text in Q to begin with? How is it even possible to > construct such a Position? In function upstream, there is no user-select check, although it does check editability. > What is visibleParagraphStart? It is #text "\n"
> > If Position::isCandidate rejects nodes that are user-select: none, and the > > #text in Q is user-select: none (inherited from Q), how did upstreamStart > > get set to the #text in Q to begin with? How is it even possible to > > construct such a Position? > In function upstream, there is no user-select check, although it does check > editability. I see. I believed, incorrectly, that upstream() called isCandidate(). But it doesn't. Tantalizingly, upstream() does call endsOfNodeAreVisuallyDistinctPositions(), which says this: // FIXME: Share code with isCandidate, if possible. static bool endsOfNodeAreVisuallyDistinctPositions(Node* node) 🤨
(In reply to Geoffrey Garen from comment #16) > // FIXME: Share code with isCandidate, if possible. > static bool endsOfNodeAreVisuallyDistinctPositions(Node* node) > > 🤨 Does it means it is time to FIXME? :-) So should we try checking selectable in endsOfNodeAreVisuallyDistinctPositions?
OK, here's my thinking: I think we should fix this bug by changing CompositeEditCommand::moveParagraphs to return early if destination is null. Because nothing mechanically prevents someone from passing null to moveParagraphs, we might as well check for it, so that a minor bug in an edit operation is just a no-op, rather than a crash. That said, Node::computeEditability() should probably return Editability::ReadOnly if the node is user-select: none. Logically, you can't edit if you can't select. And in our implementation, we seem to expect only to edit what we can select. We don't necessarily have to pursue this change, but if you have some time to test it, and it passes all tests, it might be a neat improvement.
(In reply to Geoffrey Garen from comment #18) > OK, here's my thinking: > > I think we should fix this bug by changing > CompositeEditCommand::moveParagraphs to return early if destination is null. > Because nothing mechanically prevents someone from passing null to > moveParagraphs, we might as well check for it, so that a minor bug in an > edit operation is just a no-op, rather than a crash. Sounds great! Yes, you are right, it's already happening in another radar where DeleteSelectionCommand::mergeParagraphs calls moveParagraphs with empty destination. > That said, Node::computeEditability() should probably return > Editability::ReadOnly if the node is user-select: none. Logically, you can't > edit if you can't select. And in our implementation, we seem to expect only > to edit what we can select. We don't necessarily have to pursue this change, > but if you have some time to test it, and it passes all tests, it might be a > neat improvement. Got it. I will put this on back burner when bug count is low.
Created attachment 398249 [details] Patch
Comment on attachment 398249 [details] Patch r=me
Committed r261126: <https://trac.webkit.org/changeset/261126> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398249 [details].