RESOLVED FIXED 162947
Implement InputEvent.getTargetRanges() for the input events spec
https://bugs.webkit.org/show_bug.cgi?id=162947
Summary Implement InputEvent.getTargetRanges() for the input events spec
Wenson Hsieh
Reported 2016-10-04 16:13:07 PDT
Introduce StaticRange and InputEvent bindings in preparation for the input events spec. See https://www.w3.org/TR/input-events/
Attachments
First pass (55.04 KB, patch)
2016-10-04 16:49 PDT, Wenson Hsieh
no flags
Test patch (46.87 KB, patch)
2016-10-19 13:28 PDT, Wenson Hsieh
no flags
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.13 MB, application/zip)
2016-10-19 15:05 PDT, Build Bot
no flags
Addresses EWS failures and adds layout tests. (60.79 KB, patch)
2016-10-19 21:08 PDT, Wenson Hsieh
no flags
Added ChangeLog entries. (68.29 KB, patch)
2016-10-19 23:35 PDT, Wenson Hsieh
no flags
Tweaked LayoutTest expectations again for WK1. (71.35 KB, patch)
2016-10-20 00:21 PDT, Wenson Hsieh
no flags
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.06 MB, application/zip)
2016-10-20 01:37 PDT, Build Bot
no flags
Archive of layout-test-results from ews122 for ios-simulator-wk2 (14.48 MB, application/zip)
2016-10-20 01:59 PDT, Build Bot
no flags
Tweaked iOS simulator expectations (72.33 KB, patch)
2016-10-20 07:34 PDT, Wenson Hsieh
no flags
Patch (73.91 KB, patch)
2016-10-20 07:42 PDT, Wenson Hsieh
no flags
Test Patch (includes 162947) (95.35 KB, patch)
2016-10-20 19:38 PDT, Wenson Hsieh
no flags
Patch (73.91 KB, patch)
2016-10-20 19:41 PDT, Wenson Hsieh
no flags
Patch for landing (76.62 KB, patch)
2016-10-21 08:19 PDT, Wenson Hsieh
no flags
Wenson Hsieh
Comment 1 2016-10-04 16:49:31 PDT
Created attachment 290669 [details] First pass
Ryosuke Niwa
Comment 2 2016-10-04 17:14:18 PDT
Comment on attachment 290669 [details] First pass View in context: https://bugs.webkit.org/attachment.cgi?id=290669&action=review > Source/WebCore/dom/InputEvent.h:47 > + static Ref<InputEvent> createForBindings() Why do we need this variant? > Source/WebCore/dom/InputEvent.idl:28 > +] interface InputEvent : UIEvent { We should start using this interface where we fire input events for input and textarea elements, and we should have a test validating that the interface type names. > Source/WebCore/dom/InputEvent.idl:32 > + [InitializedByEventConstructor] readonly attribute DOMString inputType; > + > + readonly attribute DataTransfer? dataTransfer; > + sequence<StaticRange> getTargetRanges(); We shouldn't add these attributes until we have a concrete implementation. Otherwise, we'd end up with a broken API when some port, e.g. GTK+ port, ships trunk before we fully bake this feature. > Source/WebCore/dom/InputEventTypeNames.h:32 > +struct InputEventTypeNames { I don't think having a list of AtomicString like this is useful because we'd need a switch statement, etc... to concert between editing command name and these input event type names. I think a better approach would be associating each AtomicString to a specific editor command inside createCommandMap(). > Source/WebCore/dom/Range.h:177 > +Node* checkNodeWOffset(Node&, unsigned offset, ExceptionCode&); Can we just make this a static public member of Range or rename it to checkRangeBoundaryNodeOffset? > Source/WebCore/dom/StaticRange.h:52 > + void setStart(Ref<Node>&& container, int offset); > + void setEnd(Ref<Node>&& container, int offset); Use unsigned instead. > Source/WebCore/dom/StaticRange.idl:31 > +] interface StaticRange { We should hide this feature behind a runtime flag until we have baked the feature enough. Also, we definitely need a test for this. r- because of the lack of tests. > Source/WebCore/dom/StaticRange.idl:42 > + [NewObject, MayThrowLegacyException] Range toRange(); Why don't we use ExceptionOr type in the return type of toRange to avoid using MayThrowLegacyException here?
Ryosuke Niwa
Comment 3 2016-10-04 17:35:35 PDT
Comment on attachment 290669 [details] First pass View in context: https://bugs.webkit.org/attachment.cgi?id=290669&action=review > Source/WebCore/dom/StaticRange.h:61 > + Document& m_document; This needs to be a Ref. You can test this by creating a StaticRange inside an iframe, delete the iframe, and then accessing it.
Wenson Hsieh
Comment 4 2016-10-04 21:54:52 PDT
After talking with Ryosuke in person, we agree that we should hold off on implementing StaticRange in WebKit until the spec is closer to being finalized. In the meantime, we should focus on making InputEvent.inputType work as expected.
Ryosuke Niwa
Comment 5 2016-10-07 12:02:42 PDT
It looks like we need to use dictionary instead so IDL / binding here needs to be completely rewritten :(
Wenson Hsieh
Comment 6 2016-10-19 13:09:46 PDT
Wenson Hsieh
Comment 7 2016-10-19 13:28:08 PDT
Created attachment 292107 [details] Test patch
Build Bot
Comment 8 2016-10-19 15:05:40 PDT
Comment on attachment 292107 [details] Test patch Attachment 292107 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2326610 New failing tests: js/dom/global-constructors-attributes.html
Build Bot
Comment 9 2016-10-19 15:05:46 PDT
Created attachment 292117 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Ryosuke Niwa
Comment 10 2016-10-19 16:56:59 PDT
Comment on attachment 292107 [details] Test patch View in context: https://bugs.webkit.org/attachment.cgi?id=292107&action=review > Source/WebCore/dom/StaticRange.idl:28 > +[ > + ImplementationLacksVTable, > +] interface StaticRange { This IDL needs to be under a runtime flag or a build flag. > Source/WebCore/editing/CompositeEditCommand.cpp:378 > +Vector<RefPtr<StaticRange>> CompositeEditCommand::contentEditableInputEventTargetRanges() const I'd call this simply targetRanges(). > Source/WebCore/editing/CompositeEditCommand.cpp:390 > +Vector<RefPtr<StaticRange>> CompositeEditCommand::inputEventTargetRanges() const And this one targetRangesForBindings. > Source/WebCore/editing/EditCommand.h:70 > + const Frame& frame() const; > + const Document& document() const { return m_document; } Why don't we define these two next to non-const variant? > Source/WebCore/editing/TypingCommand.cpp:458 > + if (range) > + m_currentAffectedRange = StaticRange::createFromRange(*range); > + else > + m_currentAffectedRange = nullptr; It's totally unclear why range is sometimes null and when m_currentAffectedRange is used and when it's not. Why is this needed? > Source/WebCore/editing/TypingCommand.cpp:631 > + if (shouldBreakOutOfEmptyListItem()) { > + if (willAddTypingToOpenCommand(DeleteKey, granularity, { }, Range::create(document(), selection.selection().start().previous(BackwardDeletion), selection.selection().end()))) { We should probably make shouldBreakOutOfEmptyListItem return a selection to delete instead.
Wenson Hsieh
Comment 11 2016-10-19 18:22:54 PDT
Comment on attachment 292107 [details] Test patch View in context: https://bugs.webkit.org/attachment.cgi?id=292107&action=review >> Source/WebCore/dom/StaticRange.idl:28 >> +] interface StaticRange { > > This IDL needs to be under a runtime flag or a build flag. Added a runtime IDL flag for InputEvents, and guarded StaticRange/InputEvents with it. >> Source/WebCore/editing/CompositeEditCommand.cpp:378 >> +Vector<RefPtr<StaticRange>> CompositeEditCommand::contentEditableInputEventTargetRanges() const > > I'd call this simply targetRanges(). Done. >> Source/WebCore/editing/CompositeEditCommand.cpp:390 >> +Vector<RefPtr<StaticRange>> CompositeEditCommand::inputEventTargetRanges() const > > And this one targetRangesForBindings. Done. >> Source/WebCore/editing/EditCommand.h:70 >> + const Document& document() const { return m_document; } > > Why don't we define these two next to non-const variant? Good point -- moved. >> Source/WebCore/editing/TypingCommand.cpp:458 >> + m_currentAffectedRange = nullptr; > > It's totally unclear why range is sometimes null and when m_currentAffectedRange is used and when it's not. > Why is this needed? On second thought, it's not. I could just plumb the range as an argument on Editor::willApplyEditing -- I'll change it to do this. >> Source/WebCore/editing/TypingCommand.cpp:631 >> + if (willAddTypingToOpenCommand(DeleteKey, granularity, { }, Range::create(document(), selection.selection().start().previous(BackwardDeletion), selection.selection().end()))) { > > We should probably make shouldBreakOutOfEmptyListItem return a selection to delete instead. Done.
Wenson Hsieh
Comment 12 2016-10-19 21:08:43 PDT
Created attachment 292149 [details] Addresses EWS failures and adds layout tests.
Wenson Hsieh
Comment 13 2016-10-19 23:35:37 PDT
Created attachment 292155 [details] Added ChangeLog entries.
Wenson Hsieh
Comment 14 2016-10-20 00:21:12 PDT
Created attachment 292157 [details] Tweaked LayoutTest expectations again for WK1.
Build Bot
Comment 15 2016-10-20 01:37:34 PDT
Comment on attachment 292157 [details] Tweaked LayoutTest expectations again for WK1. Attachment 292157 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2329253 New failing tests: js/dom/global-constructors-attributes.html
Build Bot
Comment 16 2016-10-20 01:37:39 PDT
Created attachment 292158 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Build Bot
Comment 17 2016-10-20 01:59:07 PDT
Comment on attachment 292157 [details] Tweaked LayoutTest expectations again for WK1. Attachment 292157 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2329272 New failing tests: fast/events/before-input-delete-text-target-ranges.html fast/events/before-input-replace-text-target-ranges.html
Build Bot
Comment 18 2016-10-20 01:59:12 PDT
Created attachment 292160 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Wenson Hsieh
Comment 19 2016-10-20 07:34:56 PDT
Created attachment 292180 [details] Tweaked iOS simulator expectations
Wenson Hsieh
Comment 20 2016-10-20 07:42:22 PDT
Wenson Hsieh
Comment 21 2016-10-20 19:38:53 PDT
Created attachment 292311 [details] Test Patch (includes 162947)
Wenson Hsieh
Comment 22 2016-10-20 19:41:25 PDT
Darin Adler
Comment 23 2016-10-20 22:27:29 PDT
Comment on attachment 292312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292312&action=review > Source/WebCore/dom/InputEvent.idl:28 > + EnabledAtRuntime=InputEvents, > Constructor(DOMString type, optional InputEventInit eventInitDict), Please keep extended attributes in alphabetical order. > Source/WebCore/dom/StaticRange.h:30 > +#include "Node.h" > +#include <wtf/RefCounted.h> > +#include <wtf/RefPtr.h> Please make the includes be minimal. I do not think Node.h needs to be included, but I am not sure. Would need to try it and see if adding that include elsewhere was sufficient. If Node.h is included, then RefCounted.h and RefPtr.h do not need to be included. > Source/WebCore/dom/StaticRange.h:53 > + RefPtr<Node> m_startContainer; This should be Ref<Node>, not RefPtr<Node>. > Source/WebCore/dom/StaticRange.h:55 > + RefPtr<Node> m_endContainer; Ditto. > Source/WebCore/editing/CompositeEditCommand.cpp:381 > + if (auto firstRange = frame().selection().selection().firstRange()) { We normally use early return, which means the error case comes first, rather than nesting the normal case inside an if statement. > Source/WebCore/editing/CompositeEditCommand.cpp:384 > + Vector<RefPtr<StaticRange>> ranges; > + ranges.append(StaticRange::createFromRange(*firstRange)); > + return ranges; I would write this instead: RefPtr<StaticRange> range = StaticRange::createFromRange(*firstRange); return { 1, &range }; > Source/WebCore/editing/CompositeEditCommand.cpp:1542 > + RefPtr<Node> emptyListItem = enclosingEmptyListItem(endingSelection().visibleStart()); > + RefPtr<ContainerNode> listNode = emptyListItem->parentNode(); > + RefPtr<EditingStyle> style = EditingStyle::create(endingSelection().start()); These should use auto. > Source/WebCore/editing/CompositeEditCommand.h:32 > +#include "StaticRange.h" This include is not needed here. A forward declaration should suffice. > Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:80 > + Vector<RefPtr<StaticRange>> ranges; > + ranges.append(StaticRange::createFromRange(*m_rangeToBeReplaced)); > + return ranges; I would write this instead: RefPtr<StaticRange> range = StaticRange::createFromRange(*m_rangeToBeReplaced); return { 1, &range }; > Source/WebCore/editing/SpellingCorrectionCommand.cpp:126 > + Vector<RefPtr<StaticRange>> ranges; > + ranges.append(StaticRange::createFromRange(*m_rangeToBeCorrected)); > + return ranges; I would write this instead: RefPtr<StaticRange> range = StaticRange::createFromRange(*m_rangeToBeCorrected); return { 1, &range }; > Source/WebCore/editing/TypingCommand.cpp:450 > + Vector<RefPtr<StaticRange>> staticRanges; > + staticRanges.append(StaticRange::createFromRange(*range)); > + return frame().editor().willApplyEditing(*this, WTFMove(staticRanges)); I would write something more like this instead: RefPtr<StaticRange> staticRange = StaticRange::createFromRange(*range); return frame().editor().willApplyEditing(*this, { 1, &staticRanges });
Wenson Hsieh
Comment 24 2016-10-21 08:03:00 PDT
Comment on attachment 292312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292312&action=review >> Source/WebCore/dom/InputEvent.idl:28 >> Constructor(DOMString type, optional InputEventInit eventInitDict), > > Please keep extended attributes in alphabetical order. Fixed. >> Source/WebCore/dom/StaticRange.h:30 >> +#include <wtf/RefPtr.h> > > Please make the includes be minimal. I do not think Node.h needs to be included, but I am not sure. Would need to try it and see if adding that include elsewhere was sufficient. > > If Node.h is included, then RefCounted.h and RefPtr.h do not need to be included. Good point. Removed Node.h include and moved the implementation of StaticRange::create to StaticRange.cpp. >> Source/WebCore/dom/StaticRange.h:53 >> + RefPtr<Node> m_startContainer; > > This should be Ref<Node>, not RefPtr<Node>. Changed to use Ref<Node> >> Source/WebCore/dom/StaticRange.h:55 >> + RefPtr<Node> m_endContainer; > > Ditto. Changed to use Ref<Node> >> Source/WebCore/editing/CompositeEditCommand.cpp:381 >> + if (auto firstRange = frame().selection().selection().firstRange()) { > > We normally use early return, which means the error case comes first, rather than nesting the normal case inside an if statement. Done. >> Source/WebCore/editing/CompositeEditCommand.cpp:384 >> + return ranges; > > I would write this instead: > > RefPtr<StaticRange> range = StaticRange::createFromRange(*firstRange); > return { 1, &range }; Good call! Done. >> Source/WebCore/editing/CompositeEditCommand.cpp:1542 >> + RefPtr<EditingStyle> style = EditingStyle::create(endingSelection().start()); > > These should use auto. Done. >> Source/WebCore/editing/CompositeEditCommand.h:32 >> +#include "StaticRange.h" > > This include is not needed here. A forward declaration should suffice. Done. >> Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:80 >> + return ranges; > > I would write this instead: > > RefPtr<StaticRange> range = StaticRange::createFromRange(*m_rangeToBeReplaced); > return { 1, &range }; This is really clean! Changed all similar call sites to do this. >> Source/WebCore/editing/SpellingCorrectionCommand.cpp:126 >> + return ranges; > > I would write this instead: > > RefPtr<StaticRange> range = StaticRange::createFromRange(*m_rangeToBeCorrected); > return { 1, &range }; Done. >> Source/WebCore/editing/TypingCommand.cpp:450 >> + return frame().editor().willApplyEditing(*this, WTFMove(staticRanges)); > > I would write something more like this instead: > > RefPtr<StaticRange> staticRange = StaticRange::createFromRange(*range); > return frame().editor().willApplyEditing(*this, { 1, &staticRanges }); Done.
Wenson Hsieh
Comment 25 2016-10-21 08:19:25 PDT
Created attachment 292347 [details] Patch for landing
WebKit Commit Bot
Comment 26 2016-10-21 08:54:14 PDT
Comment on attachment 292347 [details] Patch for landing Clearing flags on attachment: 292347 Committed r207670: <http://trac.webkit.org/changeset/207670>
WebKit Commit Bot
Comment 27 2016-10-21 08:54:21 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.