Enable context menus for mobile

[WoozyG]

Remove mobile "Fill" overlay, implement context menu for all.

Everything but iOS gets the simple ContextMenuEvent handler.

iOS, which doesn't support that, needs a timer to simulate long-press like Android does natively.

Most of the new code is canceling the timer when a touch is started but then the user does something else, meaning they don't want the context menu after all.

Scrolling was the big problem, as it cancelled the scroll event before the context menu listener could see it. Had to cancel the timer from the widget scroll methods instead.

This will change nearly all the PhantomJS reference images, as apparently that browser was rendering the "Fill" overlay like a touch device.

---

This change is 

[WoozyG]

Looks like the only failures are the expected PhantomJS screenshots, which no longer have the "Fill" overlay.

[WoozyG]

I don't have access to an iPhone at them moment (only one around is my wife's, which is hard to pry out of her hands, even if she were home).

Tested on an iPad Air 2, though, the only i-device I have ready access to. The problem appears to be only when zoomed in. When zoomed out as far as I can, everything is fine. Zooming in at all breaks things in ways you saw. This appears to be an issue not with these changes specifically, but with SpreadsheetConnector.showActions() and how it calculates screen position for the popup/overlay menu. Is that what you saw, or is it something else on an iPhone as opposed to an iPad? If it has to do with zoom and coordinate calculation, that should be fixed with this, I agree, but is not due to these changes. I'll keep investigating.

We have seen other buggy behavior with Spreadsheet and scrolling on iPad Pros, but only those. our suspicion is that the higher pixel density is messing with something in the Vaadin drag-scroll logic on those.

Tested on Safari 12, on an iPhone. Sometimes the context menu just flashes. Other times it opens far from where the touch event happened.

[WoozyG]

I can reproduce the overlay display issue with core Vaadin, so it isn't a problem with the context menu changes themselves. To reproduce:

Go to the sample Charts app

Click the Theme selection combo box in the top right corner. Notice the combo box list displays properly below the current value, as normal.

Close the ComboBox

pinch-zoom even a single step on an iOS device

Expand the combo box again - it will display to the left of where it should. I think this is the same core overlay issue I'm seeing testing context menus with zoom here.

Are you still seeing something different on your iPhone? Problems even when zoomed all the way out?

[tulioag]

Nice catch in Vaadin charts demo. I was able to reproduce it on the iPhone. The difference is that the overlay stays open, even though it opens in the wrong place.

For this PR, I could reproduce the issue even without zooming. The "Insert comment" disappears as soon as I remove my finger from the screen.

[WoozyG]

This is fascinating as a non-Apple dev. Behavior in versions reported as identical (iOS 12.1.4, WebKit 605.1.15) on an iPad Air 2 and an iPhone 5s differs. The iPhone 5s I got running shows exactly the behavior you recorded above. The iPad Air 2 does not. Somehow their event models are different. As I said earlier, we've noticed scrolling in Spreadsheet barely works on an iPad Pro, and now this phone/tablet difference. I have no idea what is going on yet, or whether it is purely a pixel density difference that changes timings somehow, or something core to the OS that treats devices with different screen sizes differently, or something else entirely. I'll poke blindly in the dark a bit and see what comes up.

[WoozyG]

I just noticed AbstractComponentConnector handles contextClick events, including touch event timer implementation on iOS. That's what Spreadsheet should use, similar to how Grid extends the server-side event to include extra properties.

I'll see if I can work up a more standard implementation using the existing connector code. Might be tricky to rip out the existing stuff, or maybe not.

This would replace the existing triggers, but hopefully still use the same action/ContextMenuManager interface for backward compatibility.

[WoozyG]

Try this latest re-write, using the standard Connector context click code instead of custom code (that did nearly the same thing). This version behaves much better for me on an iPhone, without breaking anything I can see on other devices.

[tulioag]

Nice. The iPhone I'm using was unavailable today, but I will try it tomorrow.

[WoozyG]

Looks like context menus aren't firing on cells with custom components displayed, or custom editors with focus. That would be a regression or change in behavior. I'm investigating. Don't know if that would be a problem or just a documentation change for folks.

If the custom editor is not displayed, the context menu displays. If the cell has a custom component always displayed, it won't use the context menu or allow the event to bubble for some reason. That sounds to me like something in AbstractComponentConnector, where the context behavior is defined.

[WoozyG]

Fixed some logic and added missing case. Sheet context menus now display when executing a context click on a cell's custom editor or custom component.

As far as I can tell this is now feature complete and ready for final testing and updated test screenshots.

[tulioag]

The overlay now stays open, allowing me to insert the comment. But the jumping around continues and makes it frustrating to use.

[WoozyG]

The jumping is a core issue, and only happens for me at zoom. It doesn't appear to be part of the context menu logic, but rather with Spreadsheet's calculation for mouse position. It doesn't seem to take viewport offsets into account. This just happens to be the most noticeable place it causes trouble. I can try to find it, but it isn't directly part of this code. The reason it wasn't noticed before was context menus didn't show up on mobile devices where zooming happened.

[tulioag]

I tried updating to the latest framework version, but it didn't fix the issue. If you can find it, great. Anyway I will open an issue in the framework.

[WoozyG]

Found it. SpreadsheetConnector.showActions() was adding the scroll position twice to the popup coordinates.

[tulioag]

Yes, now it works. Thanks.

[WoozyG]

Where are we at on this? As far as I know it is working, and needs updated integration test screenshots to reflect the changed behavior. That's not something I can get right in my setup, I don't think.

I'm ready to move with other changes, as I just pushed POI 4.1.0. Should show up on Maven Central tomorrow.

[tulioag]

I updated the screenshots, let's see if it will pass now.

[WoozyG]

I see 2 screenshots that didn't pass (didn't look at them yet) and some other new failures. Were these possibly there but hidden/not reached because of prior screenshot failures? Or something else?

[tulioag]

I updated those screenshots and they were fixed as well. Now we have some tests in which the context menu is not being shown.

[WoozyG]

Not sure what is going on now. My changes made the failing tests pass for me (PhantomJS but on Windows). See the PhantomJS issue I mentioned in my commit,

https://github.com/ariya/phantomjs/issues/14005

pointing out that PhantomJS doesn't support Selenium's contextClick()

[WoozyG]

Question: since these tests didn't have context menus showing up on PhantomJS before these changes, why are they looking for it now?

[tulioag]

I don't know, that is a good question. I still need to figure this out.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

:x: WoozyG
:x: tulioag
_{You have signed the CLA already but the status is still pending? Let us recheck it.}