Bug 13664 – REGRESSION: Extra letter in Webkit/Spell Catcher Shorthand (abbreviation) expansion

• Component: HTML Editing
• Status: RESOLVED FIXED

[Chester Wong]

When composing a message in Gmail with Safari/Webkit or Mailplane (beta), typing a Spell Catcher abbreviation causes the first letter of the abbreviation to be appended to the expansion.  E.g. if I type “btw“ it gets expanded to “bBy the way”. This does not happen in Camino nor in any word processor/text editor that I have tried.

Chet

Safari 2.0.4; Spell Catcher 10.2.3

[David Kilzer (:ddkilzer)]

Chet, does this still happen with a WebKit Nightly build?  http://nightly.webkit.org/

[Evan Gross]

(In reply to comment #0)
> When composing a message in Gmail with Safari/Webkit or Mailplane (beta),
> typing a Spell Catcher abbreviation causes the first letter of the abbreviation
> to be appended to the expansion.  E.g. if I type “btw“ it gets expanded to “bBy
> the way”. This does not happen in Camino nor in any word processor/text editor
> that I have tried.
> 
> Chet
> 
> Safari 2.0.4; Spell Catcher 10.2.3
> 

Hi folks, been a while since I've last looked at the state of text input with WebKit...

First off, tell me whether the Spell Catcher preference (Interactive pane, Typing tab) to "Make replacements directly (without backspacing) where possible" is selected (for Safari if you have it in the Applications drawer) or not.

This preference exists specifically to work-around bug 4681.

Figuring out exactly what's wrong depends largely on whether this preference is selected. It may also be nice to know what happens when you change that preference...

[Chester Wong]

RE: Comment 1

Yes, I have used nightly builds 20057, 20814, & 21240; all with the same result.

RE: Comment 2

Checking the "Make replacements directly (without backspacing) where
possible" causes the abbreviation to remain, followed by a space and the expansion such as below:

btw By the way,

Chet

[Evan Gross]

(In reply to comment #3)
> 
> Checking the "Make replacements directly (without backspacing) where
> possible" causes the abbreviation to remain, followed by a space and the
> expansion such as below:
> 
> btw By the way,
> 

OK, that behavior isn't one I've seen before - at least not with the WebKit builds I was testing with way back when.

Can you try a couple things for me? Select the "Prevent double spaces" in Spell Catcher Preferences, Interactive, Automatic tab. Tell me whether the 2nd..nth space after typing a space are suppressed or not (and whether all or just some are suppressed or "allowed" through.

Might also be useful to know whether the "Suppress separator" option for individual shorthands (checkbox is below the expansion area in the Shorthand editor window) works or not.

Also see whether typing a tab instead of space as the separator behaves any differently.

If WebKit isn't properly suppressing characters, that is a very serious (and new to me) bug that needs to be filed (and fixed!) ASAP. The consequences of this sort of "not suppressing characters when an input method want them suppressed are severe...

Evan

[Chester Wong]

(In reply to comment #4)
l. “Prevent double spaces” has no effect; i.e. typing two or three spaces after a word leaves two or three spaces in the text.

2. "Suppress separator" works as expected but again, the first letter of the abbreviation is left in place.

3. “typing a tab instead of a space” causes some really bizarre things to happen. The first time I tried it, the abbreviation stayed in place but the expansion ended up in the Google Search box at the top of the window!??! I abandoned that draft and opened a new one. This time the abbreviation remained in place but with no expansion and the TAB caused the cursor to flip to the Google Search box. Subsequent attempts to duplicate the results of the first time TAB was used as the separator just caused the cursor to end up in the Google Search box without the expansion.

Mailplane has a preference item to ”Enable rich text editing by using Webkit nightly”. When this item is unchecked (disabled), all is well with the only problem that remains being that of using TAB as the separator. In this case, the expansion is fine but the cursor is still placed in the Google Search box.

Chet

[Evan Gross]

(In reply to comment #5)
> (In reply to comment #4)
> l. “Prevent double spaces” has no effect; i.e. typing two or three spaces after
> a word leaves two or three spaces in the text.
> 
> 2. "Suppress separator" works as expected but again, the first letter of the
> abbreviation is left in place.
> 
> 3. “typing a tab instead of a space” causes some really bizarre things to
> happen. The first time I tried it, the abbreviation stayed in place but the
> expansion ended up in the Google Search box at the top of the window!??! I
> abandoned that draft and opened a new one. This time the abbreviation remained
> in place but with no expansion and the TAB caused the cursor to flip to the
> Google Search box. Subsequent attempts to duplicate the results of the first
> time TAB was used as the separator just caused the cursor to end up in the
> Google Search box without the expansion.
> 
> Mailplane has a preference item to ”Enable rich text editing by using Webkit
> nightly”. When this item is unchecked (disabled), all is well with the only
> problem that remains being that of using TAB as the separator. In this case,
> the expansion is fine but the cursor is still placed in the Google Search box.
> 
> Chet
> 

OK, well, this seems to confirm my guess that keystrokes are not being suppressed when an input method "says" they should be.

This is a very, very bad thing.

Since it's so nasty, I'll try to find time in the next week to build the latest WebKit and actually do some real testing to confirm this and see if I can find a solution. Unless someone else wants to...

Thanks for helping with this, as it absolutely must be fixed or input methods simply aren't going to work properly at all.

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> OK, well, this seems to confirm my guess that keystrokes are not being
> suppressed when an input method "says" they should be.
> 
> This is a very, very bad thing.

Could you write a small HTML test case demonstrating the incorrect behavior?  That would be most helpful.

> Since it's so nasty, I'll try to find time in the next week to build the latest
> WebKit and actually do some real testing to confirm this and see if I can find
> a solution. Unless someone else wants to...

Actually, you may download nightly WebKit builds here:  http://nightly.webkit.org/.  Simply download the DMG, mount it, and double-click on the gold Safari icon to test.

> Thanks for helping with this, as it absolutely must be fixed or input methods
> simply aren't going to work properly at all.

A working test case attached to this bug will be the fastest path (other than writing a patch yourself!) to getting this bug fixed.  Thanks!

[Evan Gross]

(In reply to comment #7)
> Could you write a small HTML test case demonstrating the incorrect behavior? 
> That would be most helpful.
> 

This is often easier said than done wrt text input-related problems...

> Actually, you may download nightly WebKit builds here: 
> http://nightly.webkit.org/.  Simply download the DMG, mount it, and
> double-click on the gold Safari icon to test.
> 

I really needed to see the source to at least begin to figure out what's going on (which I've done, but now have various questions about).

> A working test case attached to this bug will be the fastest path (other than
> writing a patch yourself!) to getting this bug fixed.  Thanks!
> 

I probably can't write it myself without some help - so Help!

- OK, looking at the source, I some questions/comments:

1. There is a comment in -insertText:

        // insertText can be called from an input method or from normal key event processing
        // If its from normal key event processing, we may need to save the action to perform it later.
        // If its from an input method, then we should go ahead and insert the text now.  
        // We assume it's from the input method if we have marked text.
        // FIXME: In theory, this could be wrong for some input methods, so we should try to find
        // another way to determine if the call is from the input method

The FIXME note is absolutely accurate, so this code must be FIXME'd. Testing for marked text is not sufficient.

2. I need to understand the meaning of WebHTMLViewInterpretKeyEventsParameters. What it contains, why it's there, what it's supposed to be used for.

3. I'm not certain what the idea behind the two different code paths for "isFromInputMethod"/"shouldSaveCommand" etc. This is new since I worked on this stuff way back when, so I'm not familiar with why this was implemented.

4. It looks like the shouldSaveCommand flag is related to either UpdateActiveInputArea calls and/or the input method returning whether or not it actually handled an event. Anyone know what this is all about?

Not entirely related, but still relevant, this comment:

    // We don't support inserting an attributed string but input methods don't appear to require this.

will be incorrect in the future. AttributedStrings MUST be supported here.

I have made some changes that fix this particular problem with Spell Catcher, but without knowing exactly what's going on (answers to the above questions, at minimum) have no idea if they will break other input methods...

[Robert Whyte]

Please forward my support for your efforts to get this bug fixed. I rely on Spell Catcher's short hands for most of my work. To me it is a mission critical app. I installed Safari 3 and it has some features that are so compelling I don't want to uninstall it. Most of all, the ability to resize text entry fields on the fly. 

I experience the same problems in Mail as have been described in detail in http://bugs.webkit.org/show_bug.cgi?id=13664 

I get the sense from this thread, that the bug in web kit will be fixed but that it is hard, and may take some time. 

I will vote for the bug to be fixed. 

Good luck. 

My example is below: the letters "siggg" are the text of my shorthand which should be suppressed. So therefore it is a problem here in Safari as well as mail. 

siggg Robert Whyte
Director, Web Development
ToadShow Pty Ltd
e [e-mail]
p 07 3004 7900
f 07 3846 1220
m 0409 055 325
w http://www.toadshow.com.au/

If for any reason your email doesn't get 
through to me, try [e-mail]

[Alexey Proskuryakov]

(In reply to comment #6)
> OK, well, this seems to confirm my guess that keystrokes are not being
> suppressed when an input method "says" they should be.
> 
> This is a very, very bad thing.

I think this is a known bug that's indeed affecting other input methods, and that is being investigated.

Marking as a regression, since Spell Catcher shorthands seem to work fine in shipping Safari (for both AppKit-based text inputs and contenteditable HTML).

[Rocke Woelk]

Please fix webkit to address address SpellCatcher's input issues as outlined here. SpellCatcher is mission critical for so many of us who write for a living. If not fixed in a timely manner, this could be a deal breaker in staying within Apple's platform. 

[David Kilzer (:ddkilzer)]

<rdar://problem/5290113>

[Albert J. Menkveld]

Please fix this issue, as I do currently not use  Apple Safari 3.0, because of this issue. 

-Albert

[Alexey Proskuryakov]

It seems that Oliver's patch didn't fix this problem - I can still reproduce it.

[Evan Gross]

(In reply to comment #14)
> It seems that Oliver's patch didn't fix this problem - I can still reproduce
> it.
> 

What/where's the patch?

I have figured out a way to get this working with Spell Catcher, but due to my lack of knowledge about the various private parameters (WebHTMLViewInterpretKeyEventsParameters ) being passed around (-insertText, -doCommandBySelector), I have no idea whether it's going to break other input methods.

In any case, it looks to me like the value of shouldSaveCommand is the key to figure out whether the text/command is the result of an UpdateActiveInputArea event. AND it looks like it's currently being used incorrectly. And checking whether there's marked text or not to decide whether the event is from an input method is just plain wrong. Input methods can send text via UAIA without maintaining an active input area at all.

I'm hesitant to post a patch without knowing more about the reasoning behind the changes that have been made since I last looked at this stuff. But if you want me to post what I've got, I certainly can.

[Oliver Hunt]

*** Bug 4681 has been marked as a duplicate of this bug. ***

[Oliver Hunt]

Evan, we were doing substantial work to fix issues with international text support, i guess ap thought these would help.  Unfortunately the symptoms experienced by SpellChecker are unrelated to our International text support problems.

Looking at this, you're right it is just the return of 4681.  Technically i should dup this to that, but there's more discussion (and a radar) associated with this so am marking 4681 as a dupe of this.

I wish there was some documentation of these "secret" attributes :-/

It looks like we should be able to do something similar to the replacement of marked text, in this case, but it still irritates me.

[Oliver Hunt]

Evan do you esxpect the space character to be inserted before or after your inputmethod logic occurs?

[Evan Gross]

(In reply to comment #18)
> Evan do you esxpect the space character to be inserted before or after your
> inputmethod logic occurs?
> 

This is what Spell Catcher does when expanding a shorthand abbreviation - as an example say that typing "fyi" should result in "for your information":

1. In an application that does NOT support TSM document access and the required sendRange parameter to the UpdateActiveInputArea event (the current state of WebHTMLView, as it doesn't support the NSTextInputReplacementRangeAttributeName attribute):

- user types f, then y, then i, then a whitespace or newline separator (we'll say it's a space for illustrative purposes).

- Spell Catcher checks the "fyi" when it receives the space typed afterwards, and finds the expansion "for your information".

- After determining it's an abbreviation that needs to be expanded, Spell Catcher SUPPRESSES the space by returning to TSM that it has handled the event in its entirety. THIS IS WHERE WebView is screwing up. No UpdateActiveInputArea event has been sent (yet), the input method is simply returning a non-zero ComponentResult from its Event dispatch call, which MUST be respected by the application. Somehow the event is still getting through, even though TSM has been told that it was handled, and therefore should NOT be handed off to the client app.

- Spell Catcher calculates how many backspace characters need to be posted to backspace over the "fyi" that was typed. REMEMBER, since the space character has been suppressed (normally, in a properly functioning app) only 3 backspaces need to be posted. Backspace and other control characters are handled very differently in Cocoa apps vs. (most) Carbon apps if sent via an UAIA call. We don't want ASCII 8 inserted into the document's text (tab characters pose a particular problem here, but I digress). so we post them with the CGRemote APIs. They need to be handled by the app as physical keystrokes.
=====THIS IS WHY the first letter of the abbreviation is "left behind" - take a look at the original problem reports=====

- NOW, in an app that supports inline input via UpdateActiveInputArea (WebView does at least support this), the expansion "for your information " is sent to the application via an UpdateActiveInputArea call, specifying that all the text should be dumped immediately - no inline area (marked text) should result. This a prime example of text coming from an input method, and there is NO marked text in the document before or afterward.
+++ ALSO NOTE CAREFULLY that the suppressed space following the f y i that was typed, is appended to the expansion - there are a number of important reasons it's done this way, no need to get into that aspect now, though.

So the basic problem stems from the fact that "input method-handled" events are still getting through to and processed by WebView. If an input method wants an event suppressed, the app must obey or really nothing is going to work properly from that point on. This is the basis of many Spell Catcher features - preventing double spaces, turning straight quotes into curly quotes, automatic capitalization changes (in certain circumstances). This is all broken because of this bug, and it's something that can absolutely not be worked-around at the input method level.

The TSM Doc Access-aware situation is far more optimal, as Spell Catcher can avoid backspacing entirely. It just replaces the "fyi " directly in the text with "for your information ". No fuss, no muss, instantaneous. That's why NSTextInputReplacementRangeAttributeName should be supported, but supporting it has absolutely no relevance or relation to fixing this particular problem. I don't think this bug and Bug 4681 are really duplicates. But they are in the same very basic text input handling arena, and both need to be fixed. Fix this one first, then optimize by supporting NSTextInputReplacementRangeAttributeName. Supporting NSTextInputReplacementRangeAttributeName without fixing this may get Spell Catcher to work right, but other input methods that don't look for doc access support in order to perform better will still have this not-suppressing-the-character-I-told-you-to-suppress issues.

Phew!

[Oliver Hunt]

The problem i've encountered is that (afaict) is when handling the replacement i end up with
"fyi " => "fFor your information "

If you are actually inserting the space i'm guessing that we aren't correctly detecting that you have consumed the space.

[Oliver Hunt]

Hmmm, something i've thought i needed to do is treat all key events as being consumed by the IM even if there isn't a marked region.

This fixes the problem, but may have a substantial impact on non-IM based input so will require substantial testing.

[Oliver Hunt]

Righto, there are two bugs involved, one, the fact they we aren't handling NSTextInputReplacementRangeAttributeName in insertText:

The other is bug 14457 which is what causes the first character of the autocorrect trigger to remain after the autocorrect

[Oliver Hunt]

Righto, 14457 should be fixed, which means the backspace route should work.

We now need to make the replacement range work.

[Oliver Hunt]

*** Bug 4681 has been marked as a duplicate of this bug. ***

[Oliver Hunt]

Created attachment 15343 [details]
First run at fixing it

First run at fixing this problem.

[Justin Garcia]

Comment on attachment 15343 [details]
First run at fixing it

     // We don't support inserting an attributed string but input methods don't appear to require this.

Might want to remove this comment.

r=me

[Justin Garcia]

(In reply to comment #26)
> Might want to remove this comment.

Oops nevermind.

[Oliver Hunt]

Created attachment 15344 [details]
Minor update

Whoops, if we get a replacement range we must not be getting "real" keyboard events, so assume it's from an IM.

What i would give to be able to have regression tests for IMs, but don't have time to be able to do the work necessary to get such a system in place.

[Oliver Hunt]

Landed r23926

[Evan Gross]

(In reply to comment #28)
> Created an attachment (id=15344) [edit]
> Minor update
> 
> Whoops, if we get a replacement range we must not be getting "real" keyboard
> events, so assume it's from an IM.
> 
> What i would give to be able to have regression tests for IMs, but don't have
> time to be able to do the work necessary to get such a system in place.
> 

Thanks Oliver! After some testing all seems *almost* perfect. There is an issue with the handling of the replacement range parameter, in that the selection is changed afterwards. This is a definite problem (NSTextView, MLTE, WASTE and BBEdit all maintain the selection properly), as is cannot be assumed that the replacement range is always at the insertion point (and indeed, there may be a selection not just an insertion point).

Should I file a separate bug?

[Oliver Hunt]

Yes, file a separate bug.  I assume this will cause the scrolling problem you referred to in #4681 and possibly an unwanted selection event will be fired -- this possibly hits the marked text path as well.

[Jon Fink]

This is not resolved/fixed. The problem persists in the latest Safari 3.

[Alexey Proskuryakov]

Please use a nightly build from <http://nightly.webkit.org>.

[Oliver Hunt]

Jon, this fix has not been released in a release version of webkit yet, you'll need to test against a nightly -- see http://nightly.webkit.org

[David Kilzer (:ddkilzer)]

(In reply to comment #32)
> This is not resolved/fixed. The problem persists in the latest Safari 3.

Jon, please test this with a recent WebKit nightly and report back the results here.  Thanks!

[David Kilzer (:ddkilzer)]

Could someone please verify that this issue is fixed with WebKit Nightly r23926 or later?  Thanks!

[Chester Wong]

(In reply to comment #36)
> Could someone please verify that this issue is fixed with WebKit Nightly r23926
> or later?  Thanks!
> 
Sorry, I’ve been delinquent on this matter. Webkit r24260 works in Mailplane beta. I have not tried in Mail.app nor Safari 3.0 beta.

Chet

[David Kilzer (:ddkilzer)]

(In reply to comment #37)
> Sorry, I’ve been delinquent on this matter. Webkit r24260 works in Mailplane
> beta. I have not tried in Mail.app nor Safari 3.0 beta.

Thanks!!