Created attachment 101155 [details] Don't include border, padding, and margin when calculating text baseline Observed in the following products: - Chromium 14.0.826.0 on Ubuntu 10.04 - WebKit 535.1 (trunk@91174) - Epiphany 2.30.2, Ubuntu 10.04 In file upload controls (e.g. on http://validator.w3.org/#validate_by_upload), the filename text is not aligned with the button text -- it's several pixels too low (see attached screenshot). I've attached a patch which seem to fix the problem. It looks like the code was accounting for the button's border, padding, & margin when the absoluteBoundingBoxRect already accounts for that.
Please follow the guidelines for contributing code <http://www.webkit.org/coding/contributing.html> to get the fix landed.
http://code.google.com/p/chromium/issues/detail?id=89591
Created attachment 101451 [details] Fix A properly formatted WebKit patch this time.
Attachment 101451 [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:3: Line contains tab character. [whitespace/tab] [5] 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:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 4 in 1 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 101452 [details] Correct style problems.
Comment on attachment 101452 [details] Correct style problems. Attachment 101452 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9193236 New failing tests: fast/forms/input-file-re-render.html fast/forms/form-element-geometry.html fast/forms/file-input-direction.html fast/forms/input-appearance-height.html fast/forms/input-value.html fast/forms/box-shadow-override.html fast/forms/file-input-disabled.html
These failures are all expected, since this patch changes the layout of file upload controls. I think we'll want to rebaseline the tests for this patch. I can include the new images for chromium-linux in my patch, if that's the right thing to do.
Created attachment 101589 [details] Current file upload control with misaligned text.
Created attachment 101590 [details] New file upload control with aligned text.
I've visually confirmed that all of tests failures in fast/forms are due to the changed text alignment, and they should all be rebaselined. Let me know if there's anything else I need to do to help land this patch.
Comment on attachment 101452 [details] Correct style problems. View in context: https://bugs.webkit.org/attachment.cgi?id=101452&action=review > Source/WebCore/ChangeLog:7 > + already accounted for by RenderButton::absoluteBoundingBoxRect(). Are you sure about this? Where? http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp&l=1085
Comment on attachment 101452 [details] Correct style problems. View in context: https://bugs.webkit.org/attachment.cgi?id=101452&action=review >> Source/WebCore/ChangeLog:7 >> + already accounted for by RenderButton::absoluteBoundingBoxRect(). > > Are you sure about this? Where? http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp&l=1085 You're right, that's wrong. I think the margin, border, and padding is accounted for by RenderBox::baselinePosition() (http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderBox.cpp&exact_package=chromium&q=baselinePosition&type=cs&l=3295). I verified this experimentally, but I can double-check the flow with a debugger. I'll fix the comment in a new patch.
Comment on attachment 101452 [details] Correct style problems. View in context: https://bugs.webkit.org/attachment.cgi?id=101452&action=review >>> Source/WebCore/ChangeLog:7 >>> + already accounted for by RenderButton::absoluteBoundingBoxRect(). >> >> Are you sure about this? Where? http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp&l=1085 > > You're right, that's wrong. I think the margin, border, and padding is accounted for by RenderBox::baselinePosition() (http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderBox.cpp&exact_package=chromium&q=baselinePosition&type=cs&l=3295). > > I verified this experimentally, but I can double-check the flow with a debugger. > > I'll fix the comment in a new patch. Whoops, that should be RenderBlock::baselinePosition: http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp&exact_package=chromium&q=baselinePosition&type=cs&l=5174
(In reply to comment #13) > (From update of attachment 101452 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101452&action=review > > >>> Source/WebCore/ChangeLog:7 > >>> + already accounted for by RenderButton::absoluteBoundingBoxRect(). > >> > >> Are you sure about this? Where? http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp&l=1085 > > > > You're right, that's wrong. I think the margin, border, and padding is accounted for by RenderBox::baselinePosition() (http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderBox.cpp&exact_package=chromium&q=baselinePosition&type=cs&l=3295). > > > > I verified this experimentally, but I can double-check the flow with a debugger. > > > > I'll fix the comment in a new patch. > > Whoops, that should be RenderBlock::baselinePosition: http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp&exact_package=chromium&q=baselinePosition&type=cs&l=5174 You must be very careful here. Look at this the baseline: http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-mac-leopard/fast/forms/input-file-re-render-expected.png It's aligned perfectly. This tells me that baselinePosition calculates a different value on Mac. Why? I don't know, but it's worth finding out, since youe change, I am guessing will cause misalignment on Mac.
I did test on Mac, and it doesn't cause a regression. But you're right -- I should figure out why. After some experimentation, it seems that on Mac, the RenderButton always has 0 padding, margin, and border. Using the following test: <!doctype html> <title></title> <input type="file" style="border: 2px solid black;"></input> The render tree on Mac is the following: layer at (0,0) size 800x600 RenderView at (0,0) size 800x600 layer at (0,0) size 800x42 RenderBlock {HTML} at (0,0) size 800x42 RenderBody {BODY} at (8,8) size 784x26 RenderFileUploadControl {INPUT} at (2,2) size 241x22 "no file selected" [border: (2px solid #000000)] RenderButton {INPUT} at (2,2) size 78x18 [bgcolor=#C0C0C0] RenderBlock (anonymous) at (8,2) size 62x13 RenderText at (0,0) size 62x13 text run at (0,0) width 62: "Choose File" RenderText {#text} at (0,0) size 0x0 #EOF And on Chromium Linux: layer at (0,0) size 800x600 RenderView at (0,0) size 800x600 layer at (0,0) size 800x46 RenderBlock {HTML} at (0,0) size 800x46 RenderBody {BODY} at (8,8) size 784x30 RenderFileUploadControl {INPUT} at (2,2) size 242x26 "No file chosen" [border: (2px solid #000000)] RenderButton {INPUT} at (2,2) size 85x22 [bgcolor=#DDDDDD] [border: (2px outset #DDDDDD)] RenderBlock (anonymous) at (8,3) size 69x16 RenderText at (0,0) size 69x16 text run at (0,0) width 69: "Choose File" RenderText {#text} at (0,0) size 0x0 #EOF Note that on Chromium Linux, the RenderButton inside the FileUploadControl has a 2px border as well. I don't know why that's the case though.
(In reply to comment #15) > > Note that on Chromium Linux, the RenderButton inside the FileUploadControl has a 2px border as well. I don't know why that's the case though. Thanks for doing this! Now, the only thing we have left is making sure that we don't turn bots red when we land this. Two ways: 1) Add all tests you expect to fail to test_expectations.txt, land, then pull expectations from the build bots and check them in 2) Add expectations ahead of time into the patch. Certainly, #2 is preferred, but both are acceptable.
Since we are in quite different time zones (I'm in CET), maybe option #2 is best. I can prepare the new baseline results tomorrow morning, and the patch should be ready to land by the time you are back online. BTW, I realized that the border on the RenderButton in this example is always 2px, and it was just a coincidence that I had chosen a 2px border for the upload control style. Even when I set it to 6px, the child RenderButton has the 2px border, so I guess it's just an implementation detail.
Created attachment 102006 [details] Fix with new baselines for chromium-linux, and exceptions for chromium-win and gtk.
Attachment 102006 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Last 3072 characters of output: ions BUGWK64692 : fast/forms/input-value.html ERROR: Line:30 Missing expectations BUGWK64692 : fast/forms/box-shadow-override.html ERROR: Line:31 Missing expectations BUGWK64692 : fast/forms/file-input-disabled.html LayoutTests/platform/gtk/test_expectations.txt:25: Missing expectations BUGWK64692 : fast/forms/input-file-re-render.html [test/expectations] [5] LayoutTests/platform/gtk/test_expectations.txt:26: Missing expectations BUGWK64692 : fast/forms/form-element-geometry.html [test/expectations] [5] LayoutTests/platform/gtk/test_expectations.txt:27: Missing expectations BUGWK64692 : fast/forms/file-input-direction.html [test/expectations] [5] LayoutTests/platform/gtk/test_expectations.txt:28: Missing expectations BUGWK64692 : fast/forms/input-appearance-height.html [test/expectations] [5] LayoutTests/platform/gtk/test_expectations.txt:29: Missing expectations BUGWK64692 : fast/forms/input-value.html [test/expectations] [5] LayoutTests/platform/gtk/test_expectations.txt:30: Missing expectations BUGWK64692 : fast/forms/box-shadow-override.html [test/expectations] [5] LayoutTests/platform/gtk/test_expectations.txt:31: Missing expectations BUGWK64692 : fast/forms/file-input-disabled.html [test/expectations] [5] ERROR: FAILURES FOR <lucid, x86_64, release, cpu> ERROR: Line:3920 Missing expectations BUGWK64692 WIN : fast/forms/input-file-re-render.html ERROR: Line:3921 Missing expectations BUGWK64692 WIN : fast/forms/form-element-geometry.html ERROR: Line:3922 Missing expectations BUGWK64692 WIN : fast/forms/file-input-direction.html ERROR: Line:3923 Missing expectations BUGWK64692 WIN : fast/forms/input-appearance-height.html ERROR: Line:3924 Missing expectations BUGWK64692 WIN : fast/forms/input-value.html ERROR: Line:3925 Missing expectations BUGWK64692 WIN : fast/forms/box-shadow-override.html ERROR: Line:3926 Missing expectations BUGWK64692 WIN : fast/forms/file-input-disabled.html LayoutTests/platform/chromium/test_expectations.txt:3920: Missing expectations BUGWK64692 WIN : fast/forms/input-file-re-render.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3921: Missing expectations BUGWK64692 WIN : fast/forms/form-element-geometry.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3922: Missing expectations BUGWK64692 WIN : fast/forms/file-input-direction.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3923: Missing expectations BUGWK64692 WIN : fast/forms/input-appearance-height.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3924: Missing expectations BUGWK64692 WIN : fast/forms/input-value.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3925: Missing expectations BUGWK64692 WIN : fast/forms/box-shadow-override.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3926: Missing expectations BUGWK64692 WIN : fast/forms/file-input-disabled.html [test/expectations] [5] Total errors found: 14 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 102008 [details] Fix style (add missing expectations). I can't build the GTK port (wrong version of libglib, and I can't upgrade) so I decided to add temporary exceptions for that and the chromium-win port. I've included the new baseline images for chromium-linux though. Hope that's ok. As soon as this lands, I'll upload a patch with the new baseline images for GTK and chromium-win.
(In reply to comment #20) > Created an attachment (id=102008) [details] > Fix style (add missing expectations). > > I can't build the GTK port (wrong version of libglib, and I can't upgrade) so I decided to add temporary exceptions for that and the chromium-win port. I've included the new baseline images for chromium-linux though. Hope that's ok. > > As soon as this lands, I'll upload a patch with the new baseline images for GTK and chromium-win. Did you mean to not mark it for review?
No, I didn't mean to. This being my first WebKit submission, I didn't actually realize I needed to do that...and it took me a few minutes to figure out how :-)
Comment on attachment 102008 [details] Fix style (add missing expectations). Energize!
Comment on attachment 102008 [details] Fix style (add missing expectations). Clearing flags on attachment: 102008 Committed r91759: <http://trac.webkit.org/changeset/91759>
All reviewed patches have been landed. Closing bug.