Bug 195537 - Multiple File Input Icon Set Regardless of File List
Summary: Multiple File Input Icon Set Regardless of File List
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad All
: P2 Minor
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 198538
  Show dependency treegraph
 
Reported: 2019-03-10 16:15 PDT by Guy Lewin
Modified: 2019-06-04 11:51 PDT (History)
14 users (show)

See Also:


Attachments
Test case for the bug. Input #1 shows the bug, #2 doesn't (550 bytes, text/html)
2019-03-10 16:15 PDT, Guy Lewin
no flags Details
Patch (3.36 KB, patch)
2019-03-10 16:37 PDT, Guy Lewin
no flags Details | Formatted Diff | Diff
Patch (1.31 KB, patch)
2019-03-10 16:45 PDT, Guy Lewin
no flags Details | Formatted Diff | Diff
Patch (1.30 KB, patch)
2019-03-10 16:58 PDT, Guy Lewin
no flags Details | Formatted Diff | Diff
Patch (22.49 KB, patch)
2019-04-08 10:38 PDT, Guy Lewin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.84 MB, application/zip)
2019-04-08 13:31 PDT, Build Bot
no flags Details
Patch (24.24 KB, patch)
2019-04-08 14:10 PDT, Guy Lewin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Guy Lewin 2019-03-10 16:15:46 PDT
Created attachment 364210 [details]
Test case for the bug. Input #1 shows the bug, #2 doesn't

This seems like the iOS version of issue number 67567.
Caused because FileInputType::filesChosen calls iconLoaded() in the end of the function regardless of what happened in the event handlers, and a few lines before the handlers are called.
Attached a demo file, this reproduces on the latest version of WebKit on iOS devices.
Comment 1 Guy Lewin 2019-03-10 16:33:12 PDT
I noticed there's already a test for this (LayoutTests/fast/forms/file/file-input-reset*.html), but since selecting files is a manual operation - it's not checked on iOS regularly. Therefore I can't add a test for this.
Comment 2 Guy Lewin 2019-03-10 16:37:10 PDT
Created attachment 364212 [details]
Patch
Comment 3 Build Bot 2019-03-10 16:38:29 PDT
Attachment 364212 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Guy Lewin 2019-03-10 16:45:08 PDT
Created attachment 364213 [details]
Patch
Comment 5 Build Bot 2019-03-10 16:47:36 PDT
Attachment 364213 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Guy Lewin 2019-03-10 16:58:28 PDT
Created attachment 364214 [details]
Patch
Comment 7 Daniel Bates 2019-03-10 20:53:25 PDT
Wait, so this bug must also be present on Mac and other platforms right?
Comment 8 Daniel Bates 2019-03-10 20:57:17 PDT
(In reply to Guy Lewin from comment #1)
> I noticed there's already a test for this
> (LayoutTests/fast/forms/file/file-input-reset*.html), but since selecting
> files is a manual operation - it's not checked on iOS regularly. Therefore I
> can't add a test for this.

So, does this test fail on Mac or other platforms? If not, why doesn’t it? Continuing with my question in comment #7, if this bug does affect Mac or some other platform then can we write a test for them to ensure we don’t regress this bug? If not, why not? Is there some difference with the UI? If so, why is the fix in common code and not iOS guarded code?
Comment 9 Daniel Bates 2019-03-10 21:05:55 PDT
Using the test case on iPhone X with iOS 12.1.4 (16D57) I get a Web process crash if I pick a photo twice with the first input. If I have a moment I will post the crash report. So, I think there might be more to this bug than meets the eye. Would like to see back trace to know if your proposed fix would resolve this crash. Still want answers to all my earlier questions.
Comment 10 Guy Lewin 2019-03-11 00:31:54 PDT
Daniel - I believe iOS is the only platform that specifies the "icon" argument as not null. (have a look at WebPageProxy::didChooseFilesForOpenPanelWithDisplayStringAndIcon).
Since the "icon" parameter is not under #ifdef - i believe this code shouldn't be either.

As for the crash - that's what got me to start debugging this as well, a crash originating in WebCore::RenderThemeIOS::paintFileUploadIconDecorations. But from what I saw the trunk version fixes this. Am I wrong?
Comment 11 Guy Lewin 2019-03-11 00:33:55 PDT
Daniel - If you want to see the stale icon behaviour - check on the trunk version and not release versions because I believe the crash was fixed in the last few days / weeks
Comment 12 Guy Lewin 2019-03-11 00:44:15 PDT
By the way I had a look at the crash you're experiencing, it's stack trace is always:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000026934b1f7 WebCore::GraphicsContext::platformContext() const + 7
1   com.apple.WebCore             	0x000000026861e2fc WebCore::RenderThemeIOS::paintFileUploadIconDecorations(WebCore::RenderObject const&, WebCore::RenderObject const&, WebCore::PaintInfo const&, WebCore::IntRect const&, WebCore::Icon*, WebCore::RenderTheme::FileUploadDecorations) + 524
2   com.apple.WebCore             	0x00000002694d223f WebCore::RenderFileUploadControl::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 2671
3   com.apple.WebCore             	0x00000002694533af WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 271
4   com.apple.WebCore             	0x00000002694c8082 WebCore::RenderElement::paintAsInlineBlock(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 162
5   com.apple.WebCore             	0x0000000269439687 WebCore::InlineElementBox::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) + 119
6   com.apple.WebCore             	0x000000026943fcf0 WebCore::InlineFlowBox::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) + 1056
...
26  com.apple.WebCore             	0x00000002691c519d WebCore::FrameView::traverseForPaintInvalidation(WebCore::GraphicsContext::PaintInvalidationReasons) + 253
27  com.apple.WebKit              	0x00000001090a1e8a WebKit::RemoteLayerTreeDrawingArea::flushLayers() + 340

