Bug 4682 – -[WebHTMLView firstRectForCharacterRange:] is using _selectedRange instead of the given range if no marked text

• Component: HTML Editing
• Status: RESOLVED FIXED

[Evan Gross]

In order for the kEventClassTextInput/kEventTextInputOffsetToPos event to work properly with non-
zero offsets when there is no current inline input area, firstRectForCharacterRange MUST use the given 
range (theRange). CarbonEvents.h states that text engines should use the 
kEventParamTextInputSendTextOffset as an insertion point-relative offset in the absence of an active 
input area.

A simple change so that the ![self hasMarkedText] case also calls [bridge 
convertToObjCDOMRange:theRange] is basically doing the trick for my input method. Some range 
checking needs to be done, I suppose, and I really don't know exactly what convertToObjCDOMRange is 
*exactly* doing, but this seems to be a step in the right direction. Much better than always ignoring the 
kEventParamTextInputSendTextOffset and using the insertion point/selection (the current behavior), 
anyway.

[Alexey Proskuryakov]

FWIW, Tiger's Dictionary popup uses kEventTextInputOffsetToPos, and seems to work fine with non-zero 
offsets regardless of the presence of an inline input area. Both negative and positive 
kEventParamTextInputSendTextOffsets appear to work.

Could you please describe some steps to reproduce this issue?

[Evan Gross]

(In reply to comment #1)
> FWIW, Tiger's Dictionary popup uses kEventTextInputOffsetToPos, and seems to work fine with non-
zero 
> offsets regardless of the presence of an inline input area. Both negative and positive 
> kEventParamTextInputSendTextOffsets appear to work.
> 
> Could you please describe some steps to reproduce this issue?

This is incorrect. The Dictionary service (I corresponded with the engineer at Apple that wrote it) uses 
private APIs and Accessibility to work in WebViews. It does NOT use kEventTextInputOffsetToPos - 
although it does seem to at least "try" to use it - but then probably gives up on it when the start and 
end h offsets are identical (for the beginning and end of a word range).

I maintain that it is absolutely broken in WebView's.

[Alexey Proskuryakov]

Yes, apparently, Dictionary indeed tries and gives up on kEventTextInputOffsetToPos, just as you describe.

[Evan Gross]

Created attachment 3872 [details]
proposed patch

[Darin Adler]

Comment on attachment 3872 [details]
proposed patch

This strange behavior is necessary to make certain input methods work.

See Richard Williamson's 2005-03-23 check-in comment.

The problem reported was:

<TCIM> CangJie: the candidate window appears at the top left hand corner during
typing in Mail and iChat

1. Can be reproduced under setting : "Show Associated Words" chosen, Dynamic
Prompt chosen/not chosen, Candidate Window direction vertical/horizontal
2. Open Mail
3. Type ??? ( CangJie code: onf vnd rsqf)

* RESULTS
the candidate window appears at the top left hand corner. see attachment

We'll need to test this case if we want to change this function. Ironically,
the Radar bug has a comment from me saying:

"But this leaves the firstRectforCharacterRange method behaving very strangely,
and ignoring the range parameter passed in. We'll have to revisit this
eventually."

[Evan Gross]

I'd like to see the screen shot that was attached to that radar - I'm not noticing any difference at all in this 
area when running with the shipping WebKit framework and the latest (as of yesterday, I think) built with 
this proposed patch (or latest without the patch).

Could just be I'm missing something since I don't have access to that entire report. Can anyone else 
confirm/deny/reproduce?

[Darin Adler]

Created attachment 4028 [details]
here's the picture mentioned in that Radar bug report

I remember this was hard to fix. There's a lot more discussion (much of it
confused) and many related bugs in Radar. Maybe the picture alone will help.

[Evan Gross]

(In reply to comment #7)
> here's the picture mentioned in that Radar bug report
> 
> I remember this was hard to fix. There's a lot more discussion (much of it
> confused) and many related bugs in Radar. Maybe the picture alone will help.

Are you sure this is actually a current, open, reproducible problem? I don't see this behavior - do you?

You say it was hard to fix, which implies it's no longer an issue. What are we on the lookout for here? I 
don't see this regression with my patch, either (though I recently did notice one odd thing that I'm 
researching - more later on that, but it's definitely NOT the same thing as shown in this picture).

[Evan Gross]

More comments here, from recent discoveries for bug 4680. Basically pasted from my comments over 
there, as they seem more relevant to firstRectForCharacterRange. It's intimately tied to 4680 
(attributedSubstringFromRange).

