Bug 109011 – File upload control doesn't apply CSS vertical padding or border to file name

• Component: Forms
• Status: RESOLVED FIXED

[Nils Barth]

File upload control doesn't apply CSS vertical padding or border to file name

[Nils Barth]

Created attachment 186769 [details]
One-line fix

Chromium Issue 160385
https://code.google.com/p/chromium/issues/detail?id=160385

Existing code forgets to add padding (or border) when computing position;
this patch fixes it.

[Kent Tamura]

Comment on attachment 186769 [details]
One-line fix

View in context: https://bugs.webkit.org/attachment.cgi?id=186769&action=review

We need a test.
I think we can't make a dumpAsText test for this change.  Probably we need a ref test.

> Source/WebCore/rendering/RenderFileUploadControl.cpp:142
> +        LayoutUnit textY = paintOffset.y() + borderLeft() + paddingLeft() + buttonRenderer->baselinePosition(AlphabeticBaseline, true, HorizontalLine, PositionOnContainingLine);

Why do you add border-left and padding-left to the vertical position?

[Nils Barth]

Created attachment 186771 [details]
oops: left/top


---
 2 files changed, 11 insertions(+), 1 deletion(-)

[Kent Tamura]

Comment on attachment 186771 [details]
oops: left/top

Need a test

[Nils Barth]