It seems like GraphicsContext has uninitialized m_data but still passed the check within platformContext(). I read the code many times to make sure this flow is no longer possible, and indeed the trunk version never crashed to me.
Comment 13 Guy Lewin 2019-03-13 14:41:46 PDT
Please let me know if there's anything missing from this code fix :)
Comment 14 Maciej Stachowiak 2019-03-19 10:46:20 PDT
Comment on attachment 364214 [details]
Patch

Since this fix is in a cross-platform code path, can you explain why the problem is iOS-only, rather than applying to all platforms?
Comment 15 Guy Lewin 2019-03-19 11:52:31 PDT
Maciej - Yeah. The only usage I could find of FileInputType::filesChosen with icon != null is by FileChooser::chooseMediaFiles, which is under #if PLATFORM(IOS).

Since the if within FileInputType::filesChosen exists in all code paths - I believe the fix should be too, as it's relevant for every usage of FileInputType::filesChosen with a not-null icon.

Perhaps unrelated to this bug fix we could put this code branch under #if PLATFORM(IOS) as well or separate into a different method, to save computing time, if you think it's right.
Comment 16 Guy Lewin 2019-04-03 08:52:16 PDT
Hi. It's been almost a month since I filed the fix, is there anything I can do to help integrating this fix? I would like to use relevant Javascript in my web app and this bug is problematic for the iOS experience.
Comment 17 Daniel Bates 2019-04-03 10:46:07 PDT
Code in question most recently modified in bug #179809. That bug has tests. Didn’t read over them..., but something is tested. So, can you please look at the tests and try to write one?
Comment 18 Daniel Bates 2019-04-03 10:48:40 PDT
@Wenson: what is going on here with the code in question? 

To both Guy and Wenson: why only check for empty list before calling iconLoaded() but not in the request icon code above it?
Comment 19 Guy Lewin 2019-04-03 14:19:23 PDT
Thanks @Daniel! Putting the empty list check before where I put it is problematic, as the function setFiles() is the one that can potentially modify the list (more specifically, only keep it the same or reset it).
Therefore I put the check straight after the setFiles() call.

