Bug 211272 - ContextMenu: entire webpage pops and disappears to generate preview of embedded image
Summary: ContextMenu: entire webpage pops and disappears to generate preview of embed...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
: 211526 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-04-30 19:15 PDT by Dean Jackson
Modified: 2020-05-07 10:17 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.60 KB, patch)
2020-04-30 19:23 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (7.77 KB, patch)
2020-05-06 13:44 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (7.67 KB, patch)
2020-05-06 14:21 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (7.64 KB, patch)
2020-05-07 09:56 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2020-04-30 19:15:36 PDT
ContextMenu:  entire webpage pops and disappears to generate preview of embedded image
Comment 1 Dean Jackson 2020-04-30 19:22:17 PDT
rdar://62482013
Comment 2 Dean Jackson 2020-04-30 19:23:18 PDT
Created attachment 398143 [details]
Patch
Comment 3 Maciej Stachowiak 2020-05-01 00:44:56 PDT
Comment on attachment 398143 [details]
Patch

Is it possible to make a test case for this? Since it regressed once, it would be nice if we could avoid regressing again. (I'm going to guess the sad answer is no, but it would be good to at least explain why a test can't be made).

r=me
Comment 4 Wenson Hsieh 2020-05-01 09:08:13 PDT
Without these early returns, is there anything that prevents the preview container from being unparented while it is being used as targeted context menu preview?

It seems like we ought to either fix the `_actionSheetAssistant` check to only early return if the action sheet assistant is in the process of presenting a context menu, or use a different container view for the action sheet assistant’s preview.
Comment 5 Megan Gardner 2020-05-01 11:27:29 PDT
Comment on attachment 398143 [details]
Patch

these returns are here for a reason, we need to have a solution that does not break the new menus that were added.
Comment 6 Dean Jackson 2020-05-01 14:49:47 PDT
(In reply to Megan Gardner from comment #5)
> Comment on attachment 398143 [details]
> Patch
> 
> these returns are here for a reason, we need to have a solution that does
> not break the new menus that were added.

Then I should roll out your patches because they caused this regression :)

It was wrong to test for the existence of _actionSheetAssistant, since it always exists - it's lifetime is (effectively) tied to the WKContentView. Your change in r260146 should have been rejected. As Maciej pointed out, it sucks that there were no tests for this.

It's also wrong to put logic in the UIContextMenu delegate methods that is affecting behaviour in the action sheet assistant and file upload. The purpose of that code is to react to what the UIContextMenu did, not what else might have happened in the meantime.

It seems the best approach would be to come up with a better solution for the case where multiple things can trigger a UIContextMenu - my suggestion would be that ActionSheetAssistant should go away or be merged with the existing UIContextMenu code. That way we could have one place that knows about what menus are active.

That's a pretty big change though. So in the meantime I think you'll need to either:

- have the action sheet menu and file upload set a flag, only when necessary, that allows our UIContextMenu delegate's animation completion block to *not* clear the view acting as the container. However, that means they'll have to clear the view themselves when necessary to avoid breaking the next context menu.

