Bug 165225 – UIViewController with WKWebView presented modally causes the presented UIViewController to be dismissed.

• Component: New Bugs
• Status: RESOLVED FIXED

[Brad Wright]

UIViewController with WkWebView presented modally causes the presented UIViewController to be dismissed.

[Brad Wright]

Created attachment 295877 [details]
Patch

[WebKit Commit Bot]

Attachment 295877 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:557:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:559:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brad Wright]

I recreated this original problem in a WkWebView project.

[Brad Wright]

Created attachment 295902 [details]
Example Project that demonstrates the problem

The example project that demonstrates the problem in the WKWebView that this change is trying to solve.

1. Compile and run the project in an iPhone Simulator.
2. When the simulator launches, click on the button in the main view called "launch modal web view".
3. Click the "Choose File" button.
4. Select "iCloud" or "Photo Library" from the menu.
5. The modal web view gets dismissed by this WebKit bug.

The web view should not get dismissed.  The problem here is that the WebKit is trying to close a UIDocumentMenuViewController by calling dismissViewControllerAnimated.  The UIDocumentMenuViewController automatically dismisses itself.  This extra call to dismissViewControllerAnimated is dismissing the modally presented WkWebView.

[Joseph Pecoraro]

Comment on attachment 295877 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        UIViewController with WkWebView presented modally causes the presented UIViewController to be dismissed.

This title seems insufficient. It seems the issues is specific to File Upload so that should be mentioned.

Perhaps a better title would be:

    WKWebView file upload dialog may incorrectly dismiss unrelated modal view controller