(In reply to comment #2)
> (From update of attachment 186769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186769&action=review
> 
> We need a test.
> I think we can't make a dumpAsText test for this change.  Probably we need a ref test.

Ok, will do.

> > Source/WebCore/rendering/RenderFileUploadControl.cpp:142
> > +        LayoutUnit textY = paintOffset.y() + borderLeft() + paddingLeft() + buttonRenderer->baselinePosition(AlphabeticBaseline, true, HorizontalLine, PositionOnContainingLine);
> 
> Why do you add border-left and padding-left to the vertical position?

Oops, fixed!
(I'd accidentally committed the change to master, so I just re-created it manually and accidentally switched left/top; didn't notice in testing b/c same.)

[Nils Barth]

(In reply to comment #4)
> (From update of attachment 186771 [details])
> Need a test

Working on it (though I'd marked this --no-review...)

[Nils Barth]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 186771 [details] [details])
> > Need a test
> 
> Working on it (though I'd marked this --no-review...)

Ah, just needed to fix my scripts using webkit-patch.
Sorry about that.

[Nils Barth]

Minor question:
Should we add "const" to variables like LayoutUnit textY etc.?
(They're just set once.)

[Kent Tamura]

(In reply to comment #8)
> Minor question:
> Should we add "const" to variables like LayoutUnit textY etc.?
> (They're just set once.)

You should not add it in this patch because it is unrelated to the bug.

[Nils Barth]

(In reply to comment #9)
> (In reply to comment #8)
> > Minor question:
> > Should we add "const" to variables like LayoutUnit textY etc.?
> > (They're just set once.)
> 
> You should not add it in this patch because it is unrelated to the bug.

Got it, thanks!

[Alexey Proskuryakov]

> > Should we add "const" to variables like LayoutUnit textY etc.?
> > (They're just set once.)
> 
> You should not add it in this patch because it is unrelated to the bug.

Furthermore, this is not something we do per WebKit coding style (as an unwritten rule, I guess).

[Nils Barth]

(In reply to comment #11)
> > > Should we add "const" to variables like LayoutUnit textY etc.?
> > > (They're just set once.)
> > 
> > You should not add it in this patch because it is unrelated to the bug.
> 
> Furthermore, this is not something we do per WebKit coding style (as an unwritten rule, I guess).

Thanks for explaining; I was a bit puzzled about use of const in WebKit (my understanding was that it was generally avoided for some types, like String, but included for others). I'd noticed:
 const String& displayedFilename = fileTextValue();
 const Font& font = style()->font();
directly above, so I was wondering about it.

But understood -- generally don't use const.

[Nils Barth]

Created attachment 187807 [details]
Patch

[Nils Barth]

(In reply to comment #13)
> Created an attachment (id=187807) [details]
> Patch

One question and two note:
1. Naming convention for layout tests?

In directory: fast/forms/file
there are tests for the file input (<input type="file"/>) element
with both these name types:
  file-input-*
  input-file-*
It's roughly even, with a few more file-input-* so that's what I used,
but I can changed it -- which is preferred?


2. Bug 109104 is related

There is a separate problem with laying out/sizing/rendering the text when the button itself isn't rendered.

Bug 109102 hits this issue (in fact causing a crash), with the note that:
LayoutTests/fast/forms/file/display-none-upload-button.html
// FIXME: The text inside the file input should give the input a contentHeight
// even when there's no upload button.

This affects the same line (due to baseline/vertical spacing), but is a separate issue.

3. Bug causes incorrect layout too

This bug not only causes incorrect positioning of the file name, but also caused the vertical size of the element to be a bit off (too big if there is CSS padding), which can be seen in the layout tests. The patch fixes this.

[Build Bot]

Comment on attachment 187807 [details]
Patch

Attachment 187807 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16510372

New failing tests:
fast/forms/file/file-input-padding-border-2.html
fast/forms/file/file-input-padding-border.html

[Kent Tamura]

(In reply to comment #14)
> 1. Naming convention for layout tests?
> 
> In directory: fast/forms/file
> there are tests for the file input (<input type="file"/>) element
> with both these name types:
>   file-input-*
>   input-file-*
> It's roughly even, with a few more file-input-* so that's what I used,
> but I can changed it -- which is preferred?

WebKit doesn't have common rules for layout test names.
However, as for fast/forms/, we'd like to name a test like:
  fast/forms/<type>/<type>-<concept>.html
fast/forms/number/ and fast/forms/time/ already follow this convention.

The test name would be file-padding-top-or-border-top.html  or something in this case.

> 2. Bug 109104 is related

Please focus on this bug. You should not fix multiple bugs in one patch unless fixes can't be split.

[Kent Tamura]

Comment on attachment 187807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187807&action=review

r- because of test failure on Mac.

> Source/WebCore/rendering/RenderFileUploadControl.cpp:106
> -    
> +

nit: Don't change unrelated lines.

> Source/WebCore/rendering/RenderFileUploadControl.cpp:149
> -        
> +

Ditto.

> Source/WebCore/rendering/RenderFileUploadControl.cpp:152
> -        
> +

Ditto.

> LayoutTests/fast/forms/file/file-input-padding-border-2.html:17
> +    .pad-all { padding: 10px; }
> +    .pad-left { padding-left: 10px; }
> +    .pad-right { padding-right: 10px; }
> +    .pad-top { padding-top: 10px; }
> +    .pad-bottom { padding-bottom: 10px; }
> +    .bor-all { border: 10px solid DarkGray; }
> +    .bor-left { border-left: 10px solid DarkGray; }
> +    .bor-right { border-right: 10px solid DarkGray; }
> +    .bor-top { border-top: 10px solid DarkGray; }
> +    .bor-bottom { border-bottom: 10px solid DarkGray; }

You fix a bug about padding-top and border-top. So you don't need to test padding-left/right/bottom and border-left/right/bottom in this patch.

[Nils Barth]

(In reply to comment #16)
> (In reply to comment #14)
> > 1. Naming convention for layout tests?
> 
> WebKit doesn't have common rules for layout test names.
> However, as for fast/forms/, we'd like to name a test like:
>   fast/forms/<type>/<type>-<concept>.html
> fast/forms/number/ and fast/forms/time/ already follow this convention.
> 
> The test name would be file-padding-top-or-border-top.html  or something in this case.

Got it, thanks!

> > 2. Bug 109104 is related
> 
> Please focus on this bug. You should not fix multiple bugs in one patch unless fixes can't be split.

Understood; just mentioning connection, but only fixing this specific bug.

[Nils Barth]

Created attachment 187986 [details]
Patch

[Nils Barth]

(In reply to comment #17)
> (From update of attachment 187807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187807&action=review
> 
> r- because of test failure on Mac.

Will look into this and fix; need to set up Mac so some delay.
(No r? while looking into this.)

Removed whitespace fixes.

> > LayoutTests/fast/forms/file/file-input-padding-border-2.html:17
> > +    .pad-all { padding: 10px; }
> > +    .pad-left { padding-left: 10px; }
> > +    .pad-right { padding-right: 10px; }
> 
> You fix a bug about padding-top and border-top. So you don't need to test padding-left/right/bottom and border-left/right/bottom in this patch.

Got it -- simplified to just overall and top padding/border;
test now fits on one page.

[Nils Barth]

Investigated this;
the file name position is fine on Safari (this patch fixes bug there too), but the test exposes a different discrepancy.

It's due to the secondary layout issue:

(In reply to comment #14)
> 3. Bug causes incorrect layout too
> This bug not only causes incorrect positioning of the file name, but also caused the vertical size of the element to be a bit off (too big if there is CSS padding), which can be seen in the layout tests. The patch fixes this.

In Safari-Mac the "expected" file has this rendering bug/difference (slight extra vertical padding), hence fails. 

I'll figure out what's causing this (it's a Chromium/Safari rendering difference post-fix); it may be another bug, so I'll see if we can isolate it and address separately if necessary.

[Nils Barth]

Created attachment 188992 [details]
Patch

[Nils Barth]

The Mac reftest failure was a vertical alignment difference,
due to default baseline alignment (not a separate bug).
This is fixed by adding having top alignment; reftest passes manually testing on Linux and Mac.

[Hajime Morrita]

Comment on attachment 188992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188992&action=review

> Source/WebCore/ChangeLog:10
> +        * rendering/RenderFileUploadControl.cpp: 1-line fix

You don't need to claim the fix size here ;-)

> LayoutTests/ChangeLog:6
> +        Add reftest for file input vertical padding layout bug.

You don't need to explain this. It's obvious from the list of added files.

[Nils Barth]

(In reply to comment #24)
> > Source/WebCore/ChangeLog:10
> > +        * rendering/RenderFileUploadControl.cpp: 1-line fix
> 
> You don't need to claim the fix size here ;-)
> 
> > LayoutTests/ChangeLog:6
> > +        Add reftest for file input vertical padding layout bug.
> 
> You don't need to explain this. It's obvious from the list of added files.

They call me "Captain Obvious".
Got it -- will strive to eliminate obvious comments from ChangeLogs too (^.-)

[Nils Barth]

BTW, what's the cq- for?
(not familiar yet)
Is this waiting for tree to open?

[Hajime Morrita]

> BTW, what's the cq- for?
> (not familiar yet)
> Is this waiting for tree to open?
No, r+/cq- meant that "you patch looks good but needs small tweak before landing".
You can rewrite NOBODY in ChangeLog to my name and and upload revised patch with cq?.
You  don't need r+ anymore since the patch already got it.

[Nils Barth]

Created attachment 189017 [details]
Update ChangeLog

Include reviewer, remove obvious CL comments.

[Nils Barth]

(In reply to comment #27)
> No, r+/cq- meant that "you patch looks good but needs small tweak before landing".
> You can rewrite NOBODY in ChangeLog to my name and and upload revised patch with cq?.
> You don't need r+ anymore since the patch already got it.

Thanks, got it -- done!

[Hajime Morrita]

Comment on attachment 189017 [details]
Update ChangeLog

Bureaucracy note: You shouldn't set r+. You can leave it and set only cq+ instead.

[WebKit Review Bot]

Comment on attachment 189017 [details]
Update ChangeLog

Rejecting attachment 189017 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'validate-changelog', '--non-interactive', 189017, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Hajime Morita found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
/mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/16655067

[WebKit Review Bot]

Comment on attachment 189017 [details]
Update ChangeLog

Rejecting attachment 189017 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'validate-changelog', '--non-interactive', 189017, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Hajime Morita found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
/mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/16655075

[Nils Barth]

Created attachment 189255 [details]
Update ChangeLog (Morita (1'r') -> Morrita (2'r's))

[WebKit Review Bot]

Comment on attachment 189255 [details]
Update ChangeLog (Morita (1'r') -> Morrita (2'r's))

Rejecting attachment 189255 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 189255 [details]
Update ChangeLog (Morita (1'r') -> Morrita (2'r's))

Clearing flags on attachment: 189255

Committed r143434: <http://trac.webkit.org/changeset/143434>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Takashi Toyoshima]

Reftest fails on Mac 10.6

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fforms%2Ffile%2Ffile-vertical-padding-border.html

I'm sorry but I'll revert this.

[WebKit Review Bot]

Re-opened since this is blocked by bug 110326

[Nils Barth]

(In reply to comment #37)
> Reftest fails on Mac 10.6
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fforms%2Ffile%2Ffile-vertical-padding-border.html
> 
> I'm sorry but I'll revert this.

Sorry for the bother and no worries!

It's due to button rendering: round/pill-shaped buttons on Mac get clipped on 10.6. I'll turn off the styling to get a rectangular button, which'll hopefully fix it.

[Nils Barth]

Created attachment 189461 [details]
Turn off platform-specific button rendering

Platform-specific button rendering was causing test failures
(due to clipping the edges of the buttons) on Mac OS X 10.6.

[Hajime Morrita]

Comment on attachment 189461 [details]
Turn off platform-specific button rendering

Let the bot see if it works.

[WebKit Review Bot]

Comment on attachment 189461 [details]
Turn off platform-specific button rendering

Clearing flags on attachment: 189461

Committed r143570: <http://trac.webkit.org/changeset/143570>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.