79435 2012-02-23 20:34:47 -0800 incorrect line-height for styled menulist (select tag) in windows theme (wincairo port) 2012-11-14 13:27:11 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) PC Windows 7 RESOLVED FIXED P2 Normal --- 0 lynn.neir bfulgham benjamin bfulgham lynn.neir webkit.review.bot oldest_to_newest 563881 0 lynn.neir 2012-02-23 20:34:47 -0800 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); } 563962 1 benjamin 2012-02-23 23:02:29 -0800 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 "?" 563967 2 benjamin 2012-02-23 23:04:40 -0800 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. 580618 3 132307 lynn.neir 2012-03-16 10:07:34 -0700 Created attachment 132307 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. 580869 4 webkit.review.bot 2012-03-16 13:17:16 -0700 Attachment 132307 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. 580873 5 lynn.neir 2012-03-16 13:22:22 -0700 (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. 580881 6 132307 benjamin 2012-03-16 13:29:48 -0700 Comment on attachment 132307 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 580893 7 lynn.neir 2012-03-16 13:36:20 -0700 (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. 580901 8 132366 lynn.neir 2012-03-16 13:44:05 -0700 Created attachment 132366 new patch fixing style issue, update to ChangeLog 643219 9 132366 gyuyoung.kim 2012-06-06 22:28:15 -0700 Comment on attachment 132366 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. 766500 10 bfulgham 2012-11-13 23:34:59 -0800 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. 767169 11 132366 bfulgham 2012-11-14 12:21:11 -0800 Comment on attachment 132366 new patch fixing style issue, update to ChangeLog I will clean up the ChangeLog and land the patch. 767236 12 bfulgham 2012-11-14 13:27:11 -0800 Committed r134657: <http://trac.webkit.org/changeset/134657> 132307 2012-03-16 10:07:34 -0700 2012-03-16 13:44:05 -0700 patch to fix line-height issue line-height-patch.patch text/plain 1537 lynn.neir SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExMTAxOCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBACisyMDEyLTAzLTE2ICBMeW5uIE5l aXIgIDxseW5uLm5laXJAc2t5cGUubmV0PgorCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD03OTQzNQorCQlpbmNvcnJlY3QgbGluZS1oZWlnaHQgZm9yIHN0 eWxlZCBtZW51bGlzdCAoc2VsZWN0IHRhZykgaW4gd2luZG93cyB0aGVtZSAod2luY2Fpcm8gcG9y dCkKKwkJCisJCWFwcGxpZWQgc2FtZSBmaXggYXMgaW4gUmVuZGVyVGhlbWVTYWZhcmk6OmFkanVz dE1lbnVMaXN0QnV0dG9uU3R5bGUgdG8gV2luZG93cyB0aGVtZSB0byBmaXggaXNzdWUgaW4gdGhy ZWUgdGVzdHM6IAorCQlmYXN0L2Zvcm1zL21lbnVsaXN0LXJlc3RyaWN0LWxpbmUtaGVpZ2h0Lmh0 bWwKKwkJZmFzdC9mb3Jtcy9jb250cm9sLXJlc3RyaWN0LWxpbmUtaGVpZ2h0Lmh0bWwKKwkJZmFz dC9mb3Jtcy9iYXNpYy1zZWxlY3RzLmh0bWwKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdHMuIChPT1BTISkKKworICAgICAgICAqIHJl bmRlcmluZy9SZW5kZXJUaGVtZVdpbi5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJUaGVt ZVdpbjo6YWRqdXN0TWVudUxpc3RCdXR0b25TdHlsZSk6CisKIDIwMTItMDMtMTYgIFBhdmVsIFBv ZGl2aWxvdiAgPHBvZGl2aWxvdkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgV2ViIEluc3BlY3Rv cjogZml4IGV4Y2VwdGlvbiB3aGVuIGhvdmVyaW5nIG92ZXIgYm91bmQgZnVuY3Rpb24gaW4gaGVh cCBwcm9maWxlci4KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUaGVtZVdp bi5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclRoZW1l V2luLmNwcAkocmV2aXNpb24gMTEwOTQ0KQorKysgU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1Jl bmRlclRoZW1lV2luLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNzg0LDYgKzc4NCw4IEBAIHZvaWQg UmVuZGVyVGhlbWVXaW46OmFkanVzdE1lbnVMaXN0QnV0dG8KICAgICBtaW5IZWlnaHQgPSBtYXgo bWluSGVpZ2h0LCBkcm9wRG93bkJveE1pbkhlaWdodCk7CiAKICAgICBzdHlsZS0+c2V0TWluSGVp Z2h0KExlbmd0aChtaW5IZWlnaHQsIEZpeGVkKSk7CisKKyAgICBzdHlsZS0+c2V0TGluZUhlaWdo dChSZW5kZXJTdHlsZTo6aW5pdGlhbExpbmVIZWlnaHQoKSk7CiAgICAgCiAgICAgLy8gV2hpdGUt c3BhY2UgaXMgbG9ja2VkIHRvIHByZQogICAgIHN0eWxlLT5zZXRXaGl0ZVNwYWNlKFBSRSk7Cg== 132366 2012-03-16 13:44:05 -0700 2012-11-14 12:21:11 -0800 new patch fixing style issue, update to ChangeLog line-height-patch2.patch text/plain 1758 lynn.neir SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExMTAxOCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBACisyMDEyLTAzLTE2ICBMeW5uIE5l aXIgIDxseW5uLm5laXJAc2t5cGUubmV0PgorCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD03OTQzNQorICAgICAgICBpbmNvcnJlY3QgbGluZS1oZWlnaHQg Zm9yIHN0eWxlZCBtZW51bGlzdCAoc2VsZWN0IHRhZykgaW4gd2luZG93cyB0aGVtZSAod2luY2Fp cm8gcG9ydCkKKworICAgICAgICBhcHBsaWVkIHNhbWUgZml4IGFzIGluIFJlbmRlclRoZW1lU2Fm YXJpOjphZGp1c3RNZW51TGlzdEJ1dHRvblN0eWxlIHRvIFdpbmRvd3MgdGhlbWUgdG8gZml4IGlz c3VlIGluIHRocmVlIHRlc3RzOiAKKyAgICAgICAgZmFzdC9mb3Jtcy9tZW51bGlzdC1yZXN0cmlj dC1saW5lLWhlaWdodC5odG1sCisgICAgICAgIGZhc3QvZm9ybXMvY29udHJvbC1yZXN0cmljdC1s aW5lLWhlaWdodC5odG1sCisgICAgICAgIGZhc3QvZm9ybXMvYmFzaWMtc2VsZWN0cy5odG1sCisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSSBkaWRuJ3Qg aW5jbHVkZSBjaGFuZ2VzIHRvIHdpbmNhaXJvIHBsYXRmb3JtIFNraXBwZWQgZmlsZSBhcyBpdCBz ZWVtcyB0ZXN0IHNjcmlwdHMgYXJlIG5vdCBzZXR1cCB0byB1c2UgdGhlIHNraXBwZWQgZmlsZS4g IEkgdmVyaWZpZWQgcmVzdWx0cyBieSBydW5uaW5nIHdpbmxhdW5jaGVyIGFuZCB2aXN1YWxseSBj b21wYXJpbmcgcmVzdWx0cyB0byBTYWZhcmkvQ2hyb21lLgorCisgICAgICAgICogcmVuZGVyaW5n L1JlbmRlclRoZW1lV2luLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlclRoZW1lV2luOjph ZGp1c3RNZW51TGlzdEJ1dHRvblN0eWxlKToKKwogMjAxMi0wMy0xNiAgUGF2ZWwgUG9kaXZpbG92 ICA8cG9kaXZpbG92QGNocm9taXVtLm9yZz4KIAogICAgICAgICBXZWIgSW5zcGVjdG9yOiBmaXgg ZXhjZXB0aW9uIHdoZW4gaG92ZXJpbmcgb3ZlciBib3VuZCBmdW5jdGlvbiBpbiBoZWFwIHByb2Zp bGVyLgpJbmRleDogU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclRoZW1lV2luLmNwcAo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGhlbWVXaW4uY3Bw CShyZXZpc2lvbiAxMTA5NDQpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGhl bWVXaW4uY3BwCSh3b3JraW5nIGNvcHkpCkBAIC03ODQsNiArNzg0LDggQEAgdm9pZCBSZW5kZXJU aGVtZVdpbjo6YWRqdXN0TWVudUxpc3RCdXR0bwogICAgIG1pbkhlaWdodCA9IG1heChtaW5IZWln aHQsIGRyb3BEb3duQm94TWluSGVpZ2h0KTsKIAogICAgIHN0eWxlLT5zZXRNaW5IZWlnaHQoTGVu Z3RoKG1pbkhlaWdodCwgRml4ZWQpKTsKKworICAgIHN0eWxlLT5zZXRMaW5lSGVpZ2h0KFJlbmRl clN0eWxlOjppbml0aWFsTGluZUhlaWdodCgpKTsKICAgICAKICAgICAvLyBXaGl0ZS1zcGFjZSBp cyBsb2NrZWQgdG8gcHJlCiAgICAgc3R5bGUtPnNldFdoaXRlU3BhY2UoUFJFKTsK