[EFL] Wrong button height on CSS tests

[Thiago Marcos P. Santos]

fast/css/button-height.html with wrong results.

[KyungTae Kim]

Created attachment 160087 [details]
Patch

[KyungTae Kim]

*** Bug 91952 has been marked as a duplicate of this bug. ***

[Gyuyoung Kim]

Comment on attachment 160087 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        The code for overriding styles originated from the GTK port's, but that was removed.

I don't understand this comment well. I understand EFL port's problematic code came from GTK port, but GTK port removed it. right? If so, you need to explain it more clearly.

> Source/WebCore/ChangeLog:9
> +        (commit: 808a8f2a, bug: https://bugs.webkit.org/show_bug.cgi?id=33936)  

I think revision number is more useful than commit#. For example,r51455 instead of ae573116.

> Source/WebCore/ChangeLog:13
> +        (commit for chromium port: ae573116, commit for layouttest: 14f37205)  

ditto.

[KyungTae Kim]

What I mean was only the EFL port overrides the Border,WhiteSpace,Height style now. The overriding codes seems coming from GTK port's old codes that are no longer exists. 

I'll explain why the overriding code must be removed, line by line :

  if (style->appearance() == PushButtonPart) {  // Setting style only for <input type=button>, not <button>

    style->resetBorder();  // This code cause reset border style for button - 2px outset - to initial value. This code makes the size of <input type=button> different from the size of <button>, it makes 'fast/css/button-height.html' fail that check whether they are same.

    style->setWhiteSpace(PRE);  // This code is not needed because the white space for buttons already set to 'PRE'

    style->setHeight(Length(Auto)); // This code override the style for height, it makes 'fast/css/button-height.html' fail that set the button's height style.

[KyungTae Kim]

Created attachment 160620 [details]
Patch

[KyungTae Kim]

Created attachment 160622 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 160622 [details]
Patch

I think adjustButtonStyle() function needs to be implemented according to port character. We don't need to reset some stuffs for "PushButtonPart" because Button size is already set by adjustSizeConstraints(). You're missing to unskip this test in TestExpectations. Please include TestExpectations file.

By the way, don't you need to set initial value for height ?

    if (style->appearance() == PushButtonPart) {
        // Ignore line-height.
        style->setLineHeight(RenderStyle::initialLineHeight());

[KyungTae Kim]

What you said is related with https://bugs.webkit.org/show_bug.cgi?id=33936.

The code that initialize 'line-height' on the GTK and chromium ports is  to fix the bug that bottom portion of text hangs off edge of SELECT or BUTTON element
(layout test: fast/forms/control-restrict-line-height.html.) 

On the EFL port, because of "adjustSizeConstraints", the bug not occurs.

Then, I think not to initialize 'line-height' is better to prevent bugs related with 'line-height' style on button.

[KyungTae Kim]

Created attachment 160634 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 160634 [details]
Patch

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

> LayoutTests/ChangeLog:62
> +        * platform/efl/fast/replaced/width100percent-button-expected.txt:

In changelog, LayoutTests/platform/efl/TestExpectations modification is missing.

[KyungTae Kim]

Created attachment 160655 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160655 [details]
Patch

Clearing flags on attachment: 160655

Committed r126750: <http://trac.webkit.org/changeset/126750>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.