Bug 164595 – Composition state should be cleared when changing focus to a non-editable element

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

[Wenson Hsieh]

Created attachment 294374 [details]
Begin a composition in the input, then tap the select

Composition state (i.e. the composition node and underlines) are not cleared when changing focus to a non-editable element, since selection is None at that point.

[Wenson Hsieh]

Created attachment 294379 [details]
Patch

[Wenson Hsieh]

<rdar://problem/26412551>

[Wenson Hsieh]

Created attachment 294380 [details]
Rebased on ToT

[Build Bot]

Comment on attachment 294380 [details]
Rebased on ToT

Attachment 294380 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2491681

New failing tests:
editing/focus-change-with-marked-text.html

[Build Bot]

Created attachment 294383 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 294380 [details]
Rebased on ToT

Attachment 294380 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2491701

New failing tests:
editing/focus-change-with-marked-text.html

[Build Bot]

Created attachment 294384 [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

[David Kilzer (:ddkilzer)]

Comment on attachment 294380 [details]
Rebased on ToT

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

> Tools/ChangeLog:34
> +        * DumpRenderTree/mac/TextInputControllerMac.m: Renamed from Tools/DumpRenderTree/mac/TextInputController.m.

Technically, this should go in /cocoa/ folder instead of /mac/, but that doesn't have to be done in this patch.

> Tools/DumpRenderTree/TextInputControllerIOS.m:2
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

This should go in /ios/ folder.

[Build Bot]

Comment on attachment 294380 [details]
Rebased on ToT

Attachment 294380 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2491748

New failing tests:
editing/focus-change-with-marked-text.html
storage/domstorage/sessionstorage/blocked-file-access.html

[Build Bot]

Created attachment 294386 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 294380 [details]
Rebased on ToT

Attachment 294380 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2491732

New failing tests:
editing/focus-change-with-marked-text.html

[Build Bot]

Created attachment 294388 [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 on attachment 294380 [details]
Rebased on ToT

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

I'm also looking into why these tests fail on Mac. It's likely the semantics of -setMarkedText: and -unmarkText are just different across the two platforms, so we might need to disable this on Mac.

>> Tools/DumpRenderTree/TextInputControllerIOS.m:2
>> + * Copyright (C) 2016 Apple Inc. All rights reserved.
> 
> This should go in /ios/ folder.

Good catch! Moved to ios/

[Wenson Hsieh]

Created attachment 294404 [details]
Patch

[Enrica Casucci]

Comment on attachment 294404 [details]
Patch

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

This is awesome!

> Tools/DumpRenderTree/TextInputController.h:2
> + * Copyright (C) 2005 Apple Inc.  All rights reserved.

Fix copyright year.

> Tools/DumpRenderTree/TextInputController.h:31
> +// FIXME: <rdar://problem/5106287> DumpRenderTree: fix TextInputController to work with iOS and re-enable tests

Is this radar now fixed? If so you should move it to integrate and reference it in the change log as well. Remove or update this comment.

> Tools/DumpRenderTree/mac/TextInputControllerMac.m:2
> + * Copyright (C) 2005, 2007 Apple Inc.  All rights reserved.

Update copyright year

[Wenson Hsieh]

Comment on attachment 294404 [details]
Patch

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

>> Tools/DumpRenderTree/TextInputController.h:2
>> + * Copyright (C) 2005 Apple Inc.  All rights reserved.
> 
> Fix copyright year.

Good catch! Fixed.

>> Tools/DumpRenderTree/TextInputController.h:31
>> +// FIXME: <rdar://problem/5106287> DumpRenderTree: fix TextInputController to work with iOS and re-enable tests
> 
> Is this radar now fixed? If so you should move it to integrate and reference it in the change log as well. Remove or update this comment.

Updated the comment -- there's still work to be done, since TextInputController on iOS isn't still up to parity with Mac (there are many commands that need to be implemented) but this is definitely progress towards that bug. I'll comment in <rdar://problem/5106287> as well.

>> Tools/DumpRenderTree/mac/TextInputControllerMac.m:2
>> + * Copyright (C) 2005, 2007 Apple Inc.  All rights reserved.
> 
> Update copyright year

Fixed!

[Wenson Hsieh]

Created attachment 294504 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 294504 [details]
Patch for landing

Clearing flags on attachment: 294504

Committed r208593: <http://trac.webkit.org/changeset/208593>

[JF Bastien]

FYI this broke the CMake build, I'm fixing the TextInputControllerMac.m rename here:
  https://bugs.webkit.org/show_bug.cgi?id=164526

I haven't checked whether there's further breakage of CMake.

Simple repro:

    mkdir out
    xcrun cmake -Wno-dev .. -G Ninja \
          -DCMAKE_BUILD_TYPE=Release \
          -DPORT=Mac
    ninja TestWTF

[Wenson Hsieh]

(In reply to comment #19)
> FYI this broke the CMake build, I'm fixing the TextInputControllerMac.m
> rename here:
>   https://bugs.webkit.org/show_bug.cgi?id=164526
> 
> I haven't checked whether there's further breakage of CMake.
> 
> Simple repro:
> 
>     mkdir out
>     xcrun cmake -Wno-dev .. -G Ninja \
>           -DCMAKE_BUILD_TYPE=Release \
>           -DPORT=Mac
>     ninja TestWTF

Sorry about that -- I forgot to update the cmake file during my refactoring. Thank you for fixing this!

[JF Bastien]

(In reply to comment #20)
> (In reply to comment #19)
> > FYI this broke the CMake build, I'm fixing the TextInputControllerMac.m
> > rename here:
> >   https://bugs.webkit.org/show_bug.cgi?id=164526
> > 
> > I haven't checked whether there's further breakage of CMake.
> > 
> > Simple repro:
> > 
> >     mkdir out
> >     xcrun cmake -Wno-dev .. -G Ninja \
> >           -DCMAKE_BUILD_TYPE=Release \
> >           -DPORT=Mac
> >     ninja TestWTF
> 
> Sorry about that -- I forgot to update the cmake file during my refactoring.
> Thank you for fixing this!

No worries, it was an easy fix!