Bug 103496 – [mac] Dictionary lookup bubble loses intrarange formatting

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Tim Horton]

Currently, the UIProcess is handed a String and a font descriptor to create an attributed string to pass to the dictionary popup. Consequently, it loses all intrarange formatting. Instead, we should pass an attributed string with all of the formatting back to the UIProcess to hand over.

I have a patch.

<rdar://problem/12762172>

[Tim Horton]

Created attachment 176433 [details]
patch

[Tim Horton]

Created attachment 176435 [details]
patch (+remove pageScaleFactor argument from PageClient::didPerformDictionaryLookup)

[Alexey Proskuryakov]

Comment on attachment 176435 [details]
patch (+remove pageScaleFactor argument from PageClient::didPerformDictionaryLookup)

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

Looks great!

> Source/WebKit2/ChangeLog:11
> +        when showing dictionary popups, so that we preserve all formatting in the yellow dictionary
> +        highlight. Also, remove the fontInfo member from DictionaryPopupInfo, since we don't need it anymore.

"All" is a strong word - there are things that cannot be preserved, such as web fonts that are only enabled in WebProcess.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:-557
> -    // We won't be able to get an NSFont in the case that a Web Font is being used, so use
> -    // the default system font at the same size instead.

How did you test that nothing bad happens because of losing this logic here? I assume WebHTMLConverter does the same thing, but explicit testing would be useful anyway.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:568
> +    RetainPtr<NSAttributedString> nsAttributedString = [WebHTMLConverter editingAttributedStringFromRange:range];

Why retain it? I think that this should be a plain pointer.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:580
> +            font = [fontManager convertFont:font toSize:[font pointSize] * pageScaleFactor()];
> +            [scaledAttributes setObject:font forKey:NSFontAttributeName];

Does this work correctly with all scaling/zooming styles (Cmd+'+' as well as pinch to zoom, and Retina displays with a default scale factor)?

[Tim Horton]

(In reply to comment #3)
> (From update of attachment 176435 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176435&action=review
> 
> Looks great!
> 
> > Source/WebKit2/ChangeLog:11
> > +        when showing dictionary popups, so that we preserve all formatting in the yellow dictionary
> > +        highlight. Also, remove the fontInfo member from DictionaryPopupInfo, since we don't need it anymore.
> 
> "All" is a strong word - there are things that cannot be preserved, such as web fonts that are only enabled in WebProcess.

Sure.

> > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:-557
> > -    // We won't be able to get an NSFont in the case that a Web Font is being used, so use
> > -    // the default system font at the same size instead.
> 
> How did you test that nothing bad happens because of losing this logic here? I assume WebHTMLConverter does the same thing, but explicit testing would be useful anyway.

Hmm, actually, apparently it doesn't. It falls back to the system font, but not at the right size... is it reasonable to fix that in WebHTMLConverter?

> > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:568
> > +    RetainPtr<NSAttributedString> nsAttributedString = [WebHTMLConverter editingAttributedStringFromRange:range];
> 
> Why retain it? I think that this should be a plain pointer.

Sure.

> > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:580
> > +            font = [fontManager convertFont:font toSize:[font pointSize] * pageScaleFactor()];
> > +            [scaledAttributes setObject:font forKey:NSFontAttributeName];
> 
> Does this work correctly with all scaling/zooming styles (Cmd+'+' as well as pinch to zoom, and Retina displays with a default scale factor)?

Pinch-to-zoom: yes.
Cmd-+ "full page": yes.
Cmd-+ "text only": yes.
Non-1 deviceScaleFactor (Retina): yes.

[Alexey Proskuryakov]

> Hmm, actually, apparently it doesn't. It falls back to the system font, but not at the right size... is it reasonable to fix that in WebHTMLConverter?

I'm not sure that this should be part of this patch. Perhaps it's best to reinstate this logic here, and add a FIXME about pushing it down.

We'd need to test what happens with web fonts and HTML editing today.

[Tim Horton]

(In reply to comment #5)
> > Hmm, actually, apparently it doesn't. It falls back to the system font, but not at the right size... is it reasonable to fix that in WebHTMLConverter?
> 
> I'm not sure that this should be part of this patch. Perhaps it's best to reinstate this logic here, and add a FIXME about pushing it down.

I'm not sure how to reinstate that logic here, because - for example - what happens if you switch to a web font in the middle of your range?

> We'd need to test what happens with web fonts and HTML editing today.

I'll file another bug for that. It's very easy to fix, and Dan thinks it's safe. I'm having some trouble with TestWebKitAPI, but I'll make a test too.

[Tim Horton]

Thanks, Alexey!

http://trac.webkit.org/changeset/136075