Bug 79435 – incorrect line-height for styled menulist (select tag) in windows theme (wincairo port)

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Lynn Neir]

According to Layout test: fast/forms/menulist-restrict-line-height.html, the line-height should not be honored for styled popup buttons.  Currently, on wincairo port, this test clips the styled popup button.

Looking at the Safari theme in method: RenderThemeSafari::adjustMenuListButtonStyle, they have extra line: style->setLineHeight(RenderStyle::initialLineHeight());.  This code is not present in windows theme code.  So, I add this line and the resulting code is shown below.  I ran tests, it fixes the test noted above and doesn't break any other tests.

void RenderThemeWin::adjustMenuListButtonStyle(CSSStyleSelector* selector, RenderStyle* style, Element* e) const
{
    // These are the paddings needed to place the text correctly in the <select> box
    const int dropDownBoxPaddingTop    = 2;
    const int dropDownBoxPaddingRight  = style->direction() == LTR ? 4 + dropDownButtonWidth : 4;
    const int dropDownBoxPaddingBottom = 2;
    const int dropDownBoxPaddingLeft   = style->direction() == LTR ? 4 : 4 + dropDownButtonWidth;
    // The <select> box must be at least 12px high for the button to render nicely on Windows
    const int dropDownBoxMinHeight = 12;
    
    // Position the text correctly within the select box and make the box wide enough to fit the dropdown button
    style->setPaddingTop(Length(dropDownBoxPaddingTop, Fixed));
    style->setPaddingRight(Length(dropDownBoxPaddingRight, Fixed));
    style->setPaddingBottom(Length(dropDownBoxPaddingBottom, Fixed));
    style->setPaddingLeft(Length(dropDownBoxPaddingLeft, Fixed));

    // Height is locked to auto
    style->setHeight(Length(Auto));

    // Calculate our min-height
    int minHeight = style->fontMetrics().height();
    minHeight = max(minHeight, dropDownBoxMinHeight);

    style->setMinHeight(Length(minHeight, Fixed));
    
    style->setLineHeight(RenderStyle::initialLineHeight());

    // White-space is locked to pre
    style->setWhiteSpace(PRE);
}

[Benjamin Poulain]

You should attach the patch to the bug ("Add an attachement").

Here are the basic steps:
-make the patch
-update the test results with your patch
-./Tools/Scripts/prepare-changelog --bug 79435
--> This update the relevant ChangeLog files. Open them and explain what the patch does
-attach the full diff here
-set the r (review) and cq (commit queue) flags to "?"

[Benjamin Poulain]

Note that we only rigorously track the bugs with a patch in "r?". So when you create a bug report like here, it can be ignored for a long time.

[Lynn Neir]

Created attachment 132307 [details]
patch to fix line-height issue

This patch fixes line-height issue in three tests:
- fast/forms/menulist-restrict-line-height.html
- fast/forms/control-restrict-line-height.html
- fast/forms/basic-selects.html

I didn't include changes to wincairo platform Skipped file as it seems test scripts are not setup to use the skipped file.  I verified results by running winlauncher and visually comparing results to Safari/Chrome.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:13:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 7 in 2 files


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

[Lynn Neir]

(In reply to comment #4)
> Attachment 132307 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebCore/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
> Source/WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
> Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
> Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
> Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
> Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
> Source/WebCore/ChangeLog:13:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]

style errors are false positives, style check ran fine for me.

as explained earlier:
This patch fixes line-height issue in three tests:
- fast/forms/menulist-restrict-line-height.html
- fast/forms/control-restrict-line-height.html
- fast/forms/basic-selects.html

I didn't include changes to wincairo platform Skipped file as it seems test scripts are not setup to use the skipped file.  I verified results by running winlauncher and visually comparing results to Safari/Chrome.

> Total errors found: 7 in 2 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 132307 [details]
patch to fix line-height issue

> > Source/WebCore/ChangeLog:13:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
> 
> style errors are false positives, style check ran fine for me.

Obviously not.
Check the file by yourself: https://bug-79435-attachments.webkit.org/attachment.cgi?id=132307

[Lynn Neir]

(In reply to comment #6)
> (From update of attachment 132307 [details])
> > > Source/WebCore/ChangeLog:13:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
> > 
> > style errors are false positives, style check ran fine for me.
> 
> Obviously not.
> Check the file by yourself: https://bug-79435-attachments.webkit.org/attachment.cgi?id=132307

Yes, sorry I didn't realize that style checks were also performed on ChangeLog files.  I will resubmit here in a bit.

[Lynn Neir]

Created attachment 132366 [details]
new patch fixing style issue, update to ChangeLog

[Gyuyoung Kim]

Comment on attachment 132366 [details]
new patch fixing style issue, update to ChangeLog

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

> Source/WebCore/ChangeLog:4
> +        incorrect line-height for styled menulist (select tag) in windows theme (wincairo port)

Generally, bug url is placed below bug title.

> Source/WebCore/ChangeLog:6
> +        applied same fix as in RenderThemeSafari::adjustMenuListButtonStyle to Windows theme to fix issue in three tests: 

Test cases are placed below patch description.

[Brent Fulgham]

This poor one-liner has been languishing for too long!

Lynn, if you don't have time to clean up the patch I'll try to move it along tomorrow.

[Brent Fulgham]

Comment on attachment 132366 [details]
new patch fixing style issue, update to ChangeLog

I will clean up the ChangeLog and land the patch.

[Brent Fulgham]

Committed r134657: <http://trac.webkit.org/changeset/134657>