| Summary: | Multiple File Input Icon Set Regardless of File List | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Guy Lewin <guy> | ||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Minor | CC: | achristensen, aestes, ap, bfulgham, commit-queue, dbates, ews-watchlist, guy, jbedard, mjs, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | iPhone / iPad | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=170809 | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 198538 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
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. Created attachment 364212 [details]
Patch
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.
Created attachment 364213 [details]
Patch
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.
Created attachment 364214 [details]
Patch
Wait, so this bug must also be present on Mac and other platforms right? (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? 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. 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? 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 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. Please let me know if there's anything missing from this code fix :) 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?
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. 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. 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? @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? 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. (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? (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? 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. (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. (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. Okay. The feature we discussed before was bug 82229, and clearly it's not fixed. That would help both WebKit2 on Mac, and iOS. (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. 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. Created attachment 366952 [details]
Patch
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 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 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
Created attachment 366978 [details]
Patch
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 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 = '';
(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. > 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. (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. 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 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 on attachment 366978 [details] Patch Clearing flags on attachment: 366978 Committed r244557: <https://trac.webkit.org/changeset/244557> All reviewed patches have been landed. Closing bug. 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. |
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.