Page MenuHomePhabricator

Mobile native selection context menu can obscure the VE toolbar
Closed, ResolvedPublic

Description

Steps to reproduce:

  1. On Android Chrome, edit an empty VE Standalone page (e.g. go to https://rawgit.com/wikimedia/VisualEditor/master/demos/ve/mobile.html#!pages/empty.html ).
  2. Type less than one line of text.
  3. Select some of the text.

Expected behaviour: the selection context menu does not obscure the VE toolbar.

Actual behaviour: the selection context menu does obscure the VE toolbar.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Scrolling an additional 60px on Android seems to work well (at least on my device). However on iOS, and scroll results in the copy/paste context menu being hidden, and there is no way to get it back programmatically.

This leaves us the following options for iOS:

  • Don't scroll at all: This requires the user to scroll the context menu out of the way of the toolbar, which is a bit fiddly.
  • Scroll 60px: The user can re-trigger the copy/paste menu by touching the selection. This is not obvious but is probably the first thing they will try.

I think we should stick with the latter (which is the current state of the patch). I would imagine users select text to use tools (e.g. link), more often than they select to copy/paste on mobile, especially as copy/paste on mobile is so fiddly.

Actually the iOS bug is slightly different, but the effect is the same. If you programatically scroll the surface with the context open nothing moves with it, even the cursor:

The fix I wrote for this 3 years ago (https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/230093/) was to reapply the selection after scrolling. This results in the selection being drawn in the correct place, but the context is not re-shown.

Either way, once the context has been rendered, we can either leave it where it is or hide it, but we can't scroll then re-show it.

dchan added a comment.Sep 5 2018, 2:09 PM

Given the context disappearance is an iOS-specific issue, would it be worth doing the 60px scroll on non-iOS devices only?

It depends if you want a context that obscures the a toolbar, or a context that you sometimes need to tap again to show if we force scroll.

@dchan I've separated the patch into Android/iOS versions. The Android part works fine and should be fairly uncontroversial.

Change 460331 had a related patch set uploaded (by Esanders; owner: Esanders):
[VisualEditor/VisualEditor@master] Scroll 60px past toolbar on mobile to avoid context menu clash on iOS

https://gerrit.wikimedia.org/r/460331

Change 457883 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Scroll 60px past toolbar on mobile to avoid context menu clash on Android

https://gerrit.wikimedia.org/r/457883

Change 473267 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (f391832c5)

https://gerrit.wikimedia.org/r/473267

Change 473267 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (f391832c5)

https://gerrit.wikimedia.org/r/473267

JTannerWMF moved this task from Inbox to Low Priority on the Editing QA board.Nov 14 2018, 4:23 PM

@dchan/@Esanders: The android test device that I have is showing the selection context menu on top of the page, so I can't really verify this. Is there a way to enable the floating native selection context menu on android?

@Ryasmeen can you provide a screenshot? Also details of the Android OS used.

Ryasmeen added a comment.EditedJan 9 2019, 10:38 PM

@Esanders: Sure! I was using Android KitKat on Samsung GT-1905G. I have upgraded the OS to the latest version but still it's appearing the same way for me. Here is the screenshot:

Esanders added a comment.EditedJan 17 2019, 1:55 PM

It looks like this floating context became standard around Android 6 or 7. Your test device (Android 4.4) is the oldest we support.

I can scope this fix to versions of Android >=6, and we can consider if we need a different fix for older versions.

Nexus (Stock Android)
Nexus 5 (4.4)Nexus 6 (5.0)Nexus 6P (6.0/7.0)
Samsung
Galaxy S5 (4.4)Galaxy S6 (5.0)Galaxy S6 (7.0)

Android usage:
https://analytics.wikimedia.org/dashboards/browsers/#all-sites-by-os

we can consider if we need a different fix for older versions.

It appears that in older versions of Android that use the top bar context, placing the cursor forces the address bar to be show, then making a selection places the context where the address bar would be. As a result there is no clash between the copy/paste context and VE UI components:

Change 485032 had a related patch set uploaded (by Esanders; owner: Esanders):
[VisualEditor/VisualEditor@master] Only fix context menu position when Android >= 6

https://gerrit.wikimedia.org/r/485032

Change 485032 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Only fix context menu position when Android >= 6

https://gerrit.wikimedia.org/r/485032

Change 485866 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (5883578a8)

https://gerrit.wikimedia.org/r/485866

Change 485866 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (5883578a8)

https://gerrit.wikimedia.org/r/485866

matmarex added a subscriber: matmarex.

The last pending patch is https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/460331.

I don't understand this patch. If scrolling hides the context menu, then we shouldn't have to scroll past the context menu?

I don't understand this patch. If scrolling hides the context menu, then we shouldn't have to scroll past the context menu?

It means that if the user tries to create the selection again (because they did want the context menu) it doesn't clash with the toolbar. We could just scroll by 1px but I think that would be less useful.

Change 460331 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Scroll 60px past toolbar on iOS too to avoid context menu clash

https://gerrit.wikimedia.org/r/460331

Change 496473 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (8bb1eb598)

https://gerrit.wikimedia.org/r/496473

Change 496473 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (a61616907)

https://gerrit.wikimedia.org/r/496473

@Esanders: So I checked this today on @Catrope's Android 9, Moto X4 device. It seems the context menu is still obscuring the VE toolbar. Am I looking at the right thing here?

Esanders added a comment.EditedApr 8 2019, 6:11 PM

@Ryasmeen can you test this in MediaWiki? This fix only works with a floating toolbar. We are not too bothered about the standalone demo as that doesn't represent a real-world use case at the moment.

@Esanders: I just checked this on Mediawiki, seems to be happening there too when the text is closer to the toolbar. Here is the screenshot:

marcella raised the priority of this task from Medium to High.Apr 29 2019, 1:53 PM
JTannerWMF removed a project: Editing QA.
JTannerWMF added a subscriber: JTannerWMF.

@Esanders putting this back into In Progress to address what @Ryasmeen raised

@Ryasmeen it looks like there isn't enough content to scroll down. Can you confirm this works on longer pages?

JTannerWMF reassigned this task from Esanders to Ryasmeen.Jun 12 2019, 2:50 PM
JTannerWMF added a project: Editing QA.
JTannerWMF moved this task from Low Priority to High Priority on the Editing QA board.

Assigning to Rummana to get a response

@Esanders: Checked on a long article, still behaves the same.

Is this still only on standalone, or does it affect MediaWiki as well?

@Esanders: I checked it on en.wiki

matmarex reassigned this task from Ryasmeen to Esanders.Jun 14 2019, 9:43 PM
matmarex moved this task from QA to In progress on the VisualEditor (Current work) board.
matmarex removed a project: Editing QA.

Change 517459 had a related patch set uploaded (by Esanders; owner: Esanders):
[VisualEditor/VisualEditor@master] scrollSelectionIntoView: Fix mobile logic

https://gerrit.wikimedia.org/r/517459

Change 517459 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] scrollSelectionIntoView: Fix mobile logic