- use a different container view for action sheets and file uploads
Comment 7 Dean Jackson 2020-05-01 14:51:31 PDT
(In reply to Maciej Stachowiak from comment #3)
> Comment on attachment 398143 [details]
> Patch
> 
> Is it possible to make a test case for this? Since it regressed once, it
> would be nice if we could avoid regressing again. (I'm going to guess the
> sad answer is no, but it would be good to at least explain why a test can't
> be made).
> 
> r=me

I believe we could write an API test that shows a menu, picks the "open" action which would trigger the navigation, navigates back to the original page and attempts to show a menu again.

At the moment we don't have the testing infrastructure to select things from the menu, but we definitely should fix this.
Comment 8 Tim Horton 2020-05-01 14:54:08 PDT
(In reply to Dean Jackson from comment #6)
> (In reply to Megan Gardner from comment #5)
> > Comment on attachment 398143 [details]
> > Patch
> > 
> > these returns are here for a reason, we need to have a solution that does
> > not break the new menus that were added.
> 
> Then I should roll out your patches because they caused this regression :)
> 
> It was wrong to test for the existence of _actionSheetAssistant, since it
> always exists - it's lifetime is (effectively) tied to the WKContentView.
> Your change in r260146 should have been rejected. As Maciej pointed out, it
> sucks that there were no tests for this.

Agreed on both points. But now it's likely easier to fix than revert.

> It's also wrong to put logic in the UIContextMenu delegate methods that is
> affecting behaviour in the action sheet assistant and file upload. The
> purpose of that code is to react to what the UIContextMenu did, not what
> else might have happened in the meantime.
> 
> It seems the best approach would be to come up with a better solution for
> the case where multiple things can trigger a UIContextMenu - my suggestion
> would be that ActionSheetAssistant should go away or be merged with the
> existing UIContextMenu code. That way we could have one place that knows
> about what menus are active.

This is a good long-term strategy. Both WKActionSheetAssistant and the context menu code are pretty 🍝

> That's a pretty big change though. So in the meantime I think you'll need to
> either:
> 
> - have the action sheet menu and file upload set a flag, only when
> necessary, that allows our UIContextMenu delegate's animation completion
> block to *not* clear the view acting as the container. However, that means
> they'll have to clear the view themselves when necessary to avoid breaking
> the next context menu.

These two small changes seem like the easiest path forward

> - use a different container view for action sheets and file uploads
Comment 9 Dean Jackson 2020-05-01 14:59:12 PDT
(In reply to Tim Horton from comment #8)
> (In reply to Dean Jackson from comment #6)
> > (In reply to Megan Gardner from comment #5)
> > > Comment on attachment 398143 [details]
> > > Patch
> > > 
> > > these returns are here for a reason, we need to have a solution that does
> > > not break the new menus that were added.
> > 
> > Then I should roll out your patches because they caused this regression :)
> > 
> > It was wrong to test for the existence of _actionSheetAssistant, since it
> > always exists - it's lifetime is (effectively) tied to the WKContentView.
> > Your change in r260146 should have been rejected. As Maciej pointed out, it
> > sucks that there were no tests for this.
> 
> Agreed on both points. But now it's likely easier to fix than revert.

Yeah, that's why I tried to fix, but I didn't understand why multiple things are now using that container.

> > - have the action sheet menu and file upload set a flag, only when
> > necessary, that allows our UIContextMenu delegate's animation completion
> > block to *not* clear the view acting as the container. However, that means
> > they'll have to clear the view themselves when necessary to avoid breaking
> > the next context menu.
> 
> These two small changes seem like the easiest path forward

As I said, I don't understand why multiple things are using the same container in different ways. The "thing known to rhyme with Sticky Forb" recreates the container each time as it is invoked. But this new code is expected it to remain.
Comment 10 Tim Horton 2020-05-01 15:03:24 PDT
(In reply to Dean Jackson from comment #9)
> (In reply to Tim Horton from comment #8)
> > (In reply to Dean Jackson from comment #6)
> > > (In reply to Megan Gardner from comment #5)
> > > > Comment on attachment 398143 [details]
> > > > Patch
> > > > 
> > > > these returns are here for a reason, we need to have a solution that does
> > > > not break the new menus that were added.
> > > 
> > > Then I should roll out your patches because they caused this regression :)
> > > 
> > > It was wrong to test for the existence of _actionSheetAssistant, since it
> > > always exists - it's lifetime is (effectively) tied to the WKContentView.
> > > Your change in r260146 should have been rejected. As Maciej pointed out, it
> > > sucks that there were no tests for this.
> > 
> > Agreed on both points. But now it's likely easier to fix than revert.
> 
> Yeah, that's why I tried to fix, but I didn't understand why multiple things
> are now using that container.

It seemed OK at the time. Might have been a mistake. They are the same kinds of things.

> > > - have the action sheet menu and file upload set a flag, only when
> > > necessary, that allows our UIContextMenu delegate's animation completion
> > > block to *not* clear the view acting as the container. However, that means
> > > they'll have to clear the view themselves when necessary to avoid breaking
> > > the next context menu.
> > 
> > These two small changes seem like the easiest path forward
> 
> As I said, I don't understand why multiple things are using the same
> container in different ways. The "thing known to rhyme with Sticky Forb"
> recreates the container each time as it is invoked. But this new code is
> expected it to remain.

Nobody's expecting it to remain. Everyone recreates the container when presenting a UIContextMenu. The thing we can't do is remove it when a new UIContextMenu has been presented before the previous one's dismissal completed, which currently means asking 3 different classes if they have done so.
Comment 11 Megan Gardner 2020-05-06 13:16:09 PDT
*** Bug 211526 has been marked as a duplicate of this bug. ***
Comment 12 Megan Gardner 2020-05-06 13:44:04 PDT
Created attachment 398650 [details]
Patch
Comment 13 Tim Horton 2020-05-06 14:05:53 PDT
Comment on attachment 398650 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9419
> +        if (![strongSelf _canRemoveContextMenuView])
>              return;
>          [std::exchange(strongSelf->_contextMenuHintContainerView, nil) removeFromSuperview];

Why not just call _removeContextMenuViewIfPossible here, and then _canRemoveContextMenuView doesn't need to be separate?
Comment 14 Megan Gardner 2020-05-06 14:21:12 PDT
Created attachment 398656 [details]
Patch
Comment 15 Tim Horton 2020-05-06 14:28:51 PDT
Comment on attachment 398656 [details]
Patch

so much better
Comment 16 Wenson Hsieh 2020-05-06 14:31:52 PDT
Comment on attachment 398656 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7716
> +    if (_actionSheetAssistant && [_actionSheetAssistant hasContextMenuInteraction])

Nit - it should be okay to just check `[_actionSheetAssistant hasContextMenuInteraction]` here.
Comment 17 Megan Gardner 2020-05-07 09:56:20 PDT
Created attachment 398749 [details]
Patch for landing
Comment 18 EWS 2020-05-07 10:17:30 PDT
Committed r261303: <https://trac.webkit.org/changeset/261303>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398749 [details].