WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Test patch
(46.87 KB, patch)
2016-10-19 13:28 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
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
Details
Addresses EWS failures and adds layout tests.
(60.79 KB, patch)
2016-10-19 21:08 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Added ChangeLog entries.
(68.29 KB, patch)
2016-10-19 23:35 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Tweaked LayoutTest expectations again for WK1.
(71.35 KB, patch)
2016-10-20 00:21 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
Tweaked iOS simulator expectations
(72.33 KB, patch)
2016-10-20 07:34 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Patch
(73.91 KB, patch)
2016-10-20 07:42 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Test Patch (includes 162947)
(95.35 KB, patch)
2016-10-20 19:38 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Patch
(73.91 KB, patch)
2016-10-20 19:41 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Patch for landing
(76.62 KB, patch)
2016-10-21 08:19 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/28853079
>
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
Created
attachment 292185
[details]
Patch
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
Created
attachment 292312
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug