Summary: | File upload control shows a single filename even if multiple files are selected | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, darin, sam | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | 31154 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Kent Tamura
2009-11-05 19:47:46 PST
Created attachment 42623 [details] Proposed patch This patch depends on a patch in bug#31154. 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 (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(). > 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 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).
(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? 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 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.
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. Created attachment 42914 [details]
Proposed patch (rev.2)
Update the test expectation for the DRT change. No update for the C++ code.
Comment on attachment 42914 [details]
Proposed patch (rev.2)
Patch does not include the added files.
Comment on attachment 42913 [details] DRT change for RenderFileUploadControl (rev.2) > + ASSERT(!object || !strcmp(object->renderName(), "RenderFileUploadControl")); Still using strcmp here. Created attachment 42915 [details]
DRT change for RenderFileUploadControl (rev.3)
* Use isFileUploadControl() in toRenderFileUploadControl()s.
Created attachment 42916 [details]
Proposed patch (rev.3)
Oops, added the files.
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 on attachment 42915 [details] DRT change for RenderFileUploadControl (rev.3) Clearing flags on attachment: 42915 Committed r50851: <http://trac.webkit.org/changeset/50851> All reviewed patches have been landed. Closing bug. Reopen for the remainder patch. 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
(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. Landed as http://trac.webkit.org/changeset/50868 |