Page MenuHomePhabricator

Prevent contenteditabe like experiences (cursor placement; menu selection for cutting text) in AddLinkArticleTarget
Closed, ResolvedPublic

Assigned To
Authored By
kostajh
May 3 2021, 9:07 PM
Referenced Files
F34462398: IMG_9742.PNG
May 21 2021, 1:01 AM
F34453257: addlink_VE.png
May 13 2021, 9:53 PM
F34453259: addlink_ceFalse.png
May 13 2021, 9:53 PM
F34451767: image.png
May 12 2021, 12:13 PM
F34450981: iosScroll_tap.webm
May 12 2021, 12:06 AM
F34450983: ios_tapscroll.png
May 12 2021, 12:06 AM
F34450980: iosScroll_js.webm
May 12 2021, 12:06 AM
F34444348: addlink_mobileTaps.webm
May 7 2021, 12:00 AM

Description

On iOS and Android, tapping on the text and/or pressing down on a link suggestion can bring up UX elements associated with contenteditable nodes, for example a cursor or a menu selection that says you could cut text.

image.png (1×584 px, 363 KB)

Event Timeline

There is possibly something off with how we are setting readOnly mode on the surface, or we perhaps need to toggle contenteditable to false on various nodes on the surface? With iOS Safari, setting contenteditable=false on the branchNode prevents the cursor / menu selection behavior, but also breaks tapping on the link suggestion.

kostajh triaged this task as Medium priority.May 3 2021, 9:09 PM
kostajh moved this task from Backlog to May 3 - May 7 on the Add-Link board.

readOnly mode keeps ce=true so that we can detect cursor placement (which it make invisible) and launch contexts.

Some options, which should probably be implemented upstream:

  • Set ce=false, and set the model selection based on click coordinates
    • Conceptually ce=false is not quite right. Although the surface is not editable, it is also not normal read HTML. In normal read HTML clicking a link would trigger it, whereas we want the edit tools to be triggered but in read mode.
  • Keep ce=true, and instantly de-activate the selection whenever it is changed (i.e. remove the native selection, but preserve the model selection).
    • Depending on event life-cycles, you make get a keyboard flicker when this happens. This probably wouldn't work very well on desktop with dragged selections, so should probably be limited to mobile.

Change 684880 had a related patch set uploaded (by Esanders; author: Esanders):

[VisualEditor/VisualEditor@master] Deactivate selection on mobile while in read-only mode

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

