Bug 162947 – Implement InputEvent.getTargetRanges() for the input events spec

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

Introduce StaticRange and InputEvent bindings in preparation for the input events spec. See https://www.w3.org/TR/input-events/

[Wenson Hsieh]

Created attachment 290669 [details]
First pass

[Ryosuke Niwa]

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 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]

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]

It looks like we need to use dictionary instead so IDL / binding here needs to be completely rewritten :(

[Wenson Hsieh]

<rdar://problem/28853079>

[Wenson Hsieh]

Created attachment 292107 [details]
Test patch

[Build Bot]

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]

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 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 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]

Created attachment 292149 [details]
Addresses EWS failures and adds layout tests.

[Wenson Hsieh]

Created attachment 292155 [details]
Added ChangeLog entries.

[Wenson Hsieh]

Created attachment 292157 [details]
Tweaked LayoutTest expectations again for WK1.

[Build Bot]

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]

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 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]

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]

Created attachment 292180 [details]
Tweaked iOS simulator expectations

[Wenson Hsieh]

Created attachment 292185 [details]
Patch

[Wenson Hsieh]

Created attachment 292311 [details]
Test Patch (includes 162947)

[Wenson Hsieh]

Created attachment 292312 [details]
Patch

[Darin Adler]

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 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]

Created attachment 292347 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 292347 [details]
Patch for landing

Clearing flags on attachment: 292347

Committed r207670: <http://trac.webkit.org/changeset/207670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.