As for the tests - I'll have a look.
Comment 20 Wenson Hsieh 2019-04-03 19:17:01 PDT
(In reply to Daniel Bates from comment #18)
> @Wenson: what is going on here with the code in question? 

This change (r215385) was made when adding support for dragging and dropping into file inputs on iOS. I think the intention was (for some reason) to unify:

    FileInputType::filesChosen(const Vector<FileChooserFileInfo>& files)

and:

    FileInputType::filesChosen(const Vector<FileChooserFileInfo>& paths, const String& displayString, Icon* icon)

...into a single function that takes optional displayString and icon arguments:

    FileInputType::filesChosen(const Vector<FileChooserFileInfo>& paths, const String& = { }, Icon* = nullptr)

I suspect that the intention behind the if (icon) check was to preserve behavior when calling filesChosen() with only files, and no displayString or icon.

> 
> To both Guy and Wenson: why only check for empty list before calling
> iconLoaded() but not in the request icon code above it?
Comment 21 Guy Lewin 2019-04-05 05:11:24 PDT
(In reply to Daniel Bates from comment #17)
> Code in question most recently modified in bug #179809. That bug has tests.
> Didn’t read over them..., but something is tested. So, can you please look
> at the tests and try to write one?

I had another look at the tests. There is a test just for this case, over at:
LayoutTests/fast/forms/file/file-reset-in-change.html
The problem is it uploads a file using eventSender.beginDragWithFiles(). This API is Mac only, I had a look at the ObjC code behind it (EventSendingController.mm) and converting NSPasteboard to UIPasteboard for iOS testing seems like a big change for this diff (if you think it's a good idea - I can do it in another ticket!)
I tried to think of a different way to simulate <input type="file"> file list change with Javascript or with the iOS-available eventSender methods, but there's nothing useful there (if there was a way to set the filelist from Javascript for something not empty - that would be an exploit).

What do you think we should do?
Comment 22 Alexey Proskuryakov 2019-04-05 13:03:43 PDT
Jonathan, do we already have a way to assign a file to an input without dragging? I remember talking about it so many times, but can't find any resolution.
Comment 23 Jonathan Bedard 2019-04-05 14:57:16 PDT
(In reply to Alexey Proskuryakov from comment #22)
> Jonathan, do we already have a way to assign a file to an input without
> dragging? I remember talking about it so many times, but can't find any
> resolution.

Not that I'm aware of. If there was a way to do this, I think Wenson would be the one most familiar with 

I think you may be thinking of a different problem which is about using JS files from the WebKit tree during testing instead of inside the WebCore.framework.
Comment 24 Wenson Hsieh 2019-04-05 15:23:26 PDT
(In reply to Guy Lewin from comment #21)
> (In reply to Daniel Bates from comment #17)
> > Code in question most recently modified in bug #179809. That bug has tests.
> > Didn’t read over them..., but something is tested. So, can you please look
> > at the tests and try to write one?
> 
> I had another look at the tests. There is a test just for this case, over at:
> LayoutTests/fast/forms/file/file-reset-in-change.html
> The problem is it uploads a file using eventSender.beginDragWithFiles().
> This API is Mac only, I had a look at the ObjC code behind it
> (EventSendingController.mm) and converting NSPasteboard to UIPasteboard for
> iOS testing seems like a big change for this diff (if you think it's a good
> idea - I can do it in another ticket!)
> I tried to think of a different way to simulate <input type="file"> file
> list change with Javascript or with the iOS-available eventSender methods,
> but there's nothing useful there (if there was a way to set the filelist
> from Javascript for something not empty - that would be an exploit).
> 
> What do you think we should do?

There are some existing API tests on iOS that drag and drop files onto file inputs (in Tools/TestWebKitAPI). An example of one such test is DragAndDropTests.ExternalSourceImageToFileInput in DragAndDropTestsIOS.mm, which drops an image and verifies that the dropped file is accessible.

Unfortunately, there's no way to get at the icon of a file input from API tests; the layout tests, on the other hand, get this right because they're ref tests that compare resulting snapshots of the two pages.

Your best bet might be to add an internal hook (e.g. on TestRunner, or UIScriptController) that fakes choosing files to upload.
Comment 25 Alexey Proskuryakov 2019-04-05 18:12:49 PDT
Okay. The feature we discussed before was bug 82229, and clearly it's not fixed. That would help both WebKit2 on Mac, and iOS.
Comment 26 Guy Lewin 2019-04-06 02:28:30 PDT
(In reply to Wenson Hsieh from comment #24)
> (In reply to Guy Lewin from comment #21)
> > (In reply to Daniel Bates from comment #17)
> > > Code in question most recently modified in bug #179809. That bug has tests.
> > > Didn’t read over them..., but something is tested. So, can you please look
> > > at the tests and try to write one?
> > 
> > I had another look at the tests. There is a test just for this case, over at:
> > LayoutTests/fast/forms/file/file-reset-in-change.html
> > The problem is it uploads a file using eventSender.beginDragWithFiles().
> > This API is Mac only, I had a look at the ObjC code behind it
> > (EventSendingController.mm) and converting NSPasteboard to UIPasteboard for
> > iOS testing seems like a big change for this diff (if you think it's a good
> > idea - I can do it in another ticket!)
> > I tried to think of a different way to simulate <input type="file"> file
> > list change with Javascript or with the iOS-available eventSender methods,
> > but there's nothing useful there (if there was a way to set the filelist
> > from Javascript for something not empty - that would be an exploit).
> > 
> > What do you think we should do?
> 
> There are some existing API tests on iOS that drag and drop files onto file
> inputs (in Tools/TestWebKitAPI). An example of one such test is
> DragAndDropTests.ExternalSourceImageToFileInput in DragAndDropTestsIOS.mm,
> which drops an image and verifies that the dropped file is accessible.
> 
> Unfortunately, there's no way to get at the icon of a file input from API
> tests; the layout tests, on the other hand, get this right because they're
> ref tests that compare resulting snapshots of the two pages.
> 
> Your best bet might be to add an internal hook (e.g. on TestRunner, or
> UIScriptController) that fakes choosing files to upload.

Thanks for the input. I'll work on adding that internal hook and the test.
Comment 27 Guy Lewin 2019-04-06 03:54:35 PDT
FYI I'm working on mocking FileChooser so we automatically (programatically) choose a file. I think this would be the best way to mimic the natural user behaviour. This should also resolve bug 82229.
Comment 28 Guy Lewin 2019-04-08 10:38:06 PDT
Created attachment 366952 [details]
Patch
Comment 29 Guy Lewin 2019-04-08 10:39:37 PDT
Let me know what you think of this fix. I added a new layout test that I made sure fails without my fix. In order to do that, I had to expose a new API to TestRunner - setOpenPanelFilesMediaIcon(). This has to be manually set (and not built from the file) because TestRunner is not always going through ObjC flows, and I didn't want to complicate our testing code with calling ObjC icon APIs from C++ code.
Comment 30 Build Bot 2019-04-08 13:30:57 PDT
Comment on attachment 366952 [details]
Patch

Attachment 366952 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11808772

New failing tests:
fast/forms/file/file-reset-in-change-using-open-panel-with-icon.html
Comment 31 Build Bot 2019-04-08 13:31:08 PDT
Created attachment 366975 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 32 Guy Lewin 2019-04-08 14:10:44 PDT
Created attachment 366978 [details]
Patch
Comment 33 Guy Lewin 2019-04-08 14:11:19 PDT
Fixed the failing Windows bug by modifying win/TestExpectations and wincairo/TestExpectations according to the similar test file-reset-in-change-using-open-panel.html
Comment 34 Alexey Proskuryakov 2019-04-08 14:54:24 PDT
Very cool.

> This has to be manually set (and not built from the file) because TestRunner is not always going through ObjC flows, and I didn't want to complicate our testing code with calling ObjC icon APIs from C++ code.

Can you elaborate on this? This is quite different from the approach I had in mind, but I don't know if there is some obstacle.

I thought that we could add a testRunner or Internals function that creates a FileList object out of a list of paths, its invocation would be very similar to beginDragWithFiles(). Once there is a FileList object, it can be simply assigned to input.files in JavaScript, and then the engine takes care of figuring out the icon.

So the test would just do 

var file1 = document.getElementById('file1');
file1.files = testRunner.createFileList(["resources/something.txt"]);
file1.value = '';
Comment 35 Guy Lewin 2019-04-08 15:09:54 PDT
(In reply to Alexey Proskuryakov from comment #34)
> Very cool.
> 
> > This has to be manually set (and not built from the file) because TestRunner is not always going through ObjC flows, and I didn't want to complicate our testing code with calling ObjC icon APIs from C++ code.
> 
> Can you elaborate on this? This is quite different from the approach I had
> in mind, but I don't know if there is some obstacle.

Of course, the obstacle here is that WebKitTestRunner/InjectedBundle/Controllers/TestRunner.cpp calls WebKitTestRunner/TestRunner/TestController.cpp:runOpenPanel which calls WKOpenPanelResultListenerChooseMediaFiles and further into chooseFiles().
These are all C++ files.
As far as Iv'e read, the APIs for getting the icon representation of a collection of files is part of UIKit, and therefore will require us getting through Objective C code to call it. I wanted to avoid that for now. 

> I thought that we could add a testRunner or Internals function that creates
> a FileList object out of a list of paths, its invocation would be very
> similar to beginDragWithFiles(). Once there is a FileList object, it can be
> simply assigned to input.files in JavaScript, and then the engine takes care
> of figuring out the icon.
> 
> So the test would just do 
> 
> var file1 = document.getElementById('file1');
> file1.files = testRunner.createFileList(["resources/something.txt"]);
> file1.value = '';
This was my original thought too.

I even began writing mockups to be able to simulate FileChooser, and then I found the setOpenPanelFiles which simulates the file choosing for me in a pretty high up API.

There's not much different between setOpenPanelFiles() and file1.files = .... 
Creating a *media* (with display string and icon) file list and creating a normal file list are different flows (media starts from a different dialog), and therefore the new function setOpenPanelFilesMediaIcon is the switch between these modes.

If we were to add the file1.files = testRunner.createFileList... functionality - we would also have to find a way to distinguish between the media and normal flows, as they are completely different in the code.
Comment 36 Alexey Proskuryakov 2019-04-08 16:49:49 PDT
> As far as Iv'e read, the APIs for getting the icon representation of a collection of files is part of UIKit, and therefore will require us getting through Objective C code to call it. I wanted to avoid that for now. 

I see. Typically, we just put the implementation in a nearby .mm file (e.g. FooClass.cpp and FooClassCocoa.mm or FooClassIOS.mm). Of course, this means that other platforms need an implementation too, if only a stub.

> Creating a *media* (with display string and icon) file list and creating a normal file list are different flows

Sounds reasonable, I'll leave this to someone familiar with this separate flow to review.

It is nice that the test ends up being more cross platform because the icon is hardcoded.
Comment 37 Guy Lewin 2019-04-09 01:41:43 PDT
(In reply to Alexey Proskuryakov from comment #36)
> > As far as Iv'e read, the APIs for getting the icon representation of a collection of files is part of UIKit, and therefore will require us getting through Objective C code to call it. I wanted to avoid that for now. 
> 
> I see. Typically, we just put the implementation in a nearby .mm file (e.g.
> FooClass.cpp and FooClassCocoa.mm or FooClassIOS.mm). Of course, this means
> that other platforms need an implementation too, if only a stub.
> 
> > Creating a *media* (with display string and icon) file list and creating a normal file list are different flows
> 
> Sounds reasonable, I'll leave this to someone familiar with this separate
> flow to review.
> 
> It is nice that the test ends up being more cross platform because the icon
> is hardcoded.

Thanks Alexey :)
Let me know if eventually you think we should switch to the .mm implementation.
Comment 38 Guy Lewin 2019-04-15 10:20:28 PDT
Is there any news around this bug? I added the testing framework feature necessary to test this bugfix and I'd be happy to fix code review suggestions. Please let me know if I can help with anything else.
Comment 39 Alexey Proskuryakov 2019-04-22 09:13:10 PDT
Comment on attachment 366978 [details]
Patch

I'm going to say r+ at this point. If folks don't like something about how it's tested, they can refactor it later. Sorry for the delay!
Comment 40 WebKit Commit Bot 2019-04-23 13:18:13 PDT
Comment on attachment 366978 [details]
Patch

Clearing flags on attachment: 366978

Committed r244557: <https://trac.webkit.org/changeset/244557>
Comment 41 WebKit Commit Bot 2019-04-23 13:18:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 42 Radar WebKit Bug Importer 2019-04-23 13:19:32 PDT
<rdar://problem/50141488>
Comment 43 Alex Christensen 2019-04-23 13:50:03 PDT
Comment on attachment 366978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366978&action=review

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2409
> +    WKRetainPtr<WKDataRef> iconData(AdoptWK, WKBundleCreateWKDataFromUInt8Array(injectedBundle.bundle(), context, data));

This constructor has since been removed.
Comment 44 Alex Christensen 2019-04-23 13:57:20 PDT
https://trac.webkit.org/changeset/244565/webkit