WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED DUPLICATE of
bug 85494
91952
[EFL]Not to adjust height of buttons
https://bugs.webkit.org/show_bug.cgi?id=91952
Summary
[EFL]Not to adjust height of buttons
KyungTae Kim
Reported
2012-07-22 17:24:06 PDT
../patch-[20120723]-[
ktf.kim@samsung.com
]-[[EFL]Not-to-adjust-height-of-Button].patch
Attachments
Patch
(6.84 KB, patch)
2012-07-22 17:25 PDT
,
KyungTae Kim
no flags
Details
Formatted Diff
Diff
Patch
(8.15 KB, patch)
2012-07-22 17:47 PDT
,
KyungTae Kim
no flags
Details
Formatted Diff
Diff
Patch
(8.11 KB, patch)
2012-07-22 18:14 PDT
,
KyungTae Kim
rakuco
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
KyungTae Kim
Comment 1
2012-07-22 17:25:16 PDT
Created
attachment 153711
[details]
Patch
WebKit Review Bot
Comment 2
2012-07-22 17:27:39 PDT
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
Comment 3
2012-07-22 17:30:43 PDT
kyungtae, bug title and changelog are wrong.
KyungTae Kim
Comment 4
2012-07-22 17:47:00 PDT
Created
attachment 153712
[details]
Patch
KyungTae Kim
Comment 5
2012-07-22 18:14:45 PDT
Created
attachment 153715
[details]
Patch
Raphael Kubo da Costa (:rakuco)
Comment 6
2012-07-22 20:15:47 PDT
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
Comment 7
2012-07-31 02:22:26 PDT
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)
Comment 8
2012-07-31 04:34:08 PDT
Adding under meta
bug 85877
that's used to track this kind of problem.
KyungTae Kim
Comment 9
2012-08-22 22:37:25 PDT
*** This bug has been marked as a duplicate of
bug 85494
***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug