Page MenuHomePhabricator

Help panel: textarea not visible when editing in VE mode on mobile
Closed, ResolvedPublic

Description

Steps to reproduce (this occurs on iOS, not on Android)

  1. Go to beta labs and log in
  2. Make sure you have "Enable editor help panel" set in preferences
  3. Login on an iOS device
  4. Make sure you are in visual edit mode
  5. Open the help panel
  6. Tap into the textarea. It will be covered up by the keyboard
  7. Switch to source edit mode, open the help panel, tap in the textarea, all looks fine

See also the short video below:

Note that the video is taken on a local site with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/482822 applied; the behavior without this patch is even more broken as iOS Safari thinks there two text inputs (the VE surface, and the process dialog text area). The patch partially fixes the problem by detaching the VE overlay, but the textarea not coming into view is still a problem.

Related Objects

Event Timeline

kostajh created this task.Jan 4 2019, 7:40 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 4 2019, 7:40 PM
MMiller_WMF renamed this task from Help panel textarea not visible when editing in VE mode on mobile to Help panel: textarea not visible when editing in VE mode on mobile.Jan 4 2019, 9:39 PM
MMiller_WMF added subscribers: SBisson, Catrope, Etonkovidova and 6 others.
kostajh updated the task description. (Show Details)Jan 7 2019, 5:41 PM

on iOS, it looks like the browser thinks that there are two text areas to fill in. Note the cursor. It lets me type into the VE surface even though the text is hidden until I close the help panel.

Volker_E updated the task description. (Show Details)Jan 7 2019, 5:56 PM
kostajh assigned this task to Catrope.Jan 7 2019, 7:31 PM

Change 482822 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/extensions/GrowthExperiments@master] (WIP) Help Panel: Fix textarea focus when VE is activated

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

kostajh updated the task description. (Show Details)Jan 8 2019, 9:40 PM
kostajh updated the task description. (Show Details)Jan 8 2019, 9:50 PM
kostajh updated the task description. (Show Details)
kostajh updated the task description. (Show Details)Jan 8 2019, 10:21 PM
kostajh updated the task description. (Show Details)
kostajh updated the task description. (Show Details)Jan 8 2019, 10:23 PM
kostajh updated the task description. (Show Details)Jan 8 2019, 10:37 PM

We debugged this today and it seems to relate to the following line introduced in T108576 :

this.useScrollContainer = ve.init.platform.constructor.static.isIos();

@kostajh was on #mediawiki-visualeditor today and correctly identified that this is the same kind of issue as T210559. It's caused by some funky things we do to work around iOS Safari's scrolling behavior that would cause our toolbar to scroll out of the viewport when opening the keyboard.

In https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/478893/3/resources/mobile.editor.ve/ve.init.mw.MobileFrontendArticleTarget.js, the fix was to just disable the funky things while a dialog is open; but VE has no way to know that GrowthExperiments (or any other extension) has opened a dialog of its own.

One silly way to fix this would be to override ve.init.mw.MobileFrontendArticleTarget.prototype.onWindowScroll so that it does nothing if your dialog is open (and call the original function if it isn't).

Alternatively, you might want to integrate your code with VE so that it knows about your dialog and manages opening/closing it. Roughly like this:

function BlahBlahDialog () {
	BlahBlahDialog.parent.call( this, arguments );
}
OO.inheritClass( BlahBlahDialog, OO.ui.ProcessDialog );
BlahBlahDialog.static.name = 'blahBlahDialog';

mw.loader.using( 'ext.visualEditor.desktopArticleTarget.init', function () {
	mw.libs.ve.addPlugin( function ( target ) {
		ve.ui.windowFactory.register( BlahBlahDialog );
	} );
} );

// Some time later, to open the dialog:
ve.init.target.getSurface().execute( 'window', 'open', 'blahBlahDialog' );


on iOS, it looks like the browser thinks that there are two text areas to fill in. Note the cursor. It lets me type into the VE surface even though the text is hidden until I close the help panel.

This is funny. On desktop, we prevent these kinds of issues by having additional invisible focusable elements at the beginning/end of the dialog, and if the focus moves to them (presumably as a result of user tabbing through a form), we return it to the first/last real focusable element in the dialog. I don't know how iOS's focus switching works, but clearly it ignores them. But maybe we could make it work by just changing the focusable elements from <div> to <input>? The relevant code is in OOUI Window.js (search for 'focusTrap').

Alternatively, you might want to integrate your code with VE so that it knows about your dialog and manages opening/closing it. Roughly like this:

function BlahBlahDialog () {
	BlahBlahDialog.parent.call( this, arguments );
}
OO.inheritClass( BlahBlahDialog, OO.ui.ProcessDialog );
BlahBlahDialog.static.name = 'blahBlahDialog';
mw.loader.using( 'ext.visualEditor.desktopArticleTarget.init', function () {
	mw.libs.ve.addPlugin( function ( target ) {
		ve.ui.windowFactory.register( BlahBlahDialog );
	} );
} );
// Some time later, to open the dialog:
ve.init.target.getSurface().execute( 'window', 'open', 'blahBlahDialog' );

I think this is the best way forward (if we can solve other issues, like the fact that we need a separate WindowManager+window to show this dialog in non-VE editors), but do you know how to do the addPlugin thing on mobile? Cause your example uses desktopArticleTarget :)

My guess is that it would be something like this?

mw.loader.using( 'ext.visualEditor.targetLoader', function () {
    mw.libs.ve.targetLoader.addPlugin( function () {
        ...
    } );
} );

