Bug 64692 – File upload control - filename text not aligned with button text

• Component: Forms
• Status: RESOLVED FIXED

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.

[Alexey Proskuryakov]

Please follow the guidelines for contributing code <http://www.webkit.org/coding/contributing.html> to get the fix landed.

[Kent Tamura]

http://code.google.com/p/chromium/issues/detail?id=89591

Created attachment 101451 [details]
Fix

A properly formatted WebKit patch this time.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

(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.

[Dimitri Glazkov (Google)]

(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.

[WebKit Review Bot]

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.

[Dimitri Glazkov (Google)]

(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 :-)

[Dimitri Glazkov (Google)]

Comment on attachment 102008 [details]
Fix style (add missing expectations).

Energize!

[WebKit Review Bot]

Comment on attachment 102008 [details]
Fix style (add missing expectations).

Clearing flags on attachment: 102008

Committed r91759: <http://trac.webkit.org/changeset/91759>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.