Bug 31195 - File upload control shows a single filename even if multiple files are selected
Summary: File upload control shows a single filename even if multiple files are selected
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 31154
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-05 19:47 PST by Kent Tamura
Modified: 2009-11-11 22:45 PST (History)
3 users (show)

See Also:


Attachments
Proposed patch (19.66 KB, patch)
2009-11-05 20:00 PST, Kent Tamura
tkent: review-
tkent: commit-queue-
Details | Formatted Diff | Diff
DRT change for RenderFileUploadControl (23.21 KB, patch)
2009-11-10 01:34 PST, Kent Tamura
sam: review-
Details | Formatted Diff | Diff
DRT change for RenderFileUploadControl (rev.2) (24.23 KB, patch)
2009-11-10 17:25 PST, Kent Tamura
darin: review-
Details | Formatted Diff | Diff
Proposed patch (rev.2) (4.48 KB, patch)
2009-11-10 17:44 PST, Kent Tamura
darin: review-
tkent: commit-queue-
Details | Formatted Diff | Diff
DRT change for RenderFileUploadControl (rev.3) (24.35 KB, patch)
2009-11-10 19:08 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.3) (6.88 KB, patch)
2009-11-10 19:12 PST, Kent Tamura
darin: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2009-11-05 19:47:46 PST
Suppose that there is a file upload control which accepts multiple files.
  <input type=file multiple>

1. A user selects 2 files for it; config.sys and autoexec.bat.  The control shows "2 files".
2. Detach the renderer for it.  For example, input.style.display = 'none'
3. Attach a renderer again; For example, input.style.display = 'inline-block'

Expected result:
  4. The control should show "2 files"

Actual result:
  4. The control shows "config.sys", which is the first file of the selected files.
Comment 1 Kent Tamura 2009-11-05 20:00:54 PST
Created attachment 42623 [details]
Proposed patch

This patch depends on a patch in bug#31154.
Comment 2 Darin Adler 2009-11-06 11:21:47 PST
Comment on attachment 42623 [details]
Proposed patch

