Bug 91952 – [EFL]Not to adjust height of buttons

• Component: Forms
• Status: RESOLVED DUPLICATE

[KyungTae Kim]

../patch-[20120723]-[[e-mail]]-[[EFL]Not-to-adjust-height-of-Button].patch

[KyungTae Kim]

Created attachment 153711 [details]
Patch

[WebKit Review Bot]

Attachment 153711 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/forms/button-width-height..." exit_code: 1
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryuan Choi]

kyungtae, bug title and changelog are wrong.

[KyungTae Kim]

Created attachment 153712 [details]
Patch

[KyungTae Kim]

Created attachment 153715 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 153715 [details]
Patch

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

Thanks for the patch. I recommend doing some research before working on a patch like this -- checking other ports' implementations of RenderTheme::adjustButtonStyle(), I came across GTK+'s, added in r69436. That revision, on its turn, points to an existing layout test that has a similar purpose to the one you have added, and which we are already skipping because it currently fails. And checking LayoutTests/platform/efl/TestExpectations, it is possible to see that the issue is being tracked in bug 85494.

I thus suggest closing this bug as a duplicate of bug 85494, checking if the existing test is enough to cover this change and sending a patch there.

> Source/WebCore/ChangeLog:3
> +        [EFL]Not to adjust height of buttons

Nit: we normally add a space between '[Category]' and the rest of the description. Plus "Not to adjust" doesn't look right, "Do not adjust" sounds better.

> Source/WebCore/ChangeLog:8
> +        The style for a width and a height should be applyed also for a button element.

s/applyed/applied/.

> Source/WebCore/ChangeLog:10
> +        The code should be removed because on other ports, the force height setting code for Buttons not exists.

Nitpicking a little on this reasoning, just because other ports don't do that isn't a good explanation; you should rather explain why it does not make sense to use Auto here.

[KyungTae Kim]

Thank you for the comment.
I agree with you. The new layout test is not needed because fast/css/button-height.html already exists.
But that test case is still failed after this patch because the difference between <button> and <input type='button'>.
The border is different and I think I need to analyze it and modify it, too.

RenderButton {BUTTON} size 65x40 [bgcolor=#C0C0C0] [border: (2px outset #C0C0C0)]
RenderButton {INPUT} size 61x36 [bgcolor=#C0C0C0]

[Dominik Röttsches (drott)]

Adding under meta bug 85877 that's used to track this kind of problem.

[KyungTae Kim]

*** This bug has been marked as a duplicate of bug 85494 ***