Enable context menus for mobile #698
Conversation
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 before the context menu listener could see it. Had to cancel the timer from the widget scroll methods instead.
|
Looks like the only failures are the expected PhantomJS screenshots, which no longer have the "Fill" overlay. |
|
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.
|
|
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? |
|
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. |
|
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. |
|
I just noticed 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. |
No change to the Spreadsheet context menu action external API, but internally now leveraging the core Vaadin context menu event handling from AbstractComponentConnector. This solves all iOS issues except the zoom coordinates issue, present in other Vaadin overlays like Menu and ComboBox.
|
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. |
deprecate, and convert to Enum based method for POI 4 Fix bug where visibility wasn't actually changed - old logic set visibility to exactly what it currently reports!
This reverts commit 33613da.
|
Nice. The iPhone I'm using was unavailable today, but I will try it tomorrow. |
|
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. |
editors or custom components. Note that the default "insert comment" logic used for the test UI doesn't think about possible custom components, and removes them when adding a comment. I left that alone, as it isn't part of enabling touch context menus.
|
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. |
|
The overlay now stays open, allowing me to insert the comment. But the jumping around continues and makes it frustrating to use. |
|
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. |
|
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. |
menus to display at the wrong coordinates
|
Found it. SpreadsheetConnector.showActions() was adding the scroll position twice to the popup coordinates. |
|
Yes, now it works. Thanks. |
|
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. |
There was a problem hiding this comment.
Reviewed 3 of 6 files at r1, 1 of 1 files at r4, 1 of 2 files at r5, 5 of 5 files at r7, 14 of 14 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @WoozyG)
vaadin-spreadsheet/src/test/java/com/vaadin/addon/spreadsheet/test/fixtures/ActionFixture.java, line 46 at r8 (raw file):
| NumberFormatException | NullPointerException e) { // ignore
Why are we ignoring this?
|
I updated the screenshots, let's see if it will pass now. |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tulioag)
vaadin-spreadsheet/src/test/java/com/vaadin/addon/spreadsheet/test/fixtures/ActionFixture.java, line 46 at r8 (raw file):
Previously, tulioag (Tulio Garcia) wrote…
Why are we ignoring this?
Because it threw errors when attempting to double a non-numeric or empty cell. For test purposes I figured doing nothing in those cases was fine - a user visible warning would be annoying and without much purpose, as a user could see the value they were trying to double wasn't a number.
|
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? |
|
I updated those screenshots and they were fixed as well. Now we have some tests in which the context menu is not being shown. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r9.
Reviewable status:complete! all files reviewed, all discussions resolved
Workaround found in the PhantomJS Git issue ariya/phantomjs#14005
|
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, pointing out that PhantomJS doesn't support Selenium's contextClick() |
|
Question: since these tests didn't have context menus showing up on PhantomJS before these changes, why are they looking for it now? |
|
I don't know, that is a good question. I still need to figure this out. |
|
|
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