Bug 21908 - size attribute has no effect for input tag file type
Summary: size attribute has no effect for input tag file type
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-27 16:17 PDT by Hin-Chung Lam
Modified: 2010-06-10 16:29 PDT (History)
1 user (show)

See Also:


Attachments
bug sample (1.93 KB, text/html)
2008-10-27 16:19 PDT, Hin-Chung Lam
no flags Details
bug image (76.93 KB, image/png)
2008-10-27 16:19 PDT, Hin-Chung Lam
no flags Details
patch for r37934 (1.40 KB, patch)
2008-10-28 13:46 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
patch for code, added layout test and modify old layout tests (470.52 KB, patch)
2009-01-07 22:38 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
patch the code and updated layout tests (470.55 KB, patch)
2009-01-08 14:54 PST, Hin-Chung Lam
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Chung Lam 2008-10-27 16:17:16 PDT
### PROBLEM ###
Although the input elements have different size attribute values, they 
have the same width.

### EXPECTED ###
The size attribute takes effect.

Chrome 0.3.154.0
Windows XP SP2 en

Not repro on Firefox 3.0.3, IE7 and IE8 beta2.
Repro on Safari 3.1.2.

Originally found in Chrome, please refer to the following link for samples:
http://code.google.com/p/chromium/issues/detail?id=3238
Comment 1 Hin-Chung Lam 2008-10-27 16:19:10 PDT
Created attachment 24699 [details]
bug sample
Comment 2 Hin-Chung Lam 2008-10-27 16:19:39 PDT
Created attachment 24700 [details]
bug image
Comment 3 Hin-Chung Lam 2008-10-28 13:46:34 PDT
Created attachment 24727 [details]
patch for r37934
Comment 4 Hin-Chung Lam 2009-01-07 22:38:04 PST
Created attachment 26520 [details]
patch for code, added layout test and modify old layout tests
Comment 5 Hin-Chung Lam 2009-01-07 22:41:25 PST
The bug happens in nightly build too
Comment 6 Darin Adler 2009-01-08 14:05:08 PST
Comment on attachment 26520 [details]
patch for code, added layout test and modify old layout tests

I think the current behavior is intentional. As a security measure, browsers don't allow styling of <input type="file">. Perhaps this long-standing security measure is obsolete, though. We'd have to decide this before making a change.

Also, this ChangeLog entries in this patch have tab characters in them. We don't allow those.
Comment 7 Hin-Chung Lam 2009-01-08 14:54:38 PST
Created attachment 26538 [details]
patch the code and updated layout tests
Comment 8 Hin-Chung Lam 2009-01-08 14:55:32 PST
removed tabs in ChangeLogs
Comment 9 Darin Adler 2009-01-10 14:51:05 PST
Comment on attachment 26538 [details]
patch the code and updated layout tests

OK, I looked more carefully at your comments and it seems you did check the behavior of other browsers.

> -        m_maxPrefWidth = (int)ceilf(charWidth * defaultWidthNumChars);
> +        m_maxPrefWidth = (int)ceilf(charWidth * static_cast<HTMLInputElement*>(node())->size()
> +                         + m_button->renderer()->maxPrefWidth() + afterButtonSpacing
> +                         + (m_fileChooser->icon() ? iconWidth + iconFilenameSpacing : 0));

Will this give us the correct behavior if someone sets the size attribute to zero or a large negative number? Do we need some kind of bounds check on the value of size() or can we just use the value no matter what integer it is? What do other browsers do in these edge cases? It also seems clear that this could overflow if the value of size is something gigantic.

The old default size was 34 characters, but the default value for HTMLInputElement's size is 20. We do add space for the other elements. But does this make the default size smaller or larger?

Is it really sensible to treat the size attribute as "number of characters of average width available for the filename"? It seems a little strange to me, and I'm not sure that's the most helpful behavior for website authors.

The defaultWidthNumChars constant is now unused. Please remove it if you're removing the last use of it.

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 39698)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,35 @@
> +2009-01-07  Alpha Lam  <hclam@google.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +       Added layout test for size property of <input type="file"> tag, updating other layout tests related to issue.
> +
> +       https://bugs.webkit.org/show_bug.cgi?id=21908

These are indented incorrectly. 7 spaces rather than 8.

> Index: LayoutTests/fast/replaced/table-percent-height-expected.txt
> ===================================================================
> --- LayoutTests/fast/replaced/table-percent-height-expected.txt	(revision 39697)
> +++ LayoutTests/fast/replaced/table-percent-height-expected.txt	(working copy)
> @@ -63,9 +63,9 @@ PASS getComputedStyleForElement(document
>  PASS getComputedStyleForElement(document.getElementById('input-checkbox-75'), 'height') is '4px'
>  PASS getComputedStyleForElement(document.getElementById('input-checkbox-100'), 'width') is '12px'
>  PASS getComputedStyleForElement(document.getElementById('input-checkbox-100'), 'height') is '6px'
> -PASS getComputedStyleForElement(document.getElementById('input-file-75'), 'width') is '237px'
> +FAIL getComputedStyleForElement(document.getElementById('input-file-75'), 'width') should be 237px. Was 222px.
>  PASS getComputedStyleForElement(document.getElementById('input-file-75'), 'height') is '13px'
> -PASS getComputedStyleForElement(document.getElementById('input-file-100'), 'width') is '237px'
> +FAIL getComputedStyleForElement(document.getElementById('input-file-100'), 'width') should be 237px. Was 222px.

I think we should do better than just checking in failing test results here. If the results should change, then we should change the expected results in the test too. If the results should not change, then lets not make the code change.

I'm going to say review- because I think that the non-trivial concerns above should be resolved before making this change.