Bug 57649 – Make WebKit2 text input handling more like WebKit1

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Alexey Proskuryakov]

We cache too much data in UI process, so input methods that ask for information get stale results.

[Alexey Proskuryakov]

Created attachment 87897 [details]
proposed patch

I know of two big remaining differences:
- attributedSubstringFromRange is not implemented (breaking a lot of stuff);
- no equivalent to WebResponderChainSink (not sure what exactly that breaks).

[Darin Adler]

Comment on attachment 87897 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87897&action=review

Patch doesn’t apply so you are not getting the benefit of EWS.

> Source/WebCore/dom/KeyboardEvent.h:42
> -        String commandName;
> +        String commandName; // Actually, a selector name - it may have a trailing colon, and a name that can be different from an editor command name.

Can we change the data member name to selector or selectorString? Is this used only with selectors, or is it sometimes used for command names?

> Source/WebKit2/Shared/mac/TextInputState.h:42
> +};

This semicolon is incorrect.

> Source/WebKit2/Shared/mac/TextInputState.h:45
> +
> +
> +#endif // TextInputState_h

Extra blank line.

> Source/WebKit2/UIProcess/WebPageProxy.h:256
> +    void setComposition(const String& text, Vector<WebCore::CompositionUnderline> underlines, uint64_t selectionStart, uint64_t selectionEnd, uint64_t replacementRangeStart, uint64_t replacementRangeEnd, TextInputState& newState);

const Vector& instead of Vector?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:109
> +};
> +
> +

Extra blank line here.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1063
> +/*
> +        // Make sure that only direct calls to doCommandBySelector: see the parameters by setting to 0.
> +        _data->interpretKeyEventsParameters = 0;
> +
> +        bool eventWasHandled;
> +
> +        Editor::Command command = [self coreCommandBySelector:selector];
> +        if (command.isSupported())
> +            eventWasHandled = command.execute(event);
> +        else {
> +            // If WebKit does not support this command, we need to pass the selector to super.
> +            _private->selectorForDoCommandBySelector = selector;
> +
> +            // The sink does two things: 1) Tells us if the responder went unhandled, and
> +            // 2) prevents any NSBeep; we don't ever want to beep here.
> +            WebResponderChainSink *sink = [[WebResponderChainSink alloc] initWithResponderChain:self];
> +            [super doCommandBySelector:selector];
> +            eventWasHandled = ![sink receivedUnhandledCommand];
> +            [sink detach];
> +            [sink release];
> +
> +            _private->selectorForDoCommandBySelector = 0;
> +        }
> +
> +        if (parameters)
> +            parameters->eventInterpretationHadSideEffects |= eventWasHandled;
> +
> +        _private->interpretKeyEventsParameters = parameters;
> +*/

Are you really planning on checking in all this code commented out? Is there some better placeholder?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1084
> +        // FIXME: We ignore most attributes from the string, so for example inserting from Character Palette loses font and glyph variation data.
> +        // It does not look like any input methods ever use insertText: with attributes other than NSTextInputReplacementRangeAttributeName.

Those two comments seem almost contradictory. Maybe the point is that Character Palette is not an input method.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1102
> +        parameters->commands->append(KeypressCommand("insertText:", text));

Can we use a string constant for insertText:?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1230
> +    if (result.location == NSNotFound)
> +        LOG(TextInput, "selectedRange -> (NSNotFound, %u)", result.length);
> +    else
> +        LOG(TextInput, "selectedRange -> (%u, %u)", result.location, result.length);

I believe that %u is incorrect for 64-bit for NSUInteger; surprised it works. I see code elsewhere making the same mistake.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:168
> -#if !PLATFORM(MAC)
> +#if PLATFORM(MAC)
> +    bool handleEditingKeyboardEvent(WebCore::KeyboardEvent*, bool saveCommands);
> +#else
>      bool handleEditingKeyboardEvent(WebCore::KeyboardEvent*);
>  #endif

I wish there was some way we could unify these.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:97
> +    map->add("insertNewlineIgnoringFieldEditor:", "InsertNewline");
> +    map->add("insertParagraphSeparator:", "InsertNewline");
> +    map->add("insertTabIgnoringFieldEditor:", "InsertTab");
> +    map->add("pageDown:", "MovePageDown");
> +    map->add("pageDownAndModifySelection:", "MovePageDownAndModifySelection");
> +    map->add("pageUp:", "MovePageUp");
> +    map->add("pageUpAndModifySelection:", "MovePageUpAndModifySelection");

It’s a bit annoying to have the mapping from selectors to command names here rather than in the class where the selectors are supported, WKView. I see that you want to avoid converting selectors to editor command names because they may round trip back to the UI process, but still it’s not great to have the rules about selectors distributed between classes.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:155
> +            // FIXME: WebHTMLView sends the event up the responder chain with WebResponderChainSink if it's not supported by the editor. Should we do the same?

Strange that this comment is here, but the commented-out code is over in WKView.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:523
> +    // FIXME: Commoon behaviors like scrolling down on Space should probably be implemented in WebCore.

Typo: "Commoon".

[Alexey Proskuryakov]

> Can we change the data member name to selector or selectorString? Is this used only with selectors, or is it sometimes used for command names?

Yes, we should. Not in this patch, since I don't want to add a huge cross-project rename to it.

> Are you really planning on checking in all this code commented out? Is there some better placeholder?

Just an oversight. I'm not sure when having this code this really matters - I do not remember ever seeing doCommandBySelector: called outside keyboard event processing or by input methods in practice.

> Those two comments seem almost contradictory. Maybe the point is that Character Palette is not an input method.

Yes, that's the idea (it's an input source, but not an input method). The line seems blurred, or maybe I'm just confused about terminology.

> > +        parameters->commands->append(KeypressCommand("insertText:", text));
> 
> Can we use a string constant for insertText:?

I think that we should use a separate KeypressCommand constructor, but didn't' want to change KeypressCommand too much yet.

> I believe that %u is incorrect for 64-bit for NSUInteger; surprised it works. I see code elsewhere making the same mistake.

Hmm. I'll keep it as is for now then, would hate to break 32-bit build.

[Enrica Casucci]

Comment on attachment 87897 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87897&action=review

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1253
>  }

Is this method called only for input methods or can it be called also when typing without using input methods? Is there a risk we might be making a synchronous call for each keystroke? I completely understand why you did it this way, but I would like to make sure what I suspect isn't true.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/82717>.

[Alexey Proskuryakov]

Comment on attachment 87897 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87897&action=review

>> Source/WebKit2/UIProcess/API/mac/WKView.mm:1253
>>  }
> 
> Is this method called only for input methods or can it be called also when typing without using input methods? Is there a risk we might be making a synchronous call for each keystroke? I completely understand why you did it this way, but I would like to make sure what I suspect isn't true.

We'd still have non-zero parameters pointer in that case, and would return a value from parameters->cachedTextInputState.hasMarkedText. For regular typing, the sequence of method calls is validAttributesForMarkedText, hasMarkedText, insertText: - so there are no saved command to execute in hasMarkedText, and there is a cached state to return.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82717 might have broken Leopard Intel Debug (Build)

[Alexey Proskuryakov]

The ever surprising string[0] issue... Fixed already in r82720.