Bug 48234 - Size of text in popup menu doesn't match size of text in <select> itself in WebKit2
Summary: Size of text in popup menu doesn't match size of text in <select> itself in W...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: data:text/html,%3Cselect%20style=%22f...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-10-25 06:35 PDT by Adam Roben (:aroben)
Modified: 2011-02-28 11:19 PST (History)
2 users (show)

See Also:


Attachments
Patch (50.96 KB, patch)
2011-02-26 19:36 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (52.57 KB, patch)
2011-02-27 10:42 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (52.80 KB, patch)
2011-02-27 11:45 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (54.12 KB, patch)
2011-02-28 10:38 PST, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2010-10-25 06:35:06 PDT
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.
Comment 1 Adam Roben (:aroben) 2010-10-25 06:35:42 PDT
<rdar://problem/8589450>
Comment 2 Sam Weinig 2011-02-26 19:36:53 PST
Created attachment 83960 [details]
Patch
Comment 3 mitz 2011-02-26 20:01:08 PST
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 4 mitz 2011-02-27 00:06:40 PST
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
Comment 5 Sam Weinig 2011-02-27 10:42:33 PST
Created attachment 83980 [details]
Patch
Comment 6 Sam Weinig 2011-02-27 11:45:59 PST
Created attachment 83983 [details]
Patch
Comment 7 Sam Weinig 2011-02-27 12:27:28 PST
(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.
Comment 8 Sam Weinig 2011-02-28 10:38:53 PST
Created attachment 84081 [details]
Patch
Comment 9 Darin Adler 2011-02-28 10:53:04 PST
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();
Comment 10 Sam Weinig 2011-02-28 11:19:14 PST
Landed in 79886.