Summary: | incorrect line-height for styled menulist (select tag) in windows theme (wincairo port) | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Lynn Neir <lynn.neir> | ||||||
Component: | Layout and Rendering | Assignee: | Brent Fulgham <bfulgham> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, bfulgham, lynn.neir, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | Windows 7 | ||||||||
Attachments: |
|
Description
Lynn Neir
2012-02-23 20:34:47 PST
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> |