Bug 109011 - File upload control doesn't apply CSS vertical padding or border to file name
Summary: File upload control doesn't apply CSS vertical padding or border to file name
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 110326
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-05 23:44 PST by Nils Barth
Modified: 2013-02-21 01:01 PST (History)
10 users (show)

See Also:


Attachments
One-line fix (2.47 KB, patch)
2013-02-05 23:45 PST, Nils Barth
no flags Details | Formatted Diff | Diff
oops: left/top (1.78 KB, patch)
2013-02-06 00:00 PST, Nils Barth
no flags Details | Formatted Diff | Diff
Patch (16.25 KB, patch)
2013-02-12 01:30 PST, Nils Barth
no flags Details | Formatted Diff | Diff
Patch (6.34 KB, patch)
2013-02-12 18:29 PST, Nils Barth
no flags Details | Formatted Diff | Diff
Patch (5.80 KB, patch)
2013-02-18 22:31 PST, Nils Barth
no flags Details | Formatted Diff | Diff
Update ChangeLog (5.72 KB, patch)
2013-02-19 01:14 PST, Nils Barth
no flags Details | Formatted Diff | Diff
Update ChangeLog (Morita (1'r') -> Morrita (2'r's)) (5.70 KB, patch)
2013-02-19 23:36 PST, Nils Barth
no flags Details | Formatted Diff | Diff
Turn off platform-specific button rendering (6.12 KB, patch)
2013-02-20 23:02 PST, Nils Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nils Barth 2013-02-05 23:44:15 PST
File upload control doesn't apply CSS vertical padding or border to file name
Comment 1 Nils Barth 2013-02-05 23:45:03 PST
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.
Comment 2 Kent Tamura 2013-02-05 23:50:15 PST
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?
Comment 3 Nils Barth 2013-02-06 00:00:15 PST
Created attachment 186771 [details]
oops: left/top


---
 2 files changed, 11 insertions(+), 1 deletion(-)
Comment 4 Kent Tamura 2013-02-06 00:01:46 PST
Comment on attachment 186771 [details]
oops: left/top

Need a test
Comment 5 Nils Barth 2013-02-06 00:02:17 PST
(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.)
Comment 6 Nils Barth 2013-02-06 00:03:27 PST
(In reply to comment #4)
> (From update of attachment 186771 [details])
> Need a test

Working on it (though I'd marked this --no-review...)
Comment 7 Nils Barth 2013-02-06 00:04:30 PST
(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.
Comment 8 Nils Barth 2013-02-06 00:16:55 PST
Minor question:
Should we add "const" to variables like LayoutUnit textY etc.?
(They're just set once.)
Comment 9 Kent Tamura 2013-02-06 00:18:57 PST
(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.
Comment 10 Nils Barth 2013-02-06 00:27:08 PST
(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!
Comment 11 Alexey Proskuryakov 2013-02-06 11:19:42 PST
> > 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).
Comment 12 Nils Barth 2013-02-08 04:14:59 PST
(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.
Comment 13 Nils Barth 2013-02-12 01:30:04 PST
Created attachment 187807 [details]
Patch
Comment 14 Nils Barth 2013-02-12 01:40:15 PST
(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.
Comment 15 Build Bot 2013-02-12 12:39:47 PST
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
Comment 16 Kent Tamura 2013-02-12 16:42:30 PST
(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.
Comment 17 Kent Tamura 2013-02-12 16:44:33 PST
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.
Comment 18 Nils Barth 2013-02-12 18:27:43 PST
(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.
Comment 19 Nils Barth 2013-02-12 18:29:15 PST
Created attachment 187986 [details]
Patch
Comment 20 Nils Barth 2013-02-12 18:31:36 PST
(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.
Comment 21 Nils Barth 2013-02-15 05:02:27 PST
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.
Comment 22 Nils Barth 2013-02-18 22:31:22 PST
Created attachment 188992 [details]
Patch
Comment 23 Nils Barth 2013-02-18 23:28:06 PST
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.
Comment 24 Hajime Morrita 2013-02-19 00:19:56 PST
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.
Comment 25 Nils Barth 2013-02-19 00:36:06 PST
(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 (^.-)
Comment 26 Nils Barth 2013-02-19 00:39:07 PST
BTW, what's the cq- for?
(not familiar yet)
Is this waiting for tree to open?
Comment 27 Hajime Morrita 2013-02-19 01:07:54 PST
> 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.
Comment 28 Nils Barth 2013-02-19 01:14:51 PST
Created attachment 189017 [details]
Update ChangeLog

Include reviewer, remove obvious CL comments.
Comment 29 Nils Barth 2013-02-19 01:20:12 PST
(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!
Comment 30 Hajime Morrita 2013-02-19 22:00:27 PST
Comment on attachment 189017 [details]
Update ChangeLog

Bureaucracy note: You shouldn't set r+. You can leave it and set only cq+ instead.
Comment 31 WebKit Review Bot 2013-02-19 22:02:37 PST
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
Comment 32 WebKit Review Bot 2013-02-19 22:13:08 PST
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
Comment 33 Nils Barth 2013-02-19 23:36:00 PST
Created attachment 189255 [details]
Update ChangeLog (Morita (1'r') -> Morrita (2'r's))
Comment 34 WebKit Review Bot 2013-02-19 23:39:10 PST
Comment on attachment 189255 [details]
Update ChangeLog (Morita (1'r') -> Morrita (2'r's))

Rejecting attachment 189255 [details] from commit-queue.

nbarth@chromium.org 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.
Comment 35 WebKit Review Bot 2013-02-20 00:11:52 PST
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>
Comment 36 WebKit Review Bot 2013-02-20 00:11:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 37 Takashi Toyoshima 2013-02-20 04:25:12 PST
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.
Comment 38 WebKit Review Bot 2013-02-20 04:28:22 PST
Re-opened since this is blocked by bug 110326
Comment 39 Nils Barth 2013-02-20 20:49:24 PST
(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.
Comment 40 Nils Barth 2013-02-20 23:02:45 PST
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.
Comment 41 Hajime Morrita 2013-02-21 00:31:20 PST
Comment on attachment 189461 [details]
Turn off platform-specific button rendering

Let the bot see if it works.
Comment 42 WebKit Review Bot 2013-02-21 01:01:51 PST
Comment on attachment 189461 [details]
Turn off platform-specific button rendering

Clearing flags on attachment: 189461

Committed r143570: <http://trac.webkit.org/changeset/143570>
Comment 43 WebKit Review Bot 2013-02-21 01:01:57 PST
All reviewed patches have been landed.  Closing bug.