Bug 31195 – File upload control shows a single filename even if multiple files are selected

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 42623 [details]
Proposed patch

This patch depends on a patch in bug#31154.

[Darin Adler]

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

[Kent Tamura]

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

[Darin Adler]

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

[Darin Adler]

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

[Sam Weinig]

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

[Kent Tamura]

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?

[Sam Weinig]

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.

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 42914 [details]
Proposed patch (rev.2)

Update the test expectation for the DRT change.  No update for the C++ code.

[Darin Adler]

Comment on attachment 42914 [details]
Proposed patch (rev.2)

Patch does not include the added files.

[Darin Adler]

Comment on attachment 42913 [details]
DRT change for RenderFileUploadControl (rev.2)

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

Still using strcmp here.

[Kent Tamura]

Created attachment 42915 [details]
DRT change for RenderFileUploadControl (rev.3)

* Use isFileUploadControl() in toRenderFileUploadControl()s.

[Kent Tamura]

Created attachment 42916 [details]
Proposed patch (rev.3)

Oops, added the files.

[Darin Adler]

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

[WebKit Commit Bot]

Comment on attachment 42915 [details]
DRT change for RenderFileUploadControl (rev.3)

Clearing flags on attachment: 42915

Committed r50851: <http://trac.webkit.org/changeset/50851>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

Reopen for the remainder patch.

[WebKit Commit Bot]

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

[Kent Tamura]

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

[Kent Tamura]

Landed as http://trac.webkit.org/changeset/50868