The above patch does indeed result in a flicker (on Android, haven't tested iOS), but I think this is neater than setting ce=true.

The above patch does indeed result in a flicker (on Android, haven't tested iOS), but I think this is neater than setting ce=true.

Thank you for looking into this! It looks weird on iOS with the AddLink plugin; if you tap on a selection or outside of a suggestion, the inspector shift upwards in a pretty distracting way.

kostajh renamed this task from Set contenteditable to false on AddLinkArticleTarget to Prevent contenteditabe like experiences (cursor placement; menu selection for cutting text) in AddLinkArticleTarget.May 4 2021, 8:17 PM
kostajh raised the priority of this task from Medium to High.
kostajh added a subscriber: mewoph.

@mewoph tentatively assigning to you; perhaps there is a way that our code can be reworked to play nicer with the proposed VE patch. Marking as high priority due to the impact this has on the mobile experience.

@kostajh I wanted to clarify to behavior you're referring to. Do you mean this effect when the link inspector shifts up as the article is scrolled when tapping anywhere on the surface?

addlink_newContextItem_scroll.gif (1×886 px, 1 MB)

For this particular issue, I don't think it's caused by RecommendedLinkToolbarDialog itself since the scroll behavior is also present with the old/default context item where the document is scrolled in response to a tap. It does seem a bit more jarring when there is a fixed position element at the bottom of the page.

addlink_defaultContextItem_scroll.gif (1×886 px, 2 MB)

@kostajh I wanted to clarify to behavior you're referring to. Do you mean this effect when the link inspector shifts up as the article is scrolled when tapping anywhere on the surface?

addlink_newContextItem_scroll.gif (1×886 px, 1 MB)

For this particular issue, I don't think it's caused by RecommendedLinkToolbarDialog itself since the scroll behavior is also present with the old/default context item where the document is scrolled in response to a tap. It does seem a bit more jarring when there is a fixed position element at the bottom of the page.

addlink_defaultContextItem_scroll.gif (1×886 px, 2 MB)

Yes that's the behavior I'm referring to. But it doesn't seem to be caused by the VE patch; it looks like that behavior is present with latest HEAD on GrowthExperiments + VisualEditor. So, I think we need to implement a fix for the weird scroll behavior in the AddLink plugin.

As an aside, even with the patch, on iOS Safari I can still press down on a link suggestion and see the "Cut" option in the menu selection which seems to indicate that the browser sees the surface as contenteditable. But this doesn't reliably occur either. In any case I think that's a much smaller issue than the scroll jump problem.

Change 685953 had a related patch set uploaded (by MewOphaswongse; author: MewOphaswongse):

[mediawiki/extensions/GrowthExperiments@master] [WIP] Add a link: Mobile UX improvements

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

Posted a WIP patch with some improvements:

  • Text cut/copy/select menu no longer shows up. The implication of this change is that the user can't copy text from the article while in machine suggestions mode.
  • The abrupt scroll behavior no longer occurs when tapping on the surface EXCEPT for the first tap before interacting with the link inspector.

Video w/latest changes:

It looks like setting inputMode='none' interferes somehow with OOUI's window opening/closing logic...perhaps it changes the focused element or scroll position? I did a quick test without the link inspector & with VE loaded in standard mode. It looks like this weird behavior only occurs when opening the dialog and tapping on a surface w/inputMode='none'.

To see this issue, inspect the page while VE is opened:

windowManager = new OO.ui.WindowManager();
$( document.body ).append( windowManager.$element );
windowManager.addWindows( [ new OO.ui.MessageDialog() ] );

$('.ve-ce-documentNode')[0].inputMode = 'none'
windowManager.openWindow( 'message' );
// Click ok/cancel & tap any where on the screen (abrupt scrolling occurs)

$('.ve-ce-documentNode')[0].inputMode = '''
windowManager.openWindow( 'message' );
//  Click ok/cancel & tap any where on the screen

Here's a summary of the changes I've tried and their issues.

Proposed ChangeKeyboard shownText edit menu shownWeird scrolling behaviorIssue
inputMode='none'NYYJarring scrolling
inputMode='none' with user-select: noneNNYJarring scrolling, user can't copy text
contenteditable=falseNNNThe user is editing the content, so contenteditable=false isn't semantically accurate. Some of the VE functionalities might break.

Thanks for digging into this @mewoph. I wonder if it might also be a listener to a 'mouseup' event, as tapping and holding doesn't seem to trigger the scroll.

I think we should go with setting contenteditable=false (probably just on mobile) on the branch node / root node.

Looking more closely, I think this weird scrolling behavior might not be caused by any JS calls to scroll methods since the kind of abrupt scroll that occurs during the flow looks different from the scroll that's invoked via JS. When scrubbing through the video of the tap-invoked scroll, I noticed that the link inspector gets pushed to the top (similar to before we set inputMode to none) while this doesn't occur w/the JS invoked scroll.

ios_tapscroll.png (1×760 px, 429 KB)

Here are videos comparing tap vs js invoked scrolls.

JS invoked scroll

Tap invoked scroll

Thanks for digging into this @mewoph. I wonder if it might also be a listener to a 'mouseup' event, as tapping and holding doesn't seem to trigger the scroll.

I think we should go with setting contenteditable=false (probably just on mobile) on the branch node / root node.

Another advantage to setting contenteditable=false is that browser plugins that interpret contenteditable=true won't get confused by our interface. For example, LanguageTool wants to markup the text with spelling / grammar fixes:

image.png (323×536 px, 73 KB)

Unfortunately I haven't found a good way to disable the keyboard jump during the first tap (with inputMode=none it looks like for the initial tap the keyboard is invoked but dismissed right away thus pushing the screen upwards). One possible improvement to this would be to set pointer-events: none on the surface (w/pointer-events: auto on the annotations) so that if the user happens to tap on the surface, the document isn't scrolled to top but the user would still see this weird behavior when the second recommendation is scrolled to. Pointer events would be reset to default upon user interaction w/the link inspector (otherwise we can't scroll to the selection).

For now, I'll submit another patch setting contenteditable=false. I've compared the diff between when the link is added manually and with machine suggestions mode with ce=false.

Default mode

addlink_VE.png (773×1 px, 406 KB)

Machine suggestions mode

addlink_ceFalse.png (968×1 px, 586 KB)

Change 692653 had a related patch set uploaded (by Kosta Harlan; author: MewOphaswongse):

[mediawiki/extensions/GrowthExperiments@wmf/1.37.0-wmf.6] Add a link: Set contentedtiable=false on mobile

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

Change 685953 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Add a link: Set contentedtiable=false on mobile

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

Change 692653 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@wmf/1.37.0-wmf.6] Add a link: Set contentedtiable=false on mobile

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

Mentioned in SAL (#wikimedia-operations) [2021-05-19T11:41:52Z] <mlitn@deploy1002> Synchronized php-1.37.0-wmf.6/extensions/GrowthExperiments/modules: Backport: [[gerrit:692653|Add a link: Set contentedtiable=false on mobile (T281771)]] (duration: 01m 06s)

Checked in wmf.6 - works fine. When an article has a template, the display will be a bit off (the links are not in the viewport), but it's scrollable, so it's no issue:

IMG_9742.PNG (1×640 px, 99 KB)

Change 684880 merged by jenkins-bot:

[VisualEditor/VisualEditor@master] Deactivate selection on mobile while in read-only mode

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

Change 693956 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (63e87da8e)

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

Change 693956 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (63e87da8e)

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