Bug 58102 - REGRESSION (WebKit2): AppKit thinks that web views don't support DocumentAccess
Summary: REGRESSION (WebKit2): AppKit thinks that web views don't support DocumentAccess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2011-04-07 17:41 PDT by Alexey Proskuryakov
Modified: 2011-04-08 12:39 PDT (History)
2 users (show)

See Also:


Attachments
proposed fix (8.37 KB, patch)
2011-04-07 17:45 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2011-04-07 17:41:31 PDT
This works in WebKit1 because it implements textStorage method, but a better way to convince AppKit would be to switch to NSTextInputClient protocol. In the future, this could let us do performance optimizations too.

Patch forthcoming.

<rdar://problem/9223246>
Comment 1 Alexey Proskuryakov 2011-04-07 17:45:02 PDT
Created attachment 88742 [details]
proposed fix

We could do this in WebHTMLView, but it has a huge baggage of hackish workarounds that changing the behavior by switching to a new protocol is super scary. The proposed alternative of having the view classes diverge is slightly less scary.
Comment 2 Enrica Casucci 2011-04-07 18:01:21 PDT
Comment on attachment 88742 [details]
proposed fix

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

The overall implementation looks good to me.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1061
>          // FIXME: We ignore most attributes from the string, so for example inserting from Character Palette loses font and glyph variation data.

Is this comment still applicable?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1324
>          // FIXME: We ignore most attributes from the string, so an input method cannot specify e.g. a font or a glyph variation.

Same as above. Is this comment still applicable?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1399
> +

unnecessary change
Comment 3 Alexey Proskuryakov 2011-04-07 19:41:21 PDT
> Is this comment still applicable?

Yes, we ignore most attributes.
Comment 4 Darin Adler 2011-04-07 23:42:24 PDT
Comment on attachment 88742 [details]
proposed fix

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

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1050
> +        LOG(TextInput, "insertText:\"%@\" replacementRange:(%u, %u)", isAttributedString ? [string string] : string, replacementRange.location, replacementRange.length);

%u is not the appropriate formatting specifier for NSUInteger; it’s right for unsigned and wrong for NSUInteger. The simplest workaround is to cast to unsigned.

>> Source/WebKit2/UIProcess/API/mac/WKView.mm:1061

> 
> Is this comment still applicable?

I believe it is.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1348
> -- (NSAttributedString *)attributedSubstringFromRange:(NSRange)nsRange
> +- (NSAttributedString *)attributedSubstringForProposedRange:(NSRange)nsRange actualRange:(NSRangePointer)actualRange

The name nsRange is not good for the first argument. Leaving it alone makes the patch smaller, but other than that I see no reason to leave it with this name.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1383
> +- (NSRect)firstRectForCharacterRange:(NSRange)theRange actualRange:(NSRangePointer)actualRange

The name theRange is not good for the first argument. Leaving it alone makes the patch smaller, but other than that I see no reason to leave it with this name.
Comment 5 Alexey Proskuryakov 2011-04-08 12:01:20 PDT
> %u is not the appropriate formatting specifier for NSUInteger; it’s right for unsigned and wrong for NSUInteger. The simplest workaround is to cast to unsigned.

This is what we do elsewhere in WebHTMLView and WKView, I would prefer to not introduce an inconsistency, and to not make global changes in this patch.

> The name nsRange is not good for the first argument. Leaving it alone makes the patch smaller, but other than that I see no reason to leave it with this name.
<...>
> The name theRange is not good for the first argument. Leaving it alone makes the patch smaller, but other than that I see no reason to leave it with this name.

I'm not sure what name would be better. AppKit uses "aRange" for these arguments, which is no more speaking than our names, and doesn't fit our common style.
Comment 6 Alexey Proskuryakov 2011-04-08 12:39:44 PDT
Committed <http://trac.webkit.org/changeset/83326>.