Bug 211272 – ContextMenu: entire webpage pops and disappears to generate preview of embedded image

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

ContextMenu:  entire webpage pops and disappears to generate preview of embedded image

[Dean Jackson]

rdar://62482013

[Dean Jackson]

Created attachment 398143 [details]
Patch

[Maciej Stachowiak]

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

[Wenson Hsieh]

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.

[Megan Gardner]

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.

[Dean Jackson]

(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

[Dean Jackson]

(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.

[Tim Horton]

(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

[Dean Jackson]

(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.

[Tim Horton]

(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.

[Megan Gardner]

*** Bug 211526 has been marked as a duplicate of this bug. ***

[Megan Gardner]

Created attachment 398650 [details]
Patch

[Tim Horton]

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?

[Megan Gardner]

Created attachment 398656 [details]
Patch

[Tim Horton]

Comment on attachment 398656 [details]
Patch

so much better

[Wenson Hsieh]

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.

[Megan Gardner]

Created attachment 398749 [details]
Patch for landing

[EWS]

Committed r261303: <https://trac.webkit.org/changeset/261303>

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