Change 482822 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Help Panel: Fix textarea focus on mobile when VE is activated

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

I think this is the best way forward (if we can solve other issues, like the fact that we need a separate WindowManager+window to show this dialog in non-VE editors), but do you know how to do the addPlugin thing on mobile? Cause your example uses desktopArticleTarget :)

Sorry, that was a bit of mindless copy-paste from me. You probably want to add to VisualEditorPluginModules in extension registration, instead of manually loading your module. Look at the plugins in e.g. AbuseFilter or TitleBlacklist for an example. (Also, this made me realize they are not loaded on mobile, even though they were meant to be. I filed and fixed T213774 about that.)

Etonkovidova closed this task as Resolved.Jan 15 2019, 5:15 PM

Focusing on the text area working correctly now - for both VE and wikitext editor.

Jdlrobson added a subscriber: alexhollender.EditedJan 15 2019, 5:24 PM

I had a report from @alexhollender yesterday that a similar problem was impacting the mobile talk overlay (accessed via the talk button at the bottom of the page for logged in users). Can you confirm that's also fixed @Etonkovidova ?

Jdlrobson reopened this task as Open.Jan 15 2019, 5:25 PM

Reopening pending that confirmation!

@Jdlrobson the fix implemented here is very narrow, in that it is targeted to the help panel in GrowthExperiments extension.

I don't see this bug with the mobile talk overlay on Safari iOS but I do see the unwanted zoom documented in T212505, which should be pretty straightforward to adjust.

Etonkovidova added a comment.EditedJan 15 2019, 5:41 PM

Re-checked and the comment was corrected.
@Jdlrobson there are some problems with focusing correctly on the area where the text is supposed to be entered. But the page is scrollable, so it's not that severe.

If the bug was not introduced by the patches, then, I agree with @kostajh - it is a separate issue.

Here is a video of the bugs I was experiencing on iOS yesterday: https://drive.google.com/file/d/1MGmryRAoUdnRucfDBJT0KjyXpjbioLTA/view?usp=sharing. The same issues don't seem to occur on Android.

To reproduce:

  • open the Talk overlay
  • open a discussion
  • scroll to the bottom of the page
  • try to continue scrolling further down — at this point one of several things seems to happen:
    1. the page freezes and I can't scroll back up
    2. the page header disappears
    3. a gray border/line appears, and is fixed towards the bottom of the screen
  • tap into the text field towards the bottom of the page
  • type something
  • tap "Done" to dismiss the keyboard
  • zoom out
  • some of the bugs above may occur now if they haven't already
Jdlrobson raised the priority of this task from Normal to High.Jan 15 2019, 9:41 PM
Jdlrobson added a project: Readers-Web-Backlog.
Jdlrobson added a subscriber: Esanders.

I think we should fix this at the source and revert https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/GrowthExperiments/+/482822 given all the parts of the UI this is impacting.

@Esanders I'd be keen to revert or amend the patch (93c6a47661) that introduced:

this.useScrollContainer = ve.init.platform.constructor.static.isIos();

I don't completely understand this code enough to know what an amendment that limits this to the VisualEditorOverlay would look like.
Can you please advise?

@Jdlrobson note that this bug is really two issues in one:

  1. The textarea won't stay in view, caused by the use of this.useScrollContainer
  2. The phantom textarea inputs behind the process dialog, which exist unless the editor-overlay is detached (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/482822/8/modules/help/ext.growthExperiments.HelpPanel.cta.js#106) and then appended.

Although the title refers to a specific issue with Help panel that was Resolved, the task has some insights on more general fixes that need to be added. Removing Growth team project related tags.

Change 487552 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] ve.init.mw.MobileFrontendArticleTarget: Disable iOS scrolling hack more aggressively

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

I think we should fix this at the source and revert https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/GrowthExperiments/+/482822 given all the parts of the UI this is impacting.
@Esanders I'd be keen to revert or amend the patch (93c6a47661) that introduced:

this.useScrollContainer = ve.init.platform.constructor.static.isIos();

I don't completely understand this code enough to know what an amendment that limits this to the VisualEditorOverlay would look like.
Can you please advise?

I don't think this is feasible. Doing so would prevent us from having a fixed header with the toolbar.

Instead, I think https://gerrit.wikimedia.org/r/487552 should be enough to limit our scrolling hack to only kick in when the editing surface really is active.

Assuming this patch is merged, I think you can remove the code in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/482822/8/modules/help/ext.growthExperiments.HelpPanel.cta.js that deals with the scrolling hack.

matmarex closed this task as Resolved.Feb 8 2019, 1:43 PM

Regardless, the issue reported here is fixed.

I agree with @Etonkovidova and @kostajh that the bug reported by @alexhollender is a separate issue:

Here is a video of the bugs I was experiencing on iOS yesterday: https://drive.google.com/file/d/1MGmryRAoUdnRucfDBJT0KjyXpjbioLTA/view?usp=sharing. The same issues don't seem to occur on Android.

For convenience and posterity, copy of the video:

Note that a) VisualEditor is not open in the background (while this bug only occurred with VE) b) the issue occurs even before the keyboard is opened (while this bug only occurred with keyboard open).

@alexhollender Would you mind filing a separate task for this?

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptFeb 8 2019, 2:39 PM

Change 489249 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/extensions/GrowthExperiments@master] Help Panel: Remove VE hack workaround

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

Change 487552 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] ve.init.mw.MobileFrontendArticleTarget: Disable iOS scrolling hack more aggressively

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

Change 489249 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Help Panel: Remove VE hack workaround

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