Bug 85840

Summary: REGRESSION (r101575): Chinese input is broken when composing mail in iCloud using Safari
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: HTML EditingAssignee: Enrica Casucci <enrica>
Status: RESOLVED FIXED    
Severity: Normal CC: rniwa
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: Unspecified   
Attachments:
Description Flags
Patch ap: review+

Description Enrica Casucci 2012-05-07 16:04:01 PDT
* STEPS TO REPRODUCE
1. login to iCloud using Safari
2. compose a new mail
3. choose Pinyin - Simplified as input source
4. type "jintian" in the mail body

* RESULTS
After step4, "jin ti" is committed directly and the candidate window shows the candidates for "an".

<rdar://problem/11115520>
Comment 1 Enrica Casucci 2012-05-07 16:11:07 PDT
Created attachment 140615 [details]
Patch
Comment 2 Alexey Proskuryakov 2012-05-07 16:14:28 PDT
Comment on attachment 140615 [details]
Patch

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

Enrica and myself looked into making a regression test, and determined it impractical. We don't want adding new tests for DOM modification events, and simulating what the page does without mutation events cannot be tested because we don't have appropriate input context hooks.

> Source/WebCore/editing/InsertTextCommand.cpp:119
> +        deleteSelection(false, true, true, false, false);

We should make this readable by using enums or a parameter struct.
Comment 3 Enrica Casucci 2012-05-07 16:19:19 PDT
(In reply to comment #2)
> (From update of attachment 140615 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140615&action=review
> 
> Enrica and myself looked into making a regression test, and determined it impractical. We don't want adding new tests for DOM modification events, and simulating what the page does without mutation events cannot be tested because we don't have appropriate input context hooks.
> 
> > Source/WebCore/editing/InsertTextCommand.cpp:119
> > +        deleteSelection(false, true, true, false, false);
> 
> We should make this readable by using enums or a parameter struct.

I agree. It was already very unreadable. I want to get rid of all these parameters and replace them with an enum whose values can be combined with an or. I will do that in a separate patch.
Thanks for the review.
Comment 4 Enrica Casucci 2012-05-07 16:27:36 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 140615 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=140615&action=review
> > 
> > Enrica and myself looked into making a regression test, and determined it impractical. We don't want adding new tests for DOM modification events, and simulating what the page does without mutation events cannot be tested because we don't have appropriate input context hooks.
> > 
> > > Source/WebCore/editing/InsertTextCommand.cpp:119
> > > +        deleteSelection(false, true, true, false, false);
> > 
> > We should make this readable by using enums or a parameter struct.
> 
> I agree. It was already very unreadable. I want to get rid of all these parameters and replace them with an enum whose values can be combined with an or. I will do that in a separate patch.
> Thanks for the review.

I want to cleanup deleteSelection in CompositeEditCommand and DeleteSelectionCommand, to avoid keeping separate member variables.
Comment 5 Ryosuke Niwa 2012-05-10 01:17:37 PDT
Closing the bug since the patch appears to have been landed in http://trac.webkit.org/changeset/116368.