Bug 162947

Summary: Implement InputEvent.getTargetRanges() for the input events spec
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebCore Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, buildbot, cdumez, commit-queue, dbates, enrica, esprehn+autocc, kangil.han, kondapallykalyan, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 163112    
Attachments:
Description Flags
First pass
none
Test patch
none
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Addresses EWS failures and adds layout tests.
none
Added ChangeLog entries.
none
Tweaked LayoutTest expectations again for WK1.
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Tweaked iOS simulator expectations
none
Patch
none
Test Patch (includes 162947)
none
Patch
none
Patch for landing none

Description Wenson Hsieh 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/
Comment 1 Wenson Hsieh 2016-10-04 16:49:31 PDT
Created attachment 290669 [details]
First pass
Comment 2 Ryosuke Niwa 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?
Comment 3 Ryosuke Niwa 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.
Comment 4 Wenson Hsieh 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.
Comment 5 Ryosuke Niwa 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 :(
Comment 6 Wenson Hsieh 2016-10-19 13:09:46 PDT
<rdar://problem/28853079>
Comment 7 Wenson Hsieh 2016-10-19 13:28:08 PDT
Created attachment 292107 [details]
Test patch
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Ryosuke Niwa 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.
Comment 11 Wenson Hsieh 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.
Comment 12 Wenson Hsieh 2016-10-19 21:08:43 PDT
Created attachment 292149 [details]
Addresses EWS failures and adds layout tests.
Comment 13 Wenson Hsieh 2016-10-19 23:35:37 PDT
Created attachment 292155 [details]
Added ChangeLog entries.
Comment 14 Wenson Hsieh 2016-10-20 00:21:12 PDT
Created attachment 292157 [details]
Tweaked LayoutTest expectations again for WK1.
Comment 15 Build Bot 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
Comment 16 Build Bot 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
Comment 17 Build Bot 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
Comment 18 Build Bot 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
Comment 19 Wenson Hsieh 2016-10-20 07:34:56 PDT
Created attachment 292180 [details]
Tweaked iOS simulator expectations
Comment 20 Wenson Hsieh 2016-10-20 07:42:22 PDT
Created attachment 292185 [details]
Patch
Comment 21 Wenson Hsieh 2016-10-20 19:38:53 PDT
Created attachment 292311 [details]
Test Patch (includes 162947)
Comment 22 Wenson Hsieh 2016-10-20 19:41:25 PDT
Created attachment 292312 [details]
Patch
Comment 23 Darin Adler 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 });
Comment 24 Wenson Hsieh 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.
Comment 25 Wenson Hsieh 2016-10-21 08:19:25 PDT
Created attachment 292347 [details]
Patch for landing
Comment 26 WebKit Commit Bot 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>
Comment 27 WebKit Commit Bot 2016-10-21 08:54:21 PDT
All reviewed patches have been landed.  Closing bug.