Bug 163948

<rdar://problem/28921433>

Created attachment 292764 [details] WIP (includes 163213)

Comment on attachment 292764 [details] WIP (includes 163213) Attachment 292764 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2368758 New failing tests: editing/pasteboard/subframe-dragndrop-1.html editing/pasteboard/drag-drop-modifies-page.html editing/pasteboard/smart-drag-drop.html

Created attachment 292775 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 292764 [details] WIP (includes 163213) Attachment 292764 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2368710 New failing tests: editing/pasteboard/subframe-dragndrop-1.html editing/pasteboard/drag-drop-modifies-page.html editing/pasteboard/smart-drag-drop.html

Created attachment 292777 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 292830 [details] First pass

Comment on attachment 292830 [details] First pass View in context: https://bugs.webkit.org/attachment.cgi?id=292830&action=review > Source/WebCore/editing/CompositeEditCommand.h:109 > - EditCommandComposition* composition() { return m_composition.get(); } > + EditCommandComposition* topLevelComposition() const; > + EditCommandComposition* composition() const { return m_composition.get(); } This doesn't make any sense. EditCommandComposition should always be created by the top-level CompositeEditCommand.

Comment on attachment 292830 [details] First pass View in context: https://bugs.webkit.org/attachment.cgi?id=292830&action=review >> Source/WebCore/editing/CompositeEditCommand.h:109 >> + EditCommandComposition* composition() const { return m_composition.get(); } > > This doesn't make any sense. EditCommandComposition should always be created by the top-level CompositeEditCommand. topLevelComposition() fetches the composition of the top-level command. m_composition will still be 0 for a CompositeEditCommand that is not at the top level.

Comment on attachment 292830 [details] First pass View in context: https://bugs.webkit.org/attachment.cgi?id=292830&action=review > Source/WebCore/editing/Editor.cpp:1087 > + if (command.defersInputEventsToChildCommands()) > + return true; I think it's more intuitive to have shouldFireInputEvent and make it return true iff it's the top-level command, and then override it in MoveSelectionCommand. > LayoutTests/fast/events/before-input-events-prevent-drag-and-drop.html:48 > + moveToCenter(source); > + eventSender.leapForward(500); > + eventSender.mouseDown(); > + eventSender.leapForward(1000); > + moveToCenter(destination); > + eventSender.leapForward(500); > + eventSender.mouseUp(); You should wrap this code inside if (window.eventSender). > LayoutTests/fast/events/before-input-events-prevent-drag-and-drop.html:53 > + copy.innerHTML = event.dataTransfer.getData("text/html"); Surely, you meant textContent. Otherwise, any markup you've included in the html won't show in the output. > LayoutTests/fast/events/input-events-drag-and-drop.html:20 > + <script src="../../resources/js-test-pre.js"></script> It really doesn't much sense this test to be including js-test-pre.js given it doesn't use should* function at all. Alternatively, you can log a list of events that got fired, and assert that those events fired in the order but that's a bit tricky to make it work both using eventSender and during manual testing. > LayoutTests/fast/events/input-events-drag-and-drop.html:29 > + description("To manually test this, drag the text in the first input into the second, then undo (cmd-z) and observe the output."); You should describe what output is expected. > LayoutTests/fast/events/input-events-drag-and-drop.html:35 > + debug("* * * Performing drag and drop * * *"); Instead of using * to separate, why don't we just have an empty debug('') to insert a new line? > LayoutTests/fast/events/input-events-drag-and-drop.html:51 > + function handleInput(event) This is more like logInputEvent. > LayoutTests/fast/events/input-events-insert-by-drop.html:4 > + <script src="../../resources/js-test-pre.js"></script> It really doesn't much sense this test to be including js-test-pre.js given it doesn't use should* function at all. > LayoutTests/fast/events/input-events-insert-by-drop.html:6 > +<style> Putting a style element between head & body is strange. Just put it inside body.

Comment on attachment 292830 [details] First pass Attachment 292830 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2370554 New failing tests: fast/forms/drag-into-textarea.html fast/forms/drag-out-of-textarea.html

Created attachment 292840 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 292830 [details] First pass Attachment 292830 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2370755 New failing tests: fast/forms/drag-into-textarea.html fast/forms/drag-out-of-textarea.html

