Summary: | -[WebHTMLView firstRectForCharacterRange:] is using _selectedRange instead of the given range if no marked text | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Evan Gross <evan> | ||||||||||||||||||
Component: | HTML Editing | Assignee: | Justin Garcia <justin.garcia> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | ap, justin.garcia | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 420+ | ||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||||||
Attachments: |
|
Description
Evan Gross
2005-08-26 23:30:30 PDT
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? (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. Yes, apparently, Dictionary indeed tries and gives up on kEventTextInputOffsetToPos, just as you describe. Created attachment 3872 [details]
proposed patch
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."
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? 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.
(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). 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? (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... 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. 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?
Created attachment 4412 [details]
Test app for comparing NSTextView and WebHTMLView
I hate ZeroLink :-/
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.
(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? (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 :(. Created attachment 4835 [details]
proposed patch
Same patch updated to cleanly apply to current ToT
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.
Applying this patch corrects the results of fast/text/attributed-substring* tests. I'm reviewing this. 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. 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]. Comment on attachment 5176 [details]
proposed patch
Great patch. r=me
i landed this |