Does that sound accurate to you?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:557
> -    if (UICurrentUserInterfaceIdiomIsPad())
> +    if (UICurrentUserInterfaceIdiomIsPad()) {
>          [self _presentPopoverWithContentViewController:viewController animated:YES];
> -    else
> -        [self _presentFullscreenViewController:viewController animated:YES];
> +    } else {

Style: WebKit's Style guideline is to not use braces for single statement conditional blocks. So these braces for the if should not be added. The else, which is multiline should have braces.
https://webkit.org/code-style-guidelines/#braces-one-line

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:560
> +        _presentationViewController = nil;

Hmm, this doesn't seem right.

  - The previous code attempts to dismiss the old view controller it presented and is then presenting the new view controller.
  - The new code ignores the old view controller and presents the new view controller.

Shouldn't we continue to dismiss the old view controller? if _presentationViewController was the incorrect view controller to be dismissing (an ancestor), then perhaps _presentationViewController's presentedViewController would be the correct view controller to dismiss instead of dismissing nothing.

What do you think?

[Brad Wright]

(In reply to comment #5)
> Comment on attachment 295877 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295877&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        UIViewController with WkWebView presented modally causes the presented UIViewController to be dismissed.
> 
> This title seems insufficient. It seems the issues is specific to File
> Upload so that should be mentioned.
> 
> Perhaps a better title would be:
> 
>     WKWebView file upload dialog may incorrectly dismiss unrelated modal
> view controller
> 
> Does that sound accurate to you?
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:557
> > -    if (UICurrentUserInterfaceIdiomIsPad())
> > +    if (UICurrentUserInterfaceIdiomIsPad()) {
> >          [self _presentPopoverWithContentViewController:viewController animated:YES];
> > -    else
> > -        [self _presentFullscreenViewController:viewController animated:YES];
> > +    } else {
> 
> Style: WebKit's Style guideline is to not use braces for single statement
> conditional blocks. So these braces for the if should not be added. The
> else, which is multiline should have braces.
> https://webkit.org/code-style-guidelines/#braces-one-line
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:560
> > +        _presentationViewController = nil;
> 
> Hmm, this doesn't seem right.
> 
>   - The previous code attempts to dismiss the old view controller it
> presented and is then presenting the new view controller.
>   - The new code ignores the old view controller and presents the new view
> controller.

You don't need to dismiss this view controller because it dismisses itself. By explicitly calling another dismiss, the code is a duplicate dismiss which then removes the modal view controller.

This problem will never appear in Safari, because there are no modally presented views.  If you run the same sample project that I uploaded, you will see that the modal view controller is not dismissed.  This is because the popup VC code path does not call dismissViewControllerAnimated.  The problem only happens on a modal VC with a WkWebView and on the iPhone.

> 
> Shouldn't we continue to dismiss the old view controller? if
> _presentationViewController was the incorrect view controller to be
> dismissing (an ancestor), then perhaps _presentationViewController's
> presentedViewController would be the correct view controller to dismiss
> instead of dismissing nothing.
> 
> What do you think?

[Brad Wright]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 295877 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=295877&action=review
> > 
> > > Source/WebKit2/ChangeLog:3
> > > +        UIViewController with WkWebView presented modally causes the presented UIViewController to be dismissed.
> > 
> > This title seems insufficient. It seems the issues is specific to File
> > Upload so that should be mentioned.
> > 
> > Perhaps a better title would be:
> > 
> >     WKWebView file upload dialog may incorrectly dismiss unrelated modal
> > view controller
> > 
> > Does that sound accurate to you?
> > 
> > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:557
> > > -    if (UICurrentUserInterfaceIdiomIsPad())
> > > +    if (UICurrentUserInterfaceIdiomIsPad()) {
> > >          [self _presentPopoverWithContentViewController:viewController animated:YES];
> > > -    else
> > > -        [self _presentFullscreenViewController:viewController animated:YES];
> > > +    } else {
> > 
> > Style: WebKit's Style guideline is to not use braces for single statement
> > conditional blocks. So these braces for the if should not be added. The
> > else, which is multiline should have braces.
> > https://webkit.org/code-style-guidelines/#braces-one-line
> > 
> > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:560
> > > +        _presentationViewController = nil;
> > 
> > Hmm, this doesn't seem right.
> > 
> >   - The previous code attempts to dismiss the old view controller it
> > presented and is then presenting the new view controller.
> >   - The new code ignores the old view controller and presents the new view
> > controller.
> 
> You don't need to dismiss this view controller because it dismisses itself.
> By explicitly calling another dismiss, the code is a duplicate dismiss which
> then removes the modal view controller.
> 
> This problem will never appear in Safari, because there are no modally
> presented views.  If you run the same sample project that I uploaded on the iPad, you
> will see that the modal view controller is not dismissed.  This is because
> the popup VC code path does not call dismissViewControllerAnimated.  The
> problem only happens on a modal VC with a WkWebView and on the iPhone.
> 
> > 
> > Shouldn't we continue to dismiss the old view controller? if
> > _presentationViewController was the incorrect view controller to be
> > dismissing (an ancestor), then perhaps _presentationViewController's
> > presentedViewController would be the correct view controller to dismiss
> > instead of dismissing nothing.
> > 
> > What do you think?

[Brad Wright]

(In reply to comment #5)
> Comment on attachment 295877 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295877&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        UIViewController with WkWebView presented modally causes the presented UIViewController to be dismissed.
> 
> This title seems insufficient. It seems the issues is specific to File
> Upload so that should be mentioned.
> 
> Perhaps a better title would be:
> 
>     WKWebView file upload dialog may incorrectly dismiss unrelated modal
> view controller
> 
> Does that sound accurate to you?
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:557
> > -    if (UICurrentUserInterfaceIdiomIsPad())
> > +    if (UICurrentUserInterfaceIdiomIsPad()) {
> >          [self _presentPopoverWithContentViewController:viewController animated:YES];
> > -    else
> > -        [self _presentFullscreenViewController:viewController animated:YES];
> > +    } else {
> 
> Style: WebKit's Style guideline is to not use braces for single statement
> conditional blocks. So these braces for the if should not be added. The
> else, which is multiline should have braces.
> https://webkit.org/code-style-guidelines/#braces-one-line
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:560
> > +        _presentationViewController = nil;
> 
> Hmm, this doesn't seem right.
> 
>   - The previous code attempts to dismiss the old view controller it
> presented and is then presenting the new view controller.
>   - The new code ignores the old view controller and presents the new view
> controller.
> 
> Shouldn't we continue to dismiss the old view controller? if
> _presentationViewController was the incorrect view controller to be
> dismissing (an ancestor), then perhaps _presentationViewController's
> presentedViewController would be the correct view controller to dismiss
> instead of dismissing nothing.
> 
> What do you think?

If you run the included example project on the iPhone, you will see the WkWebView get's dismissed when it shouldn't.  Then try running on the iPad, and you will see the WkWebView doesn't get dismissed.  This is because there is no extra call to dismissViewControllerAnimated.

Every time you call, dissmissViewControllerAnimaged it just removes another ViewController from the stack of modally presented View Controllers.  UIDocumentMenuViewController already removes itself from the stack automatically.  Thus this duplicate call, is dismissing the wrong ViewController.

[Brad Wright]

Here's the relative section from Apple's documentation on dismissViewControllerAnimated:

"If you present several view controllers in succession, thus building a stack of presented view controllers, calling this method on a view controller lower in the stack dismisses its immediate child view controller and all view controllers above that child on the stack."

In other words, don't call dismissViewControllerAnimated when you don't need to.

[Brad Wright]

We have many customers affected by this issue:

<rdar://problem/25565732>

[Wenson Hsieh]

Comment on attachment 295877 [details]
Patch

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

> Source/WebKit2/ChangeLog:7
> +

^ The above information would be great to include in the ChangeLog here, after the "Reviewed by" line.

[Tim Horton]

This is rdar://problem/28723142.

[Brad Wright]

Created attachment 302681 [details]
New Patch with changes made to conform to the style guide

No significant changes here.  I have been able to run this through and test it on both the iPad and iPhone.

[Brad Wright]

Created attachment 302901 [details]
Able to now fully test in the simulator.  Add critical fixes to WKFileUploadPanel.mm

This has now been tested against the test project in the simulator to verify that the fix does indeed work.

[Wenson Hsieh]

(In reply to comment #14)
> Created attachment 302901 [details]
> Able to now fully test in the simulator.  Add critical fixes to
> WKFileUploadPanel.mm
> 
> This has now been tested against the test project in the simulator to verify
> that the fix does indeed work.

It doesn't look like you uploaded the full patch here. You can use `./Tools/Scripts/webkit-patch upload` to upload the new version of your patch.

[Brad Wright]

Created attachment 302902 [details]
Patch

[Wenson Hsieh]

Comment on attachment 302902 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:583
> +    _presentationViewController = nil;

I don't think there's any purpose in keeping the _presentationViewController ivar if it's going to be nil in all circumstances outside of this method. Is this approach any different than just removing

    if (_presentationViewController) {
        [_presentationViewController dismissViewControllerAnimated:animated completion:^{
            _presentationViewController = nil;
        }];
    }

in -_dismissDisplayAnimated:?

[Wenson Hsieh]

Comment on attachment 302902 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        Need a short description: Fixed the file WKFileUploadPanel.mm and was able run the test app inside the simulator.  

You can remove the "Need a short description:" template here.

> Source/WebKit2/ChangeLog:4
> +        Need the bug URL:https://bugs.webkit.org/show_bug.cgi?id=165225

You can remove the "Need the bug URL:" template here.

> Source/WebKit2/ChangeLog:15
> [phone]  Brad Wright  <[e-mail]>

Please remove this extra ChangeLog entry.

[Brad Wright]

Created attachment 303014 [details]
Patch

[Brad Wright]

(In reply to comment #17)
> Comment on attachment 302902 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302902&action=review
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:583
> > +    _presentationViewController = nil;
> 
> I don't think there's any purpose in keeping the _presentationViewController
> ivar if it's going to be nil in all circumstances outside of this method. Is
> this approach any different than just removing
> 
>     if (_presentationViewController) {
>         [_presentationViewController dismissViewControllerAnimated:animated
> completion:^{
>             _presentationViewController = nil;
>         }];
>     }
> 
> in -_dismissDisplayAnimated:?

I found that the _presentationViewController should not be removed.  I went down the path of removing it, and found that it potentially needed in the class's dismiss routine which was being called externally.  I found that the much easier solution was to simply set the _presentationViewController to nil when the UIDocumentMenuViewController controller was being presented.  Overall, this makes the proposed changes much smaller.  It reduces the risk of any regression bugs from this change.

[Brad Wright]

Created attachment 303114 [details]
Patch

[Wenson Hsieh]

Comment on attachment 303114 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:-389
> -    [self _dismissDisplayAnimated:NO];

_dismissDisplayAnimated: handles dismissing the _presentationPopover. If we aren't going to go through _dismissDisplayAnimated: here, we need to still dismissPopoverAnimated: on the _presentationPopover.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:392
> +    UIViewController *presentedVC = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];

WebKit style guidelines (https://webkit.org/code-style-guidelines/) recommend using full variable names rather than abbreviations, so presentedVC should be presentedViewController.

[Brad Wright]

(In reply to comment #22)
> Comment on attachment 303114 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303114&action=review
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:-389
> > -    [self _dismissDisplayAnimated:NO];
> 
> _dismissDisplayAnimated: handles dismissing the _presentationPopover. If we
> aren't going to go through _dismissDisplayAnimated: here, we need to still
> dismissPopoverAnimated: on the _presentationPopover.

The original dismiss had animated set to NO.  I tested this condition on the iPad with the popover displayed, and it correctly removed the popover from the screen.  Therefore, I believe that calling dismissPopoverAnimated is not needed under this condition, since animated is always set to NO.
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:392
> > +    UIViewController *presentedVC = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> 
> WebKit style guidelines (https://webkit.org/code-style-guidelines/)
> recommend using full variable names rather than abbreviations, so
> presentedVC should be presentedViewController.

Sounds good, I'll change that.

[Brad Wright]

Created attachment 303130 [details]
Patch

[Darin Adler]

Comment on attachment 303130 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> +    // dismiss any viewController that is being presented. This is works all VC, popovers, etc.

Normally we would want to use a capital D in this kind of sentence-style comment.

VC is not normally an abbreviation we use for view controller in WebKit code.

"This is works all", is not grammatically correct.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:394
> +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> +    if (presentedViewController)
> +        [presentedViewController dismissViewControllerAnimated:NO completion:nil];

Would be nice to define the variable inside the "if" since this is C++ code. In fact, I personally would use auto instead of writing out UIViewController * for the type of the variable.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:508
> +    // Clear out any previous view controller reference, to prevent calling dismiss on a view controller that
> +    // was no longer being presented. Don't save a reference to the UIDocumentMenuViewController as it is self dismissing.
> +    _presentationViewController = nil;

I don’t understand this comment and therefore don’t understand the code.

On iPhone, the method called just before this will call _presentFullscreenViewController. Then this line of code will clear out the reference to that just-presented view controller. Given that, why does this comment talk about clearing out a "previous view controller reference"?

[Brad Wright]

(In reply to comment #25)
> Comment on attachment 303130 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303130&action=review
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> > +    // dismiss any viewController that is being presented. This is works all VC, popovers, etc.

This is works for all VC.  Missed the for, in the comment.
> 
> Normally we would want to use a capital D in this kind of sentence-style
> comment.
> 
> VC is not normally an abbreviation we use for view controller in WebKit code.
> 
> "This is works all", is not grammatically correct.
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:394
> > +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> > +    if (presentedViewController)
> > +        [presentedViewController dismissViewControllerAnimated:NO completion:nil];
> 
> Would be nice to define the variable inside the "if" since this is C++ code.
> In fact, I personally would use auto instead of writing out UIViewController
> * for the type of the variable.
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:508
> > +    // Clear out any previous view controller reference, to prevent calling dismiss on a view controller that
> > +    // was no longer being presented. Don't save a reference to the UIDocumentMenuViewController as it is self dismissing.
> > +    _presentationViewController = nil;
> 
> I don’t understand this comment and therefore don’t understand the code.

So what happening is that the _presentationViewController is used to called in:

- (void)_dismissDisplayAnimated:(BOOL)animated
{
    if (_presentationPopover) {
        [_presentationPopover dismissPopoverAnimated:animated];
        [_presentationPopover setDelegate:nil];
        _presentationPopover = nil;
    }

    if (_presentationViewController) {
        [_presentationViewController dismissViewControllerAnimated:animated completion:^{
            _presentationViewController = nil;
        }];
    }
}


The UIDocumentMenuViewController dismisses itself, so when _presentationViewController dismissViewControllerAnimated:animated is called in this function, it dismisses the WKWebView when is presented modally.  That's why it's set to nil in this function, so that the errant code path is never executed.
> 
> On iPhone, the method called just before this will call
> _presentFullscreenViewController. Then this line of code will clear out the
> reference to that just-presented view controller. Given that, why does this
> comment talk about clearing out a "previous view controller reference"?

[Brad Wright]

Created attachment 303134 [details]
Patch

[Brad Wright]

(In reply to comment #26)
> (In reply to comment #25)
> > Comment on attachment 303130 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=303130&action=review
> > 
> > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> > > +    // dismiss any viewController that is being presented. This is works all VC, popovers, etc.
> 
> This is works for all VC.  Missed the for, in the comment.
> > 
> > Normally we would want to use a capital D in this kind of sentence-style
> > comment.
> > 
> > VC is not normally an abbreviation we use for view controller in WebKit code.
> > 
> > "This is works all", is not grammatically correct.
> > 
> > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:394
> > > +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> > > +    if (presentedViewController)
> > > +        [presentedViewController dismissViewControllerAnimated:NO completion:nil];
> > 
> > Would be nice to define the variable inside the "if" since this is C++ code.
> > In fact, I personally would use auto instead of writing out UIViewController
> > * for the type of the variable.
> > 
> > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:508
> > > +    // Clear out any previous view controller reference, to prevent calling dismiss on a view controller that
> > > +    // was no longer being presented. Don't save a reference to the UIDocumentMenuViewController as it is self dismissing.
> > > +    _presentationViewController = nil;
> > 
> > I don’t understand this comment and therefore don’t understand the code.
> 
> So what happening is that the _presentationViewController is used to called
> in:
> 
> - (void)_dismissDisplayAnimated:(BOOL)animated
> {
>     if (_presentationPopover) {
>         [_presentationPopover dismissPopoverAnimated:animated];
>         [_presentationPopover setDelegate:nil];
>         _presentationPopover = nil;
>     }
> 
>     if (_presentationViewController) {
>         [_presentationViewController dismissViewControllerAnimated:animated
> completion:^{
>             _presentationViewController = nil;
>         }];
>     }
> }
> 
> 
> The UIDocumentMenuViewController dismisses itself, so when
> _presentationViewController dismissViewControllerAnimated:animated is called
> in this function, it dismisses the WKWebView when is presented modally. 
> That's why it's set to nil in this function, so that the errant code path is
> never executed.
> > 
> > On iPhone, the method called just before this will call
> > _presentFullscreenViewController. Then this line of code will clear out the
> > reference to that just-presented view controller. Given that, why does this
> > comment talk about clearing out a "previous view controller reference"?


Inside the call stack this is the reference that needs to be cleared out.

1. when this called
- (void)_showDocumentPickerMenu
2.It then calls this:
- (void)_presentMenuOptionForCurrentInterfaceIdiom:(UIViewController *)viewController
3. Finally, inside here the reference is saved.
- (void)_presentFullscreenViewController:(UIViewController *)viewController animated:(BOOL)animated
{
    ...
    _presentationViewController = [UIViewController 
...
}

4. When we return up the stack, I need to unset the reference.

- (void)_showDocumentPickerMenu


I put this comment and the nil reference as close together as possible to minimize future errors.

[Brad Wright]

(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > Comment on attachment 303130 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=303130&action=review
> > > 
> > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> > > > +    // dismiss any viewController that is being presented. This is works all VC, popovers, etc.
> > 
> > This is works for all VC.  Missed the for, in the comment.
> > > 
> > > Normally we would want to use a capital D in this kind of sentence-style
> > > comment.
> > > 
> > > VC is not normally an abbreviation we use for view controller in WebKit code.
> > > 
> > > "This is works all", is not grammatically correct.
> > > 
> > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:394
> > > > +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> > > > +    if (presentedViewController)
> > > > +        [presentedViewController dismissViewControllerAnimated:NO completion:nil];
> > > 
> > > Would be nice to define the variable inside the "if" since this is C++ code.
> > > In fact, I personally would use auto instead of writing out UIViewController
> > > * for the type of the variable.
> > > 
> > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:508
> > > > +    // Clear out any previous view controller reference, to prevent calling dismiss on a view controller that
> > > > +    // was no longer being presented. Don't save a reference to the UIDocumentMenuViewController as it is self dismissing.
> > > > +    _presentationViewController = nil;
> > > 
> > > I don’t understand this comment and therefore don’t understand the code.
> > 
> > So what happening is that the _presentationViewController is used to called
> > in:
> > 
> > - (void)_dismissDisplayAnimated:(BOOL)animated
> > {
> >     if (_presentationPopover) {
> >         [_presentationPopover dismissPopoverAnimated:animated];
> >         [_presentationPopover setDelegate:nil];
> >         _presentationPopover = nil;
> >     }
> > 
> >     if (_presentationViewController) {
> >         [_presentationViewController dismissViewControllerAnimated:animated
> > completion:^{
> >             _presentationViewController = nil;
> >         }];
> >     }
> > }
> > 
> > 
> > The UIDocumentMenuViewController dismisses itself, so when
> > _presentationViewController dismissViewControllerAnimated:animated is called
> > in this function, it dismisses the WKWebView when is presented modally. 
> > That's why it's set to nil in this function, so that the errant code path is
> > never executed.
> > > 
> > > On iPhone, the method called just before this will call
> > > _presentFullscreenViewController. Then this line of code will clear out the
> > > reference to that just-presented view controller. Given that, why does this
> > > comment talk about clearing out a "previous view controller reference"?
> 
> 
> Inside the call stack this is the reference that needs to be cleared out.
> 
> 1. We start here inside:
> - (void)_showDocumentPickerMenu
> 2.Then this is called:
> - (void)_presentMenuOptionForCurrentInterfaceIdiom:(UIViewController
> *)viewController
> 3. Finally, inside here the property _presentationViewController is set.
> - (void)_presentFullscreenViewController:(UIViewController *)viewController
> animated:(BOOL)animated
> {
>     ...
>     _presentationViewController = [UIViewController 
> ...
> }
> 
> 4. When we return up the stack, we need to set _presentationViewController back to nil.
> 
> - (void)_showDocumentPickerMenu
> 
> 
> I put this comment and the nil reference as close together as possible to
> minimize future errors.

[Brad Wright]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #26)
> > > (In reply to comment #25)
> > > > Comment on attachment 303130 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=303130&action=review
> > > > 
> > > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> > > > > +    // dismiss any viewController that is being presented. This is works all VC, popovers, etc.
> > > 
> > > This is works for all VC.  Missed the for, in the comment.
> > > > 
> > > > Normally we would want to use a capital D in this kind of sentence-style
> > > > comment.
> > > > 
> > > > VC is not normally an abbreviation we use for view controller in WebKit code.
> > > > 
> > > > "This is works all", is not grammatically correct.
> > > > 
> > > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:394
> > > > > +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> > > > > +    if (presentedViewController)
> > > > > +        [presentedViewController dismissViewControllerAnimated:NO completion:nil];
> > > > 
> > > > Would be nice to define the variable inside the "if" since this is C++ code.
> > > > In fact, I personally would use auto instead of writing out UIViewController
> > > > * for the type of the variable.
> > > > 
> > > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:508
> > > > > +    // Clear out any previous view controller reference, to prevent calling dismiss on a view controller that
> > > > > +    // was no longer being presented. Don't save a reference to the UIDocumentMenuViewController as it is self dismissing.
> > > > > +    _presentationViewController = nil;
> > > > 
> > > > I don’t understand this comment and therefore don’t understand the code.
> > > 
> > > So what happening is that the _presentationViewController is used to called
> > > in:
> > > 
> > > - (void)_dismissDisplayAnimated:(BOOL)animated
> > > {
> > >     if (_presentationPopover) {
> > >         [_presentationPopover dismissPopoverAnimated:animated];
> > >         [_presentationPopover setDelegate:nil];
> > >         _presentationPopover = nil;
> > >     }
> > > 
> > >     if (_presentationViewController) {
> > >         [_presentationViewController dismissViewControllerAnimated:animated
> > > completion:^{
> > >             _presentationViewController = nil;
> > >         }];
> > >     }
> > > }
> > > 
> > > 
> > > The UIDocumentMenuViewController dismisses itself, so when
> > > _presentationViewController dismissViewControllerAnimated:animated is called
> > > in this function, it dismisses the WKWebView when is presented modally. 
> > > That's why it's set to nil in this function, so that the errant code path is
> > > never executed.
> > > > 
> > > > On iPhone, the method called just before this will call
> > > > _presentFullscreenViewController. Then this line of code will clear out the
> > > > reference to that just-presented view controller. Given that, why does this
> > > > comment talk about clearing out a "previous view controller reference"?
> > 
> > 
> > Inside the call stack this is the reference that needs to be cleared out.
> > 
> > 1. We start here inside:
> > - (void)_showDocumentPickerMenu
> > 2.Then this is called:
> > - (void)_presentMenuOptionForCurrentInterfaceIdiom:(UIViewController
> > *)viewController
> > 3. Finally, inside here the property _presentationViewController is set.
> > - (void)_presentFullscreenViewController:(UIViewController *)viewController
> > animated:(BOOL)animated
> > {
> >     ...
> >     _presentationViewController = [UIViewController 
> > ...
> > }
> > 
> > 4. When we return up the stack, we need to set _presentationViewController back to nil.
> > 
> > - (void)_showDocumentPickerMenu
> > 
> > 
> > I put this comment and the nil reference as close together as possible to
> > minimize future errors.

The above code is not C++, but Objective-C.  I know this is a mixed Objective-C and C++  .mm file, but I'm not aware of any memory overhead penalty for this pointer variable in Objective-C.

[Brad Wright]

Created attachment 303250 [details]
Example test project for reproducing the problem.

Here's an example test project that was created to reproduce the problem.  

To Test:

1.  Drag the project into the WebKit.xcworkspace
2.  Compile and run.
3.  Click on the button on the main window to launch the modal WKWebView.
4.  Click on the button inside the WKWebView.
5.  Select Photos or Cloud from the menu.
6.  Without the fix, the Modal WKWebView will get disappear.
7.  With the fix, you will be able to select a photo.

[Wenson Hsieh]

Comment on attachment 303134 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> +    // dismiss any viewController that is being presented. This is works for all types of viewcontrollers , popovers, etc.

Please capitalize "dismiss"

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:392
> +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];

So if I understand correctly, [UIViewController _viewControllerForFullScreenPresentationFromView:_view] will fetch the _presentationPopover, if it exists, as well? If that is really the case, then this is OK, but we should at least tear down _presentationPopover properly (i.e. nil out _presentationPopover and set its delegate to nil).

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:506
> +    // Clear out any previous view controller reference, to prevent calling dismiss on a view controller that

Per Darin's advice a few comments above, this should really say "Clear out the view controller we just presented" rather than refer to the view controller we're clearing out as the "previous" one.

[Wenson Hsieh]

Comment on attachment 303134 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        Don't dismiss the viewcontroller if the viewcontroller dismisses itself.  - (void)dismiss now removes any presented view controller directly.

Please fix this title as well.

[Wenson Hsieh]

Comment on attachment 303134 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:393
> +    if (presentedViewController)

This nil check is not needed (if !presentedViewController, this will just be a noop).

[Brad Wright]

Comment on attachment 303134 [details]
Patch

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

>> Source/WebKit2/ChangeLog:3
>> +        Don't dismiss the viewcontroller if the viewcontroller dismisses itself.  - (void)dismiss now removes any presented view controller directly.
> 
> Please fix this title as well.

Is this okay?  

Clear out the view controller we just presented. Don't dismiss the UIDocumentMenuViewController as it is self dismissing.

[Wenson Hsieh]

(In reply to comment #35)
> Comment on attachment 303134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303134&action=review
> 
> >> Source/WebKit2/ChangeLog:3
> >> +        Don't dismiss the viewcontroller if the viewcontroller dismisses itself.  - (void)dismiss now removes any presented view controller directly.
> > 
> > Please fix this title as well.
> 
> Is this okay?  
> 
> Clear out the view controller we just presented. Don't dismiss the
> UIDocumentMenuViewController as it is self dismissing.

Oh, sorry -- I should've clarified; typically, the title of the ChangeLog entry is the title of the bug itself (so in this case, it would be 'UIViewController with WKWebView presented modally causes the presented UIViewController to be dismissed.')

[Tim Horton]

(In reply to comment #36)
> (In reply to comment #35)
> > Comment on attachment 303134 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=303134&action=review
> > 
> > >> Source/WebKit2/ChangeLog:3
> > >> +        Don't dismiss the viewcontroller if the viewcontroller dismisses itself.  - (void)dismiss now removes any presented view controller directly.
> > > 
> > > Please fix this title as well.
> > 
> > Is this okay?  
> > 
> > Clear out the view controller we just presented. Don't dismiss the
> > UIDocumentMenuViewController as it is self dismissing.
> 
> Oh, sorry -- I should've clarified; typically, the title of the ChangeLog
> entry is the title of the bug itself (so in this case, it would be
> 'UIViewController with WKWebView presented modally causes the presented
> UIViewController to be dismissed.')

Take a look at https://trac.webkit.org/changeset/213304 for a good example of change log format.

[Brad Wright]

Comment on attachment 303134 [details]
Patch

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

>> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:392
>> +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> 
> So if I understand correctly, [UIViewController _viewControllerForFullScreenPresentationFromView:_view] will fetch the _presentationPopover, if it exists, as well? If that is really the case, then this is OK, but we should at least tear down _presentationPopover properly (i.e. nil out _presentationPopover and set its delegate to nil).

We don't actually need to call [_presentationPopover dismissPopoverAnimated:animated]; because this will take care of it:
[[UIViewController _viewControllerForFullScreenPresentationFromView:_view] dismissViewControllerAnimated:NO completion:nil];
I'm going to add this:
// Clean up any pointers
    [_presentationPopover setDelegate:nil];
    _presentationPopover = nil;
    _presentationViewController = nil;

[Brad Wright]

Created attachment 303279 [details]
Patch

[Wenson Hsieh]

Comment on attachment 303279 [details]
Patch

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

Added a very minor comment. I think this is a reasonable change, but I'm not a WK2 reviewer -- Tim, what do you think?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:396
> +    // Clean up any pointers

Nit - I don't think this comment adds much.

[Tim Horton]

Comment on attachment 303279 [details]
Patch

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

> Source/WebKit2/ChangeLog:13
> +        (-[WKFileUploadPanel _cancel]):
> +        (-[WKFileUploadPanel dismiss]):
> +        (-[WKFileUploadPanel _showDocumentPickerMenu]):
> +        (-[WKFileUploadPanel _presentMenuOptionForCurrentInterfaceIdiom:]):
> +        (-[WKFileUploadPanel _presentForCurrentInterfaceIdiom:]): Deleted.

Ideally this would explain why you're doing what you're doing.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> +    // Dismiss any viewController that is being presented. This works for all types of viewcontrollers , popovers, etc.

"view controllers, popovers, etc." (spaces!) Also, each mention of viewController in this comment should probably be "view controller"

>> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:396
>> +    // Clean up any pointers
> 
> Nit - I don't think this comment adds much.

Yep, this is a "what" comment, not a "why" comment, so it should go away.

[Brad Wright]

Created attachment 303282 [details]
Patch

[Wenson Hsieh]

Comment on attachment 303282 [details]
Patch

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

Please account for Tim's review feedback -- the rest of the patch looks good!

> Source/WebKit2/ChangeLog:8
> +        * UIProcess/ios/forms/WKFileUploadPanel.mm:

As Tim suggested earlier, you should include a couple of sentences here describing what the problem was, and how this change addresses the problem. You can refer to https://trac.webkit.org/changeset/213304 for a great example of what a ChangeLog should look like.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> +    // Dismiss any viewController that is being presented. This works for all types of viewcontrollers , popovers, etc.

As Tim suggested earlier, please replace "viewController" with "view controller". There is also an extra space before ", popovers" that you should remove.

[Brad Wright]

Created attachment 303318 [details]
Patch

[Wenson Hsieh]

Comment on attachment 303318 [details]
Patch

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

Just minor nits and ChangeLog formatting issues -- the rest of the patch looks good.

> Source/WebKit2/ChangeLog:6
> +        The problem happens on an iPhone with a WKWebView inside a view controller presented modally. If the user selects an HTML file input 

Please move this description down below the "Reviewed by" line. The ordering of things should be:

```
<Title of the bug>
<bug URL>
<rdar link, if any>

Reviewed by <reviewer name>

<description of problem and how the change fixes it>

<list of individial methods changed, with inline comments if needed>
```

You may refer to an example, such as https://trac.webkit.org/changeset/213348/trunk/LayoutTests/ChangeLog, for what a ChangeLog entry should look like.

> Source/WebKit2/ChangeLog:13
> +        https://bugs.webkit.org/show_bug.cgi?id=165225

Please remove this duplicated bug URL

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> +    // Dismiss any viewController that is being presented. This works for all types of view controllers, popovers, etc.

As Tim suggested earlier, please replace "viewController" with "view controller".

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:392
> +    // If there is any kind of viewController presented on this view, it will be removed. 

Please replace "viewController" with "view controller" here too.

[Build Bot]

Comment on attachment 303318 [details]
Patch

Attachment 303318 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3230152

New failing tests:
media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html

[Build Bot]

Created attachment 303325 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Brad Wright]

These changes are iOS only.  There is no impact to the layout of HTML.

[Brad Wright]

Created attachment 303666 [details]
Patch

[Brad Wright]

Created attachment 303668 [details]
Patch

[Wenson Hsieh]

Comment on attachment 303668 [details]
Patch

r=me (and Tim Horton)

[Wenson Hsieh]

Comment on attachment 303668 [details]
Patch

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

On a second look, I noticed the ChangeLog is still formatted strangely. The description should be after the "Reviewed by" line with a newline in between. Please address this issue!

> Source/WebKit2/ChangeLog:13
> +        Reviewed by NOBODY (OOPS!).

The description goes after the "Reviewed by" line.

[Brad Wright]

Created attachment 303676 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 303676 [details]
Patch

Clearing flags on attachment: 303676

Committed r213570: <http://trac.webkit.org/changeset/213570>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Zach Waugh]

This issue is still present as of iOS 11.3. Choosing a file from within a WKWebView presented in a modal will show the iOS 11 file browser modal. Selecting a file or choosing "Cancel" from from the file browser modal will dismiss the file browser and the UIViewController underneath it

[Brad Wright]

Hi Zach:

I just tested the file input tag in iOS 11.3, and it passed my test.  The WKWebView does not close when presented modally.  It appears to me that this problem has been fixed.  You may be having a different issue, so please take a look at how I conducted my test.  Here is the relevant HTML that I used for the file input tag:

<div class="contentBody">
<p> Click on this button <input type="file"/></p>
</div> 

(In reply to Zach Waugh from comment #56)
> This issue is still present as of iOS 11.3. Choosing a file from within a
> WKWebView presented in a modal will show the iOS 11 file browser modal.
> Selecting a file or choosing "Cancel" from from the file browser modal will
> dismiss the file browser and the UIViewController underneath it

[Wenson Hsieh]

(In reply to Zach Waugh from comment #56)
> This issue is still present as of iOS 11.3. Choosing a file from within a
> WKWebView presented in a modal will show the iOS 11 file browser modal.
> Selecting a file or choosing "Cancel" from from the file browser modal will
> dismiss the file browser and the UIViewController underneath it

Hi Zach — thanks for letting us know! Would you mind opening a new bug for this issue, with a test case? The original repro case (attached here by Brad) seems fixed...

[Zach Waugh]

Thanks for the quick response. I've filed a new bug here: https://bugs.webkit.org/show_bug.cgi?id=185257