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); }
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 "?"
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.
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.
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.
(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.
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
(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.
Created attachment 132366 [details] new patch fixing style issue, update to ChangeLog
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.
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.
Comment on attachment 132366 [details] new patch fixing style issue, update to ChangeLog I will clean up the ChangeLog and land the patch.
Committed r134657: <http://trac.webkit.org/changeset/134657>