Bug 144234 – REGRESSION (r176751): line-height ignored in <button> elements

• Component: Forms
• Status: RESOLVED FIXED

[Darin Adler]

REGRESSION (r176751): line-height ignored in <button> elements

[Darin Adler]

Created attachment 251705 [details]
Patch

[Darin Adler]

<rdar://problem/19297258>

[Antti Koivisto]

Comment on attachment 251705 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> [phone]  Darin Adler  <[e-mail]>
> +
> +        REGRESSION (r176751): line-height ignored in <button> elements
> +        https://bugs.webkit.org/show_bug.cgi?id=144234
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/forms/button-line-height.html
> +

It would also be good to explain what the problem was.

[Antti Koivisto]

Comment on attachment 251705 [details]
Patch

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

> LayoutTests/fast/forms/button-line-height.html:2
> +<button style="height:24px; margin:0">Button with height</button>

Does this line test anything? The ref is identical.

> LayoutTests/fast/forms/button-line-height.html:4
> +<button style="height:124px; margin:0">Button with height</button>

This too.

[Darin Adler]

(In reply to comment #4)
> Comment on attachment 251705 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251705&action=review
> 
> > LayoutTests/fast/forms/button-line-height.html:2
> > +<button style="height:24px; margin:0">Button with height</button>
> 
> Does this line test anything? The ref is identical.

It’s just there so when you look at the test buttons in the browser you can compare one with the other right next to it. It’s not really part of the reftest, but I like having it there. Probably should have more things in the test explaining what is there and why. But I’ll just leave it as is for now.

> > LayoutTests/fast/forms/button-line-height.html:4
> > +<button style="height:124px; margin:0">Button with height</button>
> 
> This too.

Ditto.

[Darin Adler]

http://trac.webkit.org/changeset/183366 and http://trac.webkit.org/changeset/183367

[Brent Fulgham]

This reference test fails on all of the non-Mac/non-iOS ports. I don't understand why there would be a difference in behavior here, unless Windows (and GTK/EFL) have different default button heights than Mac.

[Darin Adler]

Oops. Maybe just a difference in the exact number of pixels added by padding. I’ll be happy to improve the tests at some point. Feel free to skip them for now.

[Alexey Proskuryakov]

The test is still broken, who can take care of it?

https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fforms%2Fbutton-line-height.html

[Darin Adler]

I’d be happy to do something about the test, but I don’t understand how to read the flakiness dashboard output at that URL. Does it mean the test is failing on all platforms? Or that it’s “flaky” on Mac and failing on others?

I am happy to replace the test with a different one, but I don’t understand the current status well enough to know what problem I am tackling.

The test relies on a specific relationship between the inner and outer dimensions of buttons that I now believe is platform specific. But there may be other problems with the test, too, and I would like to know that before I try to fix it.

[Alexey Proskuryakov]

Nikita skipped the test in bug 183862.

Before the skipping, the flakiness dashboard showed blue for every revision on the below bots, meaning that they failed pixel tests every time (there is a legend for the colors):

- Apple Win 7 Debug (Tests)
- Apple Win 7 Release (Tests)
- EFL Linux 64-bit Release WK2
- GTK Linux 64-bit Debug (Tests)
- GTK Linux 64-bit Release (Tests)

This is all the test queues except for Mac ones.

If you click on one of the blue rectangles, there is a "build log" link, pointing to a buildbot results page. For example, this is how this test fails on Windows:

https://build.webkit.org/results/Apple%20Win%207%20Debug%20(Tests)/r183842%20(66058)/fast/forms/button-line-height-diffs.html