Bug 164369 - Safari does not emit composition end if blurred for dead key / Japanese IME
Summary: Safari does not emit composition end if blurred for dead key / Japanese IME
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks: 163112
  Show dependency treegraph
 
Reported: 2016-11-03 10:29 PDT by Wenson Hsieh
Modified: 2016-11-05 10:57 PDT (History)
4 users (show)

See Also:


Attachments
First pass (23.67 KB, patch)
2016-11-03 21:20 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (24.61 KB, patch)
2016-11-04 13:25 PDT, Wenson Hsieh
rniwa: review+
Details | Formatted Diff | Diff
Patch for landing (24.60 KB, patch)
2016-11-04 16:16 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2016-11-03 10:29:56 PDT
1. Change IME mode to Japanese Hiragana
2. Type a composition string into a text field or content editable, but do not commit
3. Blur the editable element by clicking the document or another text field

No `compositionend` is fired for the editable element.

Firefox/Chrome both fire a `compositionend` in this case.
Comment 1 Wenson Hsieh 2016-11-03 21:20:06 PDT
Created attachment 293854 [details]
First pass
Comment 2 Wenson Hsieh 2016-11-03 21:20:38 PDT
<rdar://problem/29050439>
Comment 3 Ryosuke Niwa 2016-11-04 12:22:06 PDT
Comment on attachment 293854 [details]
First pass

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

> Source/WebCore/editing/Editor.cpp:3250
> +    if (oldSelection == newSelection)
> +        return;

We should check this condition at this function is called instead.
We already have such a check & an early exit in setSelection.

> Source/WebCore/editing/Editor.cpp:3252
> +#if PLATFORM(MAC)

Why is this behavior Mac only?
Comment 4 Wenson Hsieh 2016-11-04 12:42:11 PDT
Comment on attachment 293854 [details]
First pass

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

>> Source/WebCore/editing/Editor.cpp:3250
>> +        return;
> 
> We should check this condition at this function is called instead.
> We already have such a check & an early exit in setSelection.

Sounds good -- I'll move it there.

>> Source/WebCore/editing/Editor.cpp:3252
>> +#if PLATFORM(MAC)
> 
> Why is this behavior Mac only?

When canceling a composition, we also need to reset the platform's IME state so that the platform doesn't still think that we have a pending composition. On Mac, we accomplish this by discarding the NSTextInputContext's marked text. However, there is no analogous way to accomplish this on iOS.

Furthermore, this bug seems to only affect Mac. On iOS, we behave differently when changing selection while there is composition text -- we will commit the composition _without_ changing the focused element, which actually produces the correct behavior of dispatching a `compositionend` on the element containing the composition. For instance, if a user types a composition string into a field on iOS without confirming it and taps to switch input fields, the old input field will remain focused and the composition will be committed. Tapping again (when there is no pending composition) then switches focus. I'm not sure if this is a bug or intended behavior -- if this is a bug, we can actually fix this problem by committing the composition early here, but due to the inability to clear composition state on iOS, doing so will require more investigation.
Comment 5 Ryosuke Niwa 2016-11-04 12:45:01 PDT
Comment on attachment 293854 [details]
First pass

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

>>> Source/WebCore/editing/Editor.cpp:3252
>>> +#if PLATFORM(MAC)
>> 
>> Why is this behavior Mac only?
> 
> When canceling a composition, we also need to reset the platform's IME state so that the platform doesn't still think that we have a pending composition. On Mac, we accomplish this by discarding the NSTextInputContext's marked text. However, there is no analogous way to accomplish this on iOS.
> 
> Furthermore, this bug seems to only affect Mac. On iOS, we behave differently when changing selection while there is composition text -- we will commit the composition _without_ changing the focused element, which actually produces the correct behavior of dispatching a `compositionend` on the element containing the composition. For instance, if a user types a composition string into a field on iOS without confirming it and taps to switch input fields, the old input field will remain focused and the composition will be committed. Tapping again (when there is no pending composition) then switches focus. I'm not sure if this is a bug or intended behavior -- if this is a bug, we can actually fix this problem by committing the composition early here, but due to the inability to clear composition state on iOS, doing so will require more investigation.

In that case, we should add an empty implementation in Editor.cpp (wrapped in !PLATFORM(MAC))
and this version in EditorMac.mm.
Comment 6 Wenson Hsieh 2016-11-04 12:47:04 PDT
> In that case, we should add an empty implementation in Editor.cpp (wrapped
> in !PLATFORM(MAC))
> and this version in EditorMac.mm.

That makes sense. Will do!
Comment 7 Wenson Hsieh 2016-11-04 13:25:38 PDT
Created attachment 293918 [details]
Patch
Comment 8 Ryosuke Niwa 2016-11-04 16:13:11 PDT
Comment on attachment 293918 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:308
> +    bool selectionsAreEqual = oldSelection == newSelection;

I think it's better to negate the variable and call it didMutateSelection or something.
Comment 9 Wenson Hsieh 2016-11-04 16:16:44 PDT
Created attachment 293953 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2016-11-04 16:57:26 PDT
The commit-queue encountered the following flaky tests while processing attachment 293953 [details]:

inspector/sampling-profiler/call-frame-with-dom-functions.html bug 164399 (author: sbarati@apple.com)
The commit-queue is continuing to process your patch.
Comment 11 WebKit Commit Bot 2016-11-04 16:58:01 PDT
Comment on attachment 293953 [details]
Patch for landing

Clearing flags on attachment: 293953

Committed r208406: <http://trac.webkit.org/changeset/208406>