Bug 48234 – Size of text in popup menu doesn't match size of text in <select> itself in WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Adam Roben (:aroben)]

To reproduce:

1. Go to data:text/html,%3Cselect%20style=%22font-size:5px%22%3E%3Coption%3Ea%3Coption%3Eb
2. Click on the <select>

The menu that opens has text that is larger than the text in the <select> in the page. The font sizes are supposed to match.

[Adam Roben (:aroben)]

<rdar://problem/8589450>

[Sam Weinig]

Created attachment 83960 [details]
Patch

Comment on attachment 83960 [details]
Patch

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

> Source/WebKit2/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=48234

<rdar://problem/8589450>

> Source/WebKit2/ChangeLog:13
> +        Add class to encapsulate passing a font description over the wire.

Why isn’t it called FontDescription, then?

> Source/WebKit2/Shared/PlatformPopupMenuData.cpp:32
> +    

Whitespace

> Source/WebKit2/Shared/PlatformPopupMenuData.h:39
> +    

Here too

> Source/WebKit2/UIProcess/WebPageProxy.cpp:33
>  #include "DrawingAreaProxy.h"
> +#include "DictionaryPopupInfo.h"

Not in ASCII order.

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.h:53
> +    void populate(const Vector<WebPopupItem>&, NSFont*, WebCore::TextDirection);

Missing space after “NSFont”

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:51
> +void WebPopupMenuProxyMac::populate(const Vector<WebPopupItem>& items, NSFont* font, TextDirection menuTextDirection)

Ditto. Also extra space before “font”.

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:94
> +    NSFont *font = nil;

No need to initialize this to nil.

Comment on attachment 83960 [details]
Patch

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

r- since this breaks Qt and Windows. But please also consider my previous comments and the next one.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebPopupMenuMac.mm:53
> +    data.fontInfo.fontAttributeDictionary = fontDescriptorAttributes;
> +    data.fontInfo.fontOverrideSize = frame->pageScaleFactor() == 1 ? 0 : ([[font fontDescriptor] pointSize] * frame->pageScaleFactor());

The “override” concept seems unnecessarily cumbersome. Why not simply make a mutable copy of the dictionary and change the font size attribute? Alternatively, since the UI process knows the page scale factor, why not leave this multiplication exercise up to it

[Sam Weinig]

Created attachment 83980 [details]
Patch

[Sam Weinig]

Created attachment 83983 [details]
Patch

[Sam Weinig]

(In reply to comment #6)
> Created an attachment (id=83983) [details]
> Patch

This patch addresses many of the review comments but does not change FontInfo to be named FontDescription because that led to a name clash with WebCore that was confusing the message generator and does not remove the overrideSize concept, since that existed prior to this patch (for the dictionary lookup stuff) and I can change them both in a follow up patch.

[Sam Weinig]

Created attachment 84081 [details]
Patch

[Darin Adler]

Comment on attachment 84081 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +        * Shared/TextInfo.cpp: Removed.
> +        * Shared/TextInfo.h: Removed.
> +        Replace this with the more appropriately DictionaryPopupInfo.

I am not a big fan of the “info” suffix. Not sure what it means, since data is info and most classes have data.

> Source/WebKit2/Shared/DictionaryPopupInfo.h:43
> -class TextInfo {
> +class DictionaryPopupInfo {
>  public:
> -    TextInfo() { }
> +    DictionaryPopupInfo()
> +    {
> +    }

Normally we use struct when we have something with all public data members.

The explicit constructor isn’t needed. The compiler does this by default unless you tell it not to. It is valid to do this if you want to make the constructor private or protected, but that is not the case here.

> Source/WebKit2/Shared/FontInfo.cpp:39
> +#if PLATFORM(MAC)
> +#endif

What is this about? Please remove it.

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:97
> +        NSFontDescriptor *fontDescriptor = [NSFontDescriptor fontDescriptorWithFontAttributes:(NSDictionary *)data.fontInfo.fontAttributeDictionary.get()];
> +        font = [NSFont fontWithDescriptor:fontDescriptor size:((scaleFactor != 1) ? [fontDescriptor pointSize] * scaleFactor : 0)];

Can we share this code with PageClientImpl.mm?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:305
> +    FloatPoint origin(finalRangeRect.x(), finalRangeRect.y());

I don’t think we need a local variable for this. Can’t we just do this in the assignment statement?

    dictionaryPopupInfo.origin = finalRangeRect.location();

[Sam Weinig]

Landed in 79886.