If an input method sends kEventTextInputOffsetToPos with a negative offset (to get info for a character 
offset before the insertion point), firstRectForCharacterRange gets called with theRange.location set 
(seemingly) appropriately, but with theRange.length = 1. This is not the case with positive offsets 
(where theRange.length is always zero).

Not sure if this is as-designed or a NSTSMInputContext bug. Not quite sure what to make of this, there 
are a couple of issues that I suppose should be filed against firstRectForCharacterRange, though they 
intimately involve the results (or lack thereof) from attributedSubstringFromRange.

1. When kEventTextInputOffsetToPos is called with a positive offset, firstRectForCharacterRange gets 
called with theRange.length set to zero. Fine - sort of. When theRange.length == 0, and there's only an 
insertion point, attributedSubstringFromRange is NOT called to figure what to return for 
kEventParamTextInputReplyFMFont, kEventParamTextInputReplyPointSize, 
kEventParamTextInputReplyLineHeight or kEventParamTextInputReplyLineAscent. So the values 
returned are some sort of defaults - and in general, they're wrong.

2. When kEventTextInputOffsetToPos is called with a negative offset, firstRectForCharacterRange gets 
called with theRange.length set to one (no clue why).
- The Good: When theRange.length > 0, attributedSubstringFromRange IS called to calculate the four 
font/style/line-related parameters.
- The Bad: With this patch, anyway, IF the range given to firstRectForCharacterRange happens to be a 
space character (OK for non-breaking space, but this whole spaces/nbsp thing, while necessary, just 
seems a big mess - but I digress), attributedSubstringFromRange will return a zero-length string. This 
doesn't jive with what TSM seems to expect for the range it supplied (a string of length 
theRange.length, I'd guess). So somewhere in TSM, wherever it expects to be able to extract the above 
four return parameters for kEventTextInputOffsetToPos, it barfs on the zero-length string. An out-of-
bounds exception is thrown, and control never returns to the input method (arguably a TSM bug in that 
it should catch the exception so it can always return control back to the IM). Basically, this is akin to a 
crash.

Solutions?

+ The alternate method to obtain the attributed string:

NSAttributedString *attributedString = [bridge attributedStringFrom:[range startContainer] startOffset:
[range startOffset] to:[range endContainer] endOffset:[range endOffset]];

At least deals with the space character problem. Not sure it's good in all situations.

+ Seems we can avoid the exception being thrown simply by testing for a zero-length attributed string 
result for a non-zero theRange.length, and return nil instead of the empty string. May also apply to 
other length mismatches, at least if the attributed string length is less than theRange.length.

But it's an avoidance approach, not really a fix.

Suggestions, anyone?

[Alexey Proskuryakov]