> +          RenderButton {INPUT} at (0,0) 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
> +      RenderBlock {P} at (0,38) size 784x18
> +        RenderText {#text} at (0,0) size 442x18
> +          text run at (0,0) width 442: "The file upload control above should have text '2 files,' not a filename."

This render tree output looks like a failure. The test says the text should be "2 files", but the text says "Choose File". What's up?

review- because of the test failure issue -- the patch otherwise looks fine
Comment 3 Kent Tamura 2009-11-08 17:40:56 PST
(In reply to comment #2)
> (From update of attachment 42623 [details])
> > +          RenderButton {INPUT} at (0,0) 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
> > +      RenderBlock {P} at (0,38) size 784x18
> > +        RenderText {#text} at (0,0) size 442x18
> > +          text run at (0,0) width 442: "The file upload control above should have text '2 files,' not a filename."
> 
> This render tree output looks like a failure. The test says the text should be
> "2 files", but the text says "Choose File". What's up?

Actually, it's expected result.
"Choose File" is a text inside the button, and "2 files" or "No file chosen" message is never shown in render trees because the message drawn by paintInfo.context->drawBidiText().
Comment 4 Darin Adler 2009-11-09 09:53:16 PST
> Actually, it's expected result.
> "Choose File" is a text inside the button, and "2 files" or "No file chosen"
> message is never shown in render trees because the message drawn by
> paintInfo.context->drawBidiText().

That means that the test result is only effective as a pixel test and nothing in the render tree dump indicates the test's success or failure. I suppose we can live with that since we have to, but the test itself should probably say something about that.
Comment 5 Darin Adler 2009-11-09 09:54:27 PST
Comment on attachment 42623 [details]
Proposed patch

r=me despite my concern that the test does not show success or failure unless you compare pixel test results (which the bots currently do not do).
Comment 6 Sam Weinig 2009-11-09 14:43:50 PST
(In reply to comment #5)
> (From update of attachment 42623 [details])
> r=me despite my concern that the test does not show success or failure unless
> you compare pixel test results (which the bots currently do not do).

Can we not update RenderTreeAsText to print out the necessary information for upload controls?
Comment 7 Kent Tamura 2009-11-10 01:34:11 PST
Created attachment 42853 [details]
DRT change for RenderFileUploadControl

> Can we not update RenderTreeAsText to print out the necessary information for
> upload controls?

It sounds reasonable.  How about this patch?
Comment 8 Sam Weinig 2009-11-10 09:50:07 PST
Comment on attachment 42853 [details]
DRT change for RenderFileUploadControl

This looks good, but I would rather add an isFileUploadControl() to RenderObject instead of the strcmp. r- for that bit.
Comment 9 Kent Tamura 2009-11-10 17:25:28 PST
Created attachment 42913 [details]
DRT change for RenderFileUploadControl (rev.2)

(In reply to comment #8)
> This looks good, but I would rather add an isFileUploadControl() to
> RenderObject instead of the strcmp. r- for that bit.

Done.
Comment 10 Kent Tamura 2009-11-10 17:44:21 PST
Created attachment 42914 [details]
Proposed patch (rev.2)

Update the test expectation for the DRT change.  No update for the C++ code.
Comment 11 Darin Adler 2009-11-10 18:51:31 PST
Comment on attachment 42914 [details]
Proposed patch (rev.2)

Patch does not include the added files.
Comment 12 Darin Adler 2009-11-10 18:52:11 PST
Comment on attachment 42913 [details]
DRT change for RenderFileUploadControl (rev.2)

> +    ASSERT(!object || !strcmp(object->renderName(), "RenderFileUploadControl"));

Still using strcmp here.
Comment 13 Kent Tamura 2009-11-10 19:08:20 PST
Created attachment 42915 [details]
DRT change for RenderFileUploadControl (rev.3)

* Use isFileUploadControl() in toRenderFileUploadControl()s.
Comment 14 Kent Tamura 2009-11-10 19:12:56 PST
Created attachment 42916 [details]
Proposed patch (rev.3)

Oops, added the files.
Comment 15 Darin Adler 2009-11-11 12:49:22 PST
Comment on attachment 42915 [details]
DRT change for RenderFileUploadControl (rev.3)

> +        if (o.isFileUploadControl()) {
> +            ts << " " << quoteAndEscapeNonPrintables(toRenderFileUploadControl(&o)->fileTextValue());
> +        }

One line if statements should not have braces.

r=me, that's minor
Comment 16 WebKit Commit Bot 2009-11-11 17:46:33 PST
Comment on attachment 42915 [details]
DRT change for RenderFileUploadControl (rev.3)

Clearing flags on attachment: 42915

Committed r50851: <http://trac.webkit.org/changeset/50851>
Comment 17 WebKit Commit Bot 2009-11-11 17:46:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Kent Tamura 2009-11-11 20:35:11 PST
Reopen for the remainder patch.
Comment 19 WebKit Commit Bot 2009-11-11 20:44:44 PST
Comment on attachment 42916 [details]
Proposed patch (rev.3)

Rejecting patch 42916 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
/platform/qt/Skipped
	M	LayoutTests/platform/win/Skipped
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderFileUploadControl.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/LayoutTests/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/libexec/git-core//git-svn line 469
Comment 20 Kent Tamura 2009-11-11 22:35:43 PST
(In reply to comment #19)
>     The following ChangeLog files contain OOPS:
> 
>         trunk/LayoutTests/ChangeLog
> 
>     Please don't ever say "OOPS" in a ChangeLog file.

The patch had unnecessary OOPS line.  I'll commit it manually.
Comment 21 Kent Tamura 2009-11-11 22:45:49 PST
Landed as http://trac.webkit.org/changeset/50868