Bug 4681 – -[WebHTMLView insertText:] needs to support NSTextInputReplacementRangeAttributeName

• Component: HTML Editing
• Status: RESOLVED DUPLICATE

[Evan Gross]

The current comment in -insertText reads:

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

This is not true. In order for kEventClassTextInput/kEventTextInputUpdateActiveInputArea events that 
specify the kEventParamTextInputSendReplaceRange parameter, the "hidden" 
NSTextInputReplacementRangeAttributeName attribute must be extracted and used.

Seems like a simple change to select that range before calling _insertText basically does the trick. There 
are some issues with space runs (a separate problem), so there may be a better way. But the following 
is a big improvement:

- (void)insertText:(id)string
{
    NSString *text;
    if ([string isKindOfClass:[NSAttributedString class]]) {
	text = [string string];
        // we need to support the attribute:NSTextInputReplacementRangeAttributeName if it exists.
        // The AppKit adds a 'secret' property to the string that contains the replacement
        // range.  The replacement range is the range of the text that should be replaced
        // with the new string.
        NSString *rangeString = [string attribute:NSTextInputReplacementRangeAttributeName atIndex:0 
longestEffectiveRange:NULL inRange:NSMakeRange(0, [text length])];
        if (rangeString) {
            [[self _bridge] selectNSRange:NSRangeFromString(rangeString)];
        }
    } else {
	text = string;
    }

    [self _insertText:text selectInsertedText:NO];
}

Again, probably not enough error checking, and instead of calling _insertText some sort of string 
replacement method should be called instead.

But this alone improves things with my input method. Kotoeri also uses this event, if I'm not mistaken. I 
filed a radar on this one long, long ago...

(Note I've edited the comment to read "the" instead of "the the" - this comment was copied and pasted 
from -setMarkedText:selectedRange).

[Evan Gross]

Created attachment 3874 [details]
proposed patch

[Maciej Stachowiak]

Sounds like a good patch, but how to test this? What steps with what input method will demonstrate it?

[Maciej Stachowiak]

Comment on attachment 3874 [details]
proposed patch

The patch looks good but r- for lack of test case or even steps to reproduce.

[Evan Gross]

Indeed, not too many input methods use this.

Here's how to reproduce the problem and verify the fix:

Download Spell Catcher X (my product, the one input method I know that uses this - though I think 
Kotoeri might as well):

<http://www.rainmakerinc.com/downloads/>

Install it, run it in trial mode.

Test with whatever app you like - I use Blot, use Safari with contenteditable, or even try with Mail on 
Tiger.

The critical setting: In Spell Catcher Preferences, Interactive pane, Typing tab, the "Make replacements 
directly..." checkbox controls whether UAIA is called with a replacement range (no backspacing) or not 
(backspaces over the error/shorthand abbreviation).

Run with that checkbox on (the default).
Activate Spell Catcher's input method.
Type a word that will automatically get corrected - like "teh" or "recieve" (US English, anyway)
Try with the shipping WebKit. See that there are remnants of the word you type.
Try with a WebKit built with this patch. See that it works (no remnants, no backspacing).

[Maciej Stachowiak]

Comment on attachment 3874 [details]
proposed patch

Putting back in review? since steps to reproduce are now provided

[Darin Adler]

Comment on attachment 3874 [details]
proposed patch

This patch doesn't really match the title of this bug. It's great to support
NSTextInputReplacementRangeAttributeName, but is that really "supporting
inserting attributed strings"? Another way to ask this is: "Is this the only
attribute we need to support here?" Was this bug originally filed specifically
just for the replacement range attribute or are there other pending issues?

[Evan Gross]

You're quite right, the title of the bug more closely matches the comment that was intially in the code - 
as it say's inserting attributed strings isn't supported because input methods don't require it.

So sure, the basics are that the NSTextInputReplacementRangeAttributeName attribute needs to be 
supported. Other attributes, I honestly have no idea. Not sure if any of the TSM Glyph Access stuff 
might funnel through here - my input method doesn't use it. I think that the Character Palette may 
(insert with font), but haven't tested that a whole lot. I may well take a look, though.

And YES, there's no way my proposed patch is good enough. It's a proof that there is a way to get UAIA 
with kEventParamTextInputSendReplaceRange working in WebView.

There ARE other issues (as stated in my original comment) related to the way space runs are dealt with. 
That's a more general -insertText issue - I have a radar on that filed, and suppose I need to report it 
here. I'll need help on that one, as the whole deal with WebView and spaces/option spaces/space runs 
is a bit mysterious to me.

To correctly fix this one, I need a bit of help as well. PROPERLY implemented, if there's a replacement 
range, the behavior should be the same as how TextEdit's "Paste and Match Style" command works. I 
haven't done enough testing to see whether this patch behaves that way already. I suspect it doesn't. 
The key idea is that replacing a word that's (say) bold should leave you with a bold replacement.

Additionally, we have to make sure that we don't behave like NSTextView in one way - it will scroll the 
selection into view regardless of the replacement range. That's bad, it should be possible to replace any 
arbitrary range, independent of the current selection or insertion point, without any such side-effects.

I'll be taking another look at this in the next day or so, and I'll write up the other issue that insertText 
has in general with text that contains space runs (easy to see for yourself if you do some testing with 
Spell Catcher X, or even TypeIt4Me - don't think any of Apple's input methods will do the trick here).

[Darin Adler]

Comment on attachment 3874 [details]
proposed patch

OK, lets work on a version of this that gets as many as possible of the fine
points Evan mentions working.

[Oliver Hunt]

*** This bug has been marked as a duplicate of 13664 ***

[Alexey Proskuryakov]

Although fixing this issue would hide the symptoms of bug 13664, they are not duplicates (see bug 13664 comment 19).

[Oliver Hunt]

No, this is a duplicate.  The problems seen in 13364 are very blatantly due to us not correctly handling the case where we've been given a replacement range.  That is the bug.

[Oliver Hunt]

Whoops, should be resolved duplicate

[Oliver Hunt]

re-duplicating

*** This bug has been marked as a duplicate of 13664 ***