https://gerrit.wikimedia.org/r/517459

Change 517570 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (f53da0fd6)

https://gerrit.wikimedia.org/r/517570

@Esanders The submodule update patch is failing due to some mysterious problems in ContentTranslation, can you have a look at it?

As I was testing the new patch, I noticed another issue on iOS. When VE scrolls the page to make space for the native context menu below the toolbar, the native context menu itself remains in place – it neither scrolls with the selection, as it ideally would, nor does it disappear, as it apparently used to do a few months ago (T202723#4983015).

Initial viewAfter double-tapping to select, and VE-initiated scroll

I wonder if this is somehow caused by a change in VE, or a change in iOS? I upgraded a few weeks ago and I'm now seeing this on iOS 12.3 (on iPhone SE).

Change 517570 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (1cf71b3f3)

https://gerrit.wikimedia.org/r/517570

@Ryasmeen Ed's fix for the Android issue should be available for testing on Beta soon (I'm not sure how long it takes to update itself).

@Esanders: So I checked it today after the fix, it seems to be working when you select a text anywhere in the document except the first line which I understand is tricky to handle since there is no more content at the top, there is just no place to scroll up to to make space for the context menu. So, assuming this is a special case, I am fine closing this ticket unless we want to handle that now.

there is just no place to scroll up to to

Yeah - we probably don't want to introduce the complexity of top padding just for this edge case.

Esanders added a comment.EditedJun 24 2019, 3:22 PM

As I was testing the new patch, I noticed another issue on iOS. When VE scrolls the page to make space for the native context menu below the toolbar, the native context menu itself remains in place – it neither scrolls with the selection, as it ideally would, nor does it disappear, as it apparently used to do a few months ago (T202723#4983015).

Initial viewAfter double-tapping to select, and VE-initiated scroll

I wonder if this is somehow caused by a change in VE, or a change in iOS? I upgraded a few weeks ago and I'm now seeing this on iOS 12.3 (on iPhone SE).

Looks like an iOS change. The behaviour before was that the context menu disappeared on iOS if you did a programmatic scroll (inline comment; "Note that scrolling on iOS closes the context, but it will re-open when the user touches the selection."). We did introduce a setTimeout to the scroll, but even if I turn that off, the context menu doesn't move with the selection.

The only way to hide the context menu now would be to re-apply the selection, but that would hide the selection handles.

Change 518747 had a related patch set uploaded (by Esanders; owner: Esanders):
[VisualEditor/VisualEditor@master] Remove iOS context scroll fix

https://gerrit.wikimedia.org/r/518747

Change 518747 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Remove iOS context scroll fix

https://gerrit.wikimedia.org/r/518747

Change 519091 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (39b766b3a)

https://gerrit.wikimedia.org/r/519091

Change 519091 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (39b766b3a)

https://gerrit.wikimedia.org/r/519091

matmarex moved this task from QA to Product owner review on the VisualEditor (Current work) board.

Actually, I think this was already QA'd on the prototype server:

@Esanders: So I checked it today after the fix, it seems to be working when you select a text anywhere in the document except the first line which I understand is tricky to handle since there is no more content at the top, there is just no place to scroll up to to make space for the context menu. So, assuming this is a special case, I am fine closing this ticket unless we want to handle that now.

ppelberg closed this task as Resolved.Aug 7 2019, 12:15 AM
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptAug 7 2019, 12:15 AM