(In reply to comment #7)

Could it be that TCIM worked fine, but it was firstRectForCharacterRange that returned a zero rect for 
whatever reason? There's even code for that (resultRect = NSMakeRect(0,0,0,0)).

As for the multiple problems with kEventTextInputOffsetToPos - it would help to have some test 
application attached to this bug - otherwise, there's no way to reproduce the problems or to test the 
solution...

[Evan Gross]

Created attachment 4162 [details]
Input Method for testing various issues

Here's a modified version of the BasicInputMethod sample, enhanced to do the
following:

1. The Send Event Palette can send OffsetToPos with positive/zero/negative
offsets. Good for this particular bug.
2. Various enhancements to the inline input:

- Can do click-positioning of the caret via PosToOffset
- Arrow keys work
- Drag-selecting within inline area works (well, not in NSTextView, radar filed
long, long ago)
- Shift-arrow extends selection
- You can type spaces and tabs into the inline text (good for showing all the
bugs with input methods sending whitespace vs. typing spaces and tabs - radars
filed long, long ago).

A bit buggy, you may need to open and close a window to get it to activate
properly at times.

To see the various behaviors with non-zero offsets, just show the send event
palette, play with the offset when there's no active inline input, and see what
happens in firstRectForCharacterRange and attributedSubstringForRange.

Good for testing with bug 5230 and bug 4681, perhaps others.

Sorry I can't share the version of Spell Catcher X that I'm working on that
beats up on all this stuff BIG time. It sends almost every TSM Doc Access
event, along with most other TSM events, and the problems are immediately
obvious with just a few minutes of playing with it. If you work at Apple and
*really* need to look at this more closely, we can probably work something out.

[Alexey Proskuryakov]

Created attachment 4411 [details]
Test app for comparing NSTextView and WebHTMLView

I'm working on a rather large rewrite of the method, to make it 100% in sync
with NSTextView.

Are there any known bugs in NSTextView's implementation of
firstRectForCharacterRange that WebKit shouldn't mimic?

[Alexey Proskuryakov]

Created attachment 4412 [details]
Test app for comparing NSTextView and WebHTMLView

I hate ZeroLink :-/

[Alexey Proskuryakov]

Created attachment 4424 [details]
proposed patch

This patch includes changes at multiple levels (WebHTMLView, WebCoreBridge and
khtml::TextIterator). Changes the behavior to closely match NSTextView,
including edge cases.

I couldn't reproduce the TCIM issue with either old or new implementation -
perhaps, it was caused by a bug laying somewhere deeper and that is already
fixed? In any case, NSTextView also doesn't use selection-relative indexing.

[Evan Gross]

(In reply to comment #14)
> Created an attachment (id=4424) [edit]

Not quite sure if it's something I'm doing wrong, but I can't get this patch to apply - I get "**** unexpected 
end of file in patch" when it tries to patch WebCoreBridge.mm and WebHTMLView.m. This is with today's 
unmodified versions of these files. Is it just me?

[Alexey Proskuryakov]

(In reply to comment #15)
It's possible that I got something wrong when editing the patch to exclude other changes... Unfortunately, 
there's no way to ensure that all our NSTextInput changes that are currently in the queues will apply 
without conflicts, and yet work on their own, too, so some merging pain is inevitable :(.

[Alexey Proskuryakov]

Created attachment 4835 [details]
proposed patch

Same patch updated to cleanly apply to current ToT

[Darin Adler]

Comment on attachment 4835 [details]
proposed patch

I think that either Justin Garcia or David Harrison should review this. It's
been around for a while, so we ought to review it soon.

[Alexey Proskuryakov]

Applying this patch corrects the results of fast/text/attributed-substring* tests.

[Justin Garcia]

I'm reviewing this.

[Justin Garcia]

Comment on attachment 4835 [details]
proposed patch

From -[WebHTMLView firstRectForCharacterRange]:

+    // Just to match NSTextView's behavior. Regression tests cannot detect
this;
+    // to reproduce, use a test application from
http://bugzilla.opendarwin.org/show_bug.cgi?id=4682
+    // (type something; set start=1, length=-1).
+    if ((theRange.location + theRange.length < theRange.location) &&
(theRange.location + theRange.length != 0))
+	 theRange.length = 0;

It sounds like you're saying that start=1 length=-1 is a case where this
special case code is needed.  But if that's true, why the && (theRange.location
+ theRange.length != 0)?  (1 + -1 == 0).

Also I think that:
theRange.length < 0
is clearer than:
theRange.location + theRange.length < theRange.location

Also I think that:
+    if (!range)
+	 return NSMakeRect(0,0,0,0);
Should probably be:
+    if (!range || ![range startContainer])
+	 return NSMakeRect(0,0,0,0);
You took out the check for ![range startContainer] and added !range, so perhaps
you have a good reason for not including ![range startContainer].

From [WebCoreBridge convertNSRangeToDOMRange:]:

+    if (nsrange.location > INT_MAX)
+	 return 0;
+    if (nsrange.length > INT_MAX || nsrange.location + nsrange.length >
INT_MAX)
+	 nsrange.length = INT_MAX - nsrange.location;
+

I think that this would be more appropriate done in -[WebCoreBridge
convertToDOMRange], since that is where the NSRange is given to a function that
expects signed ints.

r- for now.

[Alexey Proskuryakov]

Created attachment 5176 [details]
proposed patch

> It sounds like you're saying that start=1 length=-1 is a case where this
> special case code is needed.	But if that's true, why the &&
(theRange.location
> + theRange.length != 0)?  (1 + -1 == 0).

  The comment was somewhat misleading, I have reworded it a bit. Resetting the 

length to zero is not needed when theRange.location+theRange.length is zero.

> Also I think that:
> theRange.length < 0
> is clearer than:
> theRange.location + theRange.length < theRange.location

  theRange.length is unsigned, so it can never be less than zero. Another way
to
write this is "(int)theRange.length < 0", but I'm not sure if it's portable.

  I took out the check for [range startContainer] because
convertNSRangeToDOMRange
is now supposed to either return nil or a proper range. I've added a couple of 

assertions for this.

  Moved INT_MAX checks to -[WebCoreBridge convertToDOMRange].

[Justin Garcia]

Comment on attachment 5176 [details]
proposed patch

Great patch.  r=me

[Justin Garcia]

i landed this