Bug 50624 – [GTK] Don't use a fixed size for search field icons

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We should scale the icons depending on search field size.

[Carlos Garcia Campos]

Created attachment 79580 [details]
Scale icons when needed to make them fit in the search field

[Martin Robinson]

Comment on attachment 79580 [details]
Scale icons when needed to make them fit in the search field

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

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:-261
> -    // We also add one pixel here to ensure that the y coordinate is rounded up for box heights
> -    // that are even, which looks in relation to the box text.

I believe kov asked me to add this comment specifically so we probably shouldn't delete it. It explains the "+ 1" below. It should say "looks good" though I think.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:276
>      GRefPtr<GdkPixbuf> icon = getStockIcon(GTK_TYPE_ENTRY, GTK_STOCK_FIND,
>                                             gtkTextDirection(renderObject->style()->direction()),
>                                             gtkIconState(this, renderObject), GTK_ICON_SIZE_MENU);

Wouldn't it either make sense to either pass the actual size here or get the largest possible size and scale down?

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 79580 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79580&action=review
> 
> > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:-261
> > -    // We also add one pixel here to ensure that the y coordinate is rounded up for box heights
> > -    // that are even, which looks in relation to the box text.
> 
> I believe kov asked me to add this comment specifically so we probably shouldn't delete it. It explains the "+ 1" below. It should say "looks good" though I think.

This is a mistake I didn't want to remove that, sorry. 

> > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:276
> >      GRefPtr<GdkPixbuf> icon = getStockIcon(GTK_TYPE_ENTRY, GTK_STOCK_FIND,
> >                                             gtkTextDirection(renderObject->style()->direction()),
> >                                             gtkIconState(this, renderObject), GTK_ICON_SIZE_MENU);
> 
> Wouldn't it either make sense to either pass the actual size here 

Gtk expects a GtkIconSize which is actually an integer, even if we register new icon sizes, gtk_icon_set_render_icon_pixbuf() doesn't guarantee that the returned pixbuf will have the pixel size of the given icon size, if it finds a builtin pixbuf in the theme close to the given size, it will be returned. This is because GtkIconFactory uses gtk_icon_theme_choose_icon() with only the flag GTK_ICON_LOOKUP_USE_BUILTIN, it doesn't use GTK_ICON_LOOKUP_FORCE_SIZE. In this case we want to make sure the pixbuf has the pixel size we want, so we have to scale it. 

> or get the largest possible size and scale down?

The largest possible size is GTK_ICON_SIZE_MENU so we are indeed doing so. We define GTK_ICON_SIZE_MENU as the max size in adjustSearchField methods, in most of the cases this is what we want and the returned pixbuf, which is cached by gtk, is valid, and only in a few cases we need to sacle it down.

[Carlos Garcia Campos]

Created attachment 79710 [details]
Updated patch

I've moved the code to scale the icon to paintGdkPixbuf() that now receives the icon rect instead od the icon location.

[Martin Robinson]

Comment on attachment 79710 [details]
Updated patch

I think we have a slight issue here which is that we don't modify the adjustSearchField*ButtonStyle methods, so WebCore doesn't really know how large the icon is. We probably want to do something similar to the RenderThemeChromiumSkia which does this to determine the icon size:

    // Scale the button size based on the font size
    float fontScale = style->fontSize() / defaultControlFontPixelSize;
    int cancelButtonSize = lroundf(std::min(std::max(minCancelButtonSize, defaultCancelButtonSize * fontScale), maxCancelButtonSize));
    style->setWidth(Length(cancelButtonSize, Fixed));
    style->setHeight(Length(cancelButtonSize, Fixed));

and them simply centers the icon vertically as we do now. After thinking about this a big, I've decided its probably better to use the font size of the field, rather than the height to figure out the icon size.

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 79710 [details])
> I think we have a slight issue here which is that we don't modify the adjustSearchField*ButtonStyle methods, so WebCore doesn't really know how large the icon is. 

Yes, we modify the style in RenderThemeGtk::adjustSearchFieldCancelButtonStyle() and RenderThemeGtk::adjustSearchFieldResultsDecorationStyle()

> We probably want to do something similar to the RenderThemeChromiumSkia which does this to determine the icon size:
> 
>     // Scale the button size based on the font size
>     float fontScale = style->fontSize() / defaultControlFontPixelSize;
>     int cancelButtonSize = lroundf(std::min(std::max(minCancelButtonSize, defaultCancelButtonSize * fontScale), maxCancelButtonSize));
>     style->setWidth(Length(cancelButtonSize, Fixed));
>     style->setHeight(Length(cancelButtonSize, Fixed));

fontsize is always the same for us, so it's not that different to what we currently do settings a fixed size. 

> and them simply centers the icon vertically as we do now. After thinking about this a big, I've decided its probably better to use the font size of the field, rather than the height to figure out the icon size.

I tried it, but it didn't help because the font size was always 13.

[Martin Robinson]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 79710 [details] [details])
> > I think we have a slight issue here which is that we don't modify the adjustSearchField*ButtonStyle methods, so WebCore doesn't really know how large the icon is. 
> 
> Yes, we modify the style in RenderThemeGtk::adjustSearchFieldCancelButtonStyle() and RenderThemeGtk::adjustSearchFieldResultsDecorationStyle()

In adjustSearchFieldCancelButtonStyle we simply set the icon to the size of GTK_ICON_SIZE_MENU. 

> fontsize is always the same for us, so it's not that different to what we currently do settings a fixed size. 

This isn't actually true. :) Try this:

<input type="search" id="target" results="5" value="Search me" style="font-size:50px; ">

[Martin Robinson]

(In reply to comment #3)

> The largest possible size is GTK_ICON_SIZE_MENU so we are indeed doing so. We define GTK_ICON_SIZE_MENU as the max size in adjustSearchField methods, in most of the cases this is what we want and the returned pixbuf, which is cached by gtk, is valid, and only in a few cases we need to sacle it down.

Even so, wouldn't it be better to avoid any initial scaling at all by using (GtkIconSize)-1 explicitly?

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #3)
> 
> > The largest possible size is GTK_ICON_SIZE_MENU so we are indeed doing so. We define GTK_ICON_SIZE_MENU as the max size in adjustSearchField methods, in most of the cases this is what we want and the returned pixbuf, which is cached by gtk, is valid, and only in a few cases we need to sacle it down.
> 
> Even so, wouldn't it be better to avoid any initial scaling at all by using (GtkIconSize)-1 explicitly?

I think we want to have a max icon size. Using GTK_ICON_SIZE_MENU we make sure the icon won't be bigger than 16 and we use the icon cache for most of the cases.

[Carlos Garcia Campos]

Created attachment 79910 [details]
New patch

It uses the current font size to select the icon size now.

[Martin Robinson]

Comment on attachment 79910 [details]
New patch

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

Okay! This should land with new DRT dumps / pixel results (for GTK+ 2.x).

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:252
> +static GtkIconSize getIconSizeForPixelSize(gint pixelSize)

I think it's more appropriate to use int instead of gint.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:284
> +    gint fontSize = style->fontSize();

Ditto.

[Carlos Garcia Campos]

Created attachment 81223 [details]
Updated patch

[Martin Robinson]

Committed r77726: <http://trac.webkit.org/changeset/77726>