Bug 185854 - [GTK] Buttons are drawn too large, text not centered
Summary: [GTK] Buttons are drawn too large, text not centered
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-21 18:27 PDT by Michael Catanzaro
Modified: 2018-08-06 01:27 PDT (History)
7 users (show)

See Also:


Attachments
bugs.webkit.org screenshot (95.82 KB, image/png)
2018-05-21 18:27 PDT, Michael Catanzaro
no flags Details
launchpad.net screenshot (181.82 KB, image/png)
2018-05-21 18:28 PDT, Michael Catanzaro
no flags Details
Patch (4.30 KB, patch)
2018-07-31 05:52 PDT, Carlos Bentzen
no flags Details | Formatted Diff | Diff
launchpad.net after patch (291.14 KB, image/png)
2018-07-31 05:58 PDT, Carlos Bentzen
no flags Details
bugs.webkit.org after patch (196.32 KB, image/png)
2018-07-31 06:06 PDT, Carlos Bentzen
no flags Details
Before and after in bugzilla button (3.12 KB, image/png)
2018-07-31 06:16 PDT, Carlos Bentzen
no flags Details
Archive of layout-test-results from ews206 for win-future (12.99 MB, application/zip)
2018-07-31 12:12 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2018-05-21 18:27:45 PDT
Created attachment 340936 [details]
bugs.webkit.org screenshot

Look at these attached screenshots from bugs.webkit.org and launchpad.net. Then buttons are drawn too large. There's too much space on the buttom: the text is not vertically centered.

On the bugs.webkit.org screenshot, the Search button at the top demonstrates the problem: the button is supposed to have the same vertical height as the search entry, and it's placed correctly at the top, but it uses too much vertical space on the bottom. I think the Quick Search button in the middle has the same problem, but it's much more subtle and less obvious here. It's most obvious with the Search button on the bottom, notice how it's not aligned at all with the dotted lines.

On launchpad.net, the problem is pretty obvious with the "Log Out" and "Search Launchpad" buttons.
Comment 1 Michael Catanzaro 2018-05-21 18:28:25 PDT
Created attachment 340937 [details]
launchpad.net screenshot
Comment 2 Carlos Bentzen 2018-07-31 05:52:49 PDT
Created attachment 346157 [details]
Patch
Comment 3 Carlos Bentzen 2018-07-31 05:56:05 PDT
The buttons were only being drawn down to the minimum size:

bool RenderThemeGadget::render(cairo_t* cr, const FloatRect& paintRect, FloatRect* contentsRect)
{
    FloatRect rect = paintRect;

    auto margin = marginBox();
    rect.move(margin.left, margin.top);
    rect.contract(margin.left + margin.right, margin.top + margin.bottom);

    auto minSize = minimumSize();
    rect.setWidth(std::max<float>(rect.width(), minSize.width()));
    rect.setHeight(std::max<float>(rect.height(), minSize.height()));

    ...
}

So I created another Gadget children class RenderThemeButtonGadget, which overrides minimumSize(). 

Note: this is exactly like RenderThemeTextFieldGadget, but I created another class to be explicit and if further customization on buttons are needed in the future.
Comment 4 Carlos Bentzen 2018-07-31 05:58:30 PDT
Created attachment 346158 [details]
launchpad.net after patch
Comment 5 Carlos Bentzen 2018-07-31 06:06:15 PDT
Created attachment 346159 [details]
bugs.webkit.org after patch

In bugs.webkit.org it is more subtle to notice the patch, because it still is not completely aligned with the text entries.

One way to do so is use the inspector to see the margins drawn before and after. The background was being drawn outside the borders and now it is not.
Comment 6 Carlos Bentzen 2018-07-31 06:16:11 PDT
Created attachment 346160 [details]
Before and after in bugzilla button
Comment 7 EWS Watchlist 2018-07-31 12:12:05 PDT
Comment on attachment 346157 [details]
Patch

Attachment 346157 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8712147

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html
imported/blink/transitions/unprefixed-transform.html
legacy-animation-engine/imported/blink/transitions/unprefixed-transform.html
Comment 8 EWS Watchlist 2018-07-31 12:12:18 PDT
Created attachment 346187 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 9 Michael Catanzaro 2018-08-04 15:08:17 PDT
Looks good to me. I think Carlos Garcia will want to review it.
Comment 10 WebKit Commit Bot 2018-08-06 01:25:43 PDT
Comment on attachment 346157 [details]
Patch

Clearing flags on attachment: 346157

Committed r234592: <https://trac.webkit.org/changeset/234592>
Comment 11 WebKit Commit Bot 2018-08-06 01:25:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-08-06 01:27:06 PDT
<rdar://problem/42961028>