Created attachment 292844 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 292830 [details] First pass View in context: https://bugs.webkit.org/attachment.cgi?id=292830&action=review >> Source/WebCore/editing/Editor.cpp:1087 >> + return true; > > I think it's more intuitive to have shouldFireInputEvent and make it return true iff it's the top-level command, and then override it in MoveSelectionCommand. I like this idea! Done. >> LayoutTests/fast/events/before-input-events-prevent-drag-and-drop.html:48 >> + eventSender.mouseUp(); > > You should wrap this code inside if (window.eventSender). Good catch -- done. >> LayoutTests/fast/events/before-input-events-prevent-drag-and-drop.html:53 >> + copy.innerHTML = event.dataTransfer.getData("text/html"); > > Surely, you meant textContent. Otherwise, any markup you've included in the html won't show in the output. The original intent was actually to make `copy` mirror the contents of the first content editable, but on second thought, setting textContent to event.dataTransfer.getData("text/html") is a better idea. Changed. >> LayoutTests/fast/events/input-events-drag-and-drop.html:20 >> + <script src="../../resources/js-test-pre.js"></script> > > It really doesn't much sense this test to be including js-test-pre.js given it doesn't use should* function at all. > Alternatively, you can log a list of events that got fired, and assert that those events fired in the order > but that's a bit tricky to make it work both using eventSender and during manual testing. This is a good point. Really the only thing I use them for here is debug(). I'll remove the js-test-* script includes. >> LayoutTests/fast/events/input-events-drag-and-drop.html:29 >> + description("To manually test this, drag the text in the first input into the second, then undo (cmd-z) and observe the output."); > > You should describe what output is expected. Added more detail to the descriptions in all 3 tests. >> LayoutTests/fast/events/input-events-drag-and-drop.html:35 >> + debug("* * * Performing drag and drop * * *"); > > Instead of using * to separate, why don't we just have an empty debug('') to insert a new line? Done. >> LayoutTests/fast/events/input-events-drag-and-drop.html:51 >> + function handleInput(event) > > This is more like logInputEvent. Renamed. >> LayoutTests/fast/events/input-events-insert-by-drop.html:4 >> + <script src="../../resources/js-test-pre.js"></script> > > It really doesn't much sense this test to be including js-test-pre.js given it doesn't use should* function at all. I agree -- removed the js-test-* includes. >> LayoutTests/fast/events/input-events-insert-by-drop.html:6 >> +<style> > > Putting a style element between head & body is strange. Just put it inside body. Oh, good catch! Fixed.

Created attachment 292861 [details] Fix unit tests and address feedback

Comment on attachment 292861 [details] Fix unit tests and address feedback Attachment 292861 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2371635 New failing tests: fast/events/focus-change-crash2.html fast/forms/focus-change-on-keypress.html

Created attachment 292865 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 292868 [details] Fix bad assertion

Comment on attachment 292868 [details] Fix bad assertion View in context: https://bugs.webkit.org/attachment.cgi?id=292868&action=review > Source/WebCore/editing/CompositeEditCommand.cpp:416 > + auto* command = this; > + while (command->parent()) { > + ASSERT(!command->m_composition); > + command = command->parent(); > + } > + return command->m_composition.get(); This loop could be written in a more straightforward manner. I would suggest this version: for (auto* command = this; command; command = command->parent()) if (auto* composition = command->m_composition) { ASSERT(!command->parent()); return composition; } } return nullptr; But I can also imagine other versions that are closer to what you wrote, but without evaluating command->parent() twice. > Source/WebCore/editing/EditCommand.cpp:183 > + if (EditCommandComposition* composition = compositionIfPossible(cmd)) I suggest auto* here. > Source/WebCore/editing/EditCommand.cpp:194 > + if (EditCommandComposition* composition = compositionIfPossible(cmd)) I suggest auto* here.

Comment on attachment 292868 [details] Fix bad assertion View in context: https://bugs.webkit.org/attachment.cgi?id=292868&action=review >> Source/WebCore/editing/CompositeEditCommand.cpp:416 >> + return command->m_composition.get(); > > This loop could be written in a more straightforward manner. I would suggest this version: > > for (auto* command = this; command; command = command->parent()) > if (auto* composition = command->m_composition) { > ASSERT(!command->parent()); > return composition; > } > } > return nullptr; > > But I can also imagine other versions that are closer to what you wrote, but without evaluating command->parent() twice. Sounds good! Done. >> Source/WebCore/editing/EditCommand.cpp:183 >> + if (EditCommandComposition* composition = compositionIfPossible(cmd)) > > I suggest auto* here. Done. >> Source/WebCore/editing/EditCommand.cpp:194 >> + if (EditCommandComposition* composition = compositionIfPossible(cmd)) > > I suggest auto* here. Done.

Created attachment 293067 [details] Patch for landing

Comment on attachment 293067 [details] Patch for landing Clearing flags on attachment: 293067 Committed r208014: <http://trac.webkit.org/changeset/208014>

All reviewed patches have been landed. Closing bug.