[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Page MenuHomePhabricator

Editing toolbar on mobile VE is not sticky (can scroll out of view) on iOS
Closed, ResolvedPublic

Description

Recently all of the scrolling hacks for iOS in MobileFrontend were removed (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/496827) because they were causing multiple different scrolling problems in several different interfaces.

The purpose of the hacks was to allow the editing toolbar (and other sticky headers in other overlays) to stay on the screen while the keyboard is open. Without them, it can be just scrolled away, because of iOS Safari's reckless disregard for position: fixed.

Here's a video demonstrating the problem (on iPhone SE running iOS 12.1.14, testing on https://en.m.wikipedia.beta.wmflabs.org/wiki/Division this morning). When the keyboard opens, the toolbar scrolls out of the viewport. If you want to scroll the toolbar back in, you have to scroll away from your selection. If you want to view the selection again, you have to scroll away from the toolbar. This makes any formatting changes very awkward.

This task is to figure out a new set of hacks to fix this. The new approach should only apply to the VisualEditor toolbar, and not any other sticky headers (see T218415 about that).

Related Objects

Event Timeline

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

Without them, it can be just scrolled away, because of iOS Safari's reckless disregard for position: fixed.

I made a diagram this morning trying to explain why this happens, I really liked it so here it is again:

viewports.png (2×6 px, 1 MB)

@iamjessklein and I met to discuss this earlier today. There are at least two different headers in the VE experience:

general headertext-tools header
IMG_2373.PNG (1×750 px, 94 KB)
IMG_2374.PNG (1×750 px, 130 KB)

General header - as long as this header always re-appears at the top of the screen upon closing the keyboard (as it currently seems to when in source-editing mode, though not in visual-editing mode), it is not necessarily an issue that it scrolls offscreen while the keyboard is active. There is even a perspective that it's beneficial to the user for it to disappear while the keyboard is open since there is such limited screen space at that time.

Text-tools header - it is important that this header always remains fixed to the top of the screen. When it scrolls out of view it becomes difficult to complete the various actions that it contains.

Potentially related bugs

We also discovered a number of bugs that seem to be related to the VE overlay and scrolling. We are unsure if some of these are already tracked elsewhere in phabricator, but it seems reasonable document them here.

Header not quite at top of viewport

Details: logged in, iOS/Safari, iPhone 6S
Sometimes, when opening the editor the general header is not quite at the top of the viewport. In this screenshot you can see the bottom of the Wikipedia app banner:

header not quite at top.PNG (1×750 px, 139 KB)

Header z-index issue

Details: logged in, iOS/Safari, iPhone 6S
Only when logged in, if I scroll down the page in edit mode (visual or source), after approximately one screen-length of scrolling, the header disappears underneath the page content:

IMG_2359.PNG (1×750 px, 156 KB)
IMG_2360.PNG (1×750 px, 155 KB)
IMG_2361.PNG (1×750 px, 155 KB)
IMG_2376.PNG (1×750 px, 45 KB)
IMG_2377.PNG (1×750 px, 32 KB)
IMG_2378.PNG (1×750 px, 24 KB)
Invisible content in source editor

Details: logged in, iOS/Safari, iPhone 6S
Only when logged in, if I scroll down the page in edit mode (source only), after after approximately one screen-length of scrolling the rest of the content is invisible. The content seems to be there, since I can place my cursor into the empty space, and there are also scrollbars:

scrolling to ~ the bottom of one screen-lengthfurther down the page
IMG_9BA89A0A19E8-1.jpeg (1×750 px, 79 KB)
IMG_53E4F6C7A004-1.jpeg (1×750 px, 174 KB)
Page scrolls up after action is taken on text

Details: logged in, iOS/Safari, iPhone 6S
If I highlight text, then bold it, the editor jumps up to a different part of the page

IMG_2368.PNG (1×750 px, 115 KB)
IMG_2369.PNG (1×750 px, 114 KB)
IMG_2370.PNG (1×750 px, 102 KB)

Header not quite at top of viewport

This looks like a problem with the iOS "open in our app" banner...

Header z-index issue

I'd hope this would be an easy one to solve. Want to cut open a bug?

I'll review Alex's comment later and file (or find) separate tasks.

As for the main topic of the toolbar scrolling out of view: I wrote a patch (https://gerrit.wikimedia.org/r/497453 https://gerrit.wikimedia.org/r/498256) to detect when the editor toolbar is scrolled out of view, and bring it back in, with a similar slide-in animation as when the editor loads. As far as I can tell, the behavior of Safari prevents us from really keeping it in view at all times (assuming we don't want to bring back the approach we ditched last week because it was causing so many other problems), and this is the best we can do (and it looks almost intentional). Video:


Change 497453 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] mobile.editor.ve: Bring the toolbar back into view after it scrolls out

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

Gerrit is down for maintenance and I'd like hear your thoughts about this today, so here's a patch the old-fashioned way:


(Remember to run 'npm run build')

@alexhollender Thanks for the review, my responses to the other issues:

In T218414#5033938, @alexhollender wrote:
  • Header not quite at top of viewport

    Sometimes, when opening the editor the general header is not quite at the top of the viewport. In this screenshot you can see the bottom of the Wikipedia app banner:

I don't see the banner at all on my phone, and I can't figure out how it's generated. Assuming this is a "Smart App Banner", then apparently I dismissed it, and I'd have to completely reset my (personal) phone to get it back, which I'd rather not do. Please file a task and hopefully someone with a loaner phone from the office can reset it and investigate :)

In T218414#5033938, @alexhollender wrote:
  • Header z-index issue
  • Invisible content in source editor

I tracked these two down to this rule in your personal styles at https://en.wikipedia.org/wiki/User:AHollender_(WMF)/minerva.css:

html, body {
	overflow-x: hidden;
	-webkit-overflow-scrolling: touch;
}

I'm not sure how exactly it causes it, but I can reproduce after adding this CSS and can't reproduce otherwise. I can only recommend removing it.

In T218414#5033938, @alexhollender wrote:
  • Page scrolls up after action is taken on text

I can reproduce this. Apparently just opening and closing the "Format" menu is enough to cause it (without changing styles). I filed it as T218650.

I don't see the banner at all on my phone, and I can't figure out how it's generated.

That's very simple, you need to install the iOS Wikipedia app. Any iOS app that has an app associated domain (universal link) will ALWAYS show this app banner on Safari for urls the app can handle, for as long as the app is installed. The banner is NOT dismissible (and it's a different banner from the promotional app banner which indeed is dismissible and requires resetting your browser to reappear).

When a user taps a universal link that you handle, iOS also examines the user’s recent choices to determine whether to open your app or your website. For example, a user who has tapped a universal link to open your app can later choose to open your website in Safari by tapping a breadcrumb button in the status bar. After the user makes this choice, iOS continues to open your website in Safari until the user chooses to open your app by tapping OPEN in the Smart App Banner on the webpage.

https://developer.apple.com/library/archive/documentation/General/Conceptual/AppSearch/UniversalLinks.html

8AB343B7-94DD-47B3-B447-264B788B328D.png (2×1 px, 1 MB)

Longpressing a link allows you to easily switch between app and safari

B8CC788F-F695-4DA7-8375-19D63C330BA3.png (2×1 px, 584 KB)

Thanks @TheDJ, I see the banner now. I don't see any problems with it though. You can get the banner into a position like in @alexhollender's screenshot, but then you can just scroll it away. And it even fires 'resize' events immediately, unlike when "scrolling away" the address bar (T218455), so it does not cause any visual issues, as far as I can tell. (It makes me wonder though, why is that not the case for the address bar, if we clearly have the technology…)

We talked about this for a few minutes in a meeting today and folks had some ideas for designs that work around the problem (inability to keep a toolbar sticky while scrolling with keyboard open), two that I remember:

  • Toolbar always disappears when scrolling, and appears again when you place a new selection – apparently Google Docs does that?
  • Keyboard closes when scrolling (by swapping to a fake selection), so the toolbar can stay sticky – see also T217797

I don't remember who proposed them, I know that @iamjessklein @DLynch @Esanders @dchan were talking.

For the record, the "native" iOS approach to toolbars like ours is either attaching them above the keyboard (seen e.g. in Apple Notes and Wikipedia app), which we can't do because we can't detect the size of the keyboard, or putting them in a popup next to the selection, which we can't do because Safari already displays such a popup there.

@Esanders asked what were the problems with the old approach, and if we can just bring it back but limit it to VE only.

We can bring it back if we decide that having a sticky toolbar is the only good design. I think it will be harder to maintain, but we can deal with it.

Here are the reasons I could come up with why I prefer ditching the old approach, and coming up with some design alternative for the sticky toolbar fail in the new approach:

  • We removed 300-400 lines of complicated special-case code for iOS to set up and synchronize a "viewport element"
  • No more funny stuff when the user manages to scroll the viewport rather than our "viewport element" (previously weird things happened if you tried to swipe to scroll, but started your touch on the toolbar)
  • Gives more screen space because the address bar can collapse (old approach forced it to always be expanded)
  • Better compatibility with built-in browser behaviors:
    • Allows Safari's native scroll-to-selection to work, when opening the keyboard and it would cover the selection (previously we had to fight against it to scroll the "viewport element" instead)
    • More natural behavior when trying to scroll past the beginning/end of the page (e.g. "bouncing" rather than no reaction; scrolling-related expand/collapse of iOS Safari address bar)
    • Compatible with tapping the browser top bar to scroll to the top
  • Fixes T217769: Excessive white space at the end of article/section on iOS
  • Fixes T217798: Selection lags behind scroll on iPhone

The drawbacks of the new approach are, as far as I know:


If we stick to the new approach, there are a few more bugs in it right now, but all of these are trivial to fix (mostly deleting more old code that is making wrong assumptions now) and already have patches:

Although this new solution isn't the longterm solution, I think that this patch should be merged as it makes the experience usable. My only caveat here is that we commit towards working on a sustainable solution that we are are satisfied with.

I think that this patch should be merged as it makes the experience usable.

Can you clarify what "this patch" is?

I think that this patch should be merged as it makes the experience usable.

Can you clarify what "this patch" is?

This one: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/497453. Description of the proposed behavior: T218414#5034848.

David and Ed proposed just constantly updating the toolbar's position, but I don't think that can work. Our code has no way to run before the newly scrolled page is rendered, as far as I know. This patch (https://gerrit.wikimedia.org/r/497833) demonstrates this approach, and here's how it looks like:

Change 497833 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] [DO NOT MERGE] mobile.editor.ve: Keep toolbar always in view

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

Change 497852 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] mobile.editor.ve: Keep toolbar always in view (restore old approach)

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

Patch https://gerrit.wikimedia.org/r/497852 https://gerrit.wikimedia.org/r/498257 demonstrates what we would have to do to bring back the old approach and make it only apply to VisualEditor, and none of the other mobile overlays. It includes my previously proposed refactor that should fix scrolling in dialogs.

Questions around this patch that have surfaced elsewhere...

What will be the scope of this patch?

This patch will be limited to the VisualEditor toolbar.

It should be noted that it has not yet been decided what approach we will take to resolve the issue this task is responding to. [1] With this said, the scope (VE only) will hold regardless. This thinking is also represented in this task's description. [2]

Where will the code for this to be decided patch live?

The code for the approach we move forward with will live in the VisualEditor repo.


  1. Issue: Editing toolbar on mobile VE is not sticky (can scroll out of view) on iOS
  1. Task description: "The new approach should only apply to the VisualEditor toolbar, and not any other sticky headers (see T218415 about that)."

cc @Jdlrobson @matmarex @Esanders @marcella @DLynch @ovasileva

Where will the code for this to be decided patch live?

The code for the approach we move forward with will live in the VisualEditor repo.

I will update my patches to move the code to the VisualEditor repo. I did not realize we prefer to have it in VisualEditor, so I submitted them for MobileFrontend since that's where most of iOS scrolling code has lived before.

...I did not realize we prefer to have it in VisualEditor, so I submitted them for MobileFrontend since that's where most of iOS scrolling code has lived before.

@matmarex, that sounds like a logical assumption considering the request to move the code to the VisualEditor repo was made after the patch was written.

20-March discussion...

Context

We talked over chat and during our planning meeting about which of the following interim patches to implement to restore mobile VisualEditing to a usable state:

  1. Deploy the vanishing-toolbars-while-scrolling patch proposed here
  1. Reinstate the sticky toolbar which had been live before T218414 emerged

We acknowledged it is important the solution we choose be able to be implemented "quickly" while minimizing the burden on engineering and design, (now and in the future).

Outcomes

Coming out of yesterday, we seem to have shared conviction that:

We are divided on whether:

In response to the above, @matmarex worked up:

  • A prototype of the vanishing-toolbars-while-scrolling solve so we can better assess its usability
  • A patch for reinstating the sticky toolbar so we can better asses the complexity involved with implementing and maintaining it

Next steps

In my mind, the key question we need to answer now is: "What – if anything - is contributing to our thinking that the vanishing-toolbars-while-scrolling creates an editing experience that is less usable than what was was live prior to T218414?

And I think we should be able to have an answer to the above by Tuesday, 26-March.

📲Vanishing-Toolbars-While-Scrolling-Prototype

Change 497453 abandoned by Bartosz Dziewoński:
mobile.editor.ve: Bring the toolbar back into view after it scrolls out

Reason:
To be moved to mediawiki/extensions/VisualEditor repo

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

Change 497833 abandoned by Bartosz Dziewoński:
[DO NOT MERGE] mobile.editor.ve: Keep toolbar always in view

Reason:
To be moved to mediawiki/extensions/VisualEditor repo

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

Change 497852 abandoned by Bartosz Dziewoński:
mobile.editor.ve: Keep toolbar always in view (restore old approach)

Reason:
To be moved to mediawiki/extensions/VisualEditor repo

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

21-March discussion...

Context

@Esanders, @matmarex, @iamjessklein and I met to assess the usability of the vanishing-toolbars-while-scrolling patch and associated 📲prototype.

And more specifically, to answer:

In my mind, the key question we need to answer now is: "What – if anything - is contributing to our thinking that the vanishing-toolbars-while-scrolling creates an editing experience that is less usable than what was was live prior to T218414?

Outcomes

Through today's conversation we agreed:

  • The patch, in its current state, is as usable as the experience that was live prior to T218414 emerging.
  • The patch, in its current state, should be deployed
  • In the future, we should revisit the patch's assumption that the toolbar should be displayed without the user explicitly "asking" for it to be. Ideas for what "explicitly asking" could mean in this context included, showing the toolbar:
    • When the user repositions the cursor
    • When the user swipes down after having swiped up
    • After the user's scroll ends and the cursor never leaves the screen

The above enhancements are now represented in a separate ticket: T218967

Next steps

  1. When are deploying this?
  2. How much time do we need for QA?
  3. How – if at all – do answers to the questions above impact T218939 and T218851?

Change 498256 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] ve.init.mw.MobileArticleTarget: Bring the toolbar back into view after it scrolls out

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

Change 498257 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] ve.init.mw.MobileArticleTarget: Keep toolbar always in view (restore old approach)

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

Related to this topic: it seems like we'd benefit (both on iOS and Android) from moving the "Publish" button in the Talk > Reply UI to the header. For Android users this would mean the button is always visible, and for both iOS and Android it would be a more consistent experience with other Publish/Save actions.

go to existing talk topictap into text field
Screenshot_20190322-122939.png (2×1 px, 183 KB)
Screenshot_20190322-122955.png (2×1 px, 181 KB)

Context

Next steps

  • When are deploying this?
  • How much time do we need for QA?
  • How – if at all – do answers to the questions above impact T218939 and T218851?

Outcomes

1. When are we deploying this patch?
This patch will be deployed on this upcoming Tuesday's, 26-March, train and live on all wikis on Thursday, 28-March.

2. How much time do we need for QA?
We will QA this patch as follows:

Today, 22-March
↳ Do pre-deployment QA on T218414 patch

Tuesday, 26-March
↳ QA T218414 patch in production

Wednesday, 27-March
↳ QA T218414 patch in production

3. How – if at all – do answers to the questions above impact T218939 and T218851?

T218939 The config change to "turn Section Editing on" will happen on Thursday, 28-March

T218851 The Section Editing A/B test will begin on Thursday, 28-March.

Started testing on prototype:

Noticing couple of things. Listing them below. @matmarex/@Esanders: let me know if I should file separate tasks for this as I am not sure if these are already considered/discussed somewhere:

  1. The general header stays visible when keyboard is open and scrolling up, but vanishes while scrolling down and re-appears when scrolling stops, is this expected?
  1. Tried to verify: T217798, but can't do this since the browser is crashing when trying to select a text when the keyboard is open (already reported: T219034), which seems to be how that bug was surfacing. I am however seeing if there is a selection on the surface and the keyboard is "closed", if you scroll down the selection appears on top of the toolbar. Should I file a separate task for this?
  1. After applying link to a text the page scrolls up a little bit, making the change that has just been made, out of view (Behind the keyboard).
  1. The general header stays visible when keyboard is open and scrolling up, but vanishes while scrolling down and re-appears when scrolling stops, is this expected?

Yes, this is expected for now. We're discussing some better ideas: T218967

  1. Tried to verify: T217798, but can't do this since the browser is crashing when trying to select a text when the keyboard is open (already reported: T219034), which seems to be how that bug was surfacing.

I commented, can't reproduce. Let's continue on that task.

I am however seeing if there is a selection on the surface and the keyboard is "closed", if you scroll down the selection appears on top of the toolbar. Should I file a separate task for this?

Known issue: T217797

  1. After applying link to a text the page scrolls up a little bit, making the change that has just been made, out of view (Behind the keyboard).

I don't think this is a new issue in this patch, something is wrong with the behavior of scrolling selection into view already on master. (It's not visible in production because scrolling selection into view was actually completely non-functional, T218635.) But I don't think we had a task, can you file one? Let's investigate this later.

I noticed an issue with the patch where if you scrolled almost to the end of the page/section with keyboard open, the toolbar would get stuck endlessly sliding in and out.

Fixed this in patchset 3 and just deployed the updated patch to the prototype site. It would be great if you could test particularly thoroughly when using the editor near the end of the page.

Esanders added a project: Editing QA.
Esanders moved this task from Inbox to High Priority on the Editing QA board.

Change 498256 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ve.init.mw.MobileArticleTarget: Bring the toolbar back into view after it scrolls out

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

In T218414#5048560, @alexhollender wrote:

Related to this topic: it seems like we'd benefit (both on iOS and Android) from moving the "Publish" button in the Talk > Reply UI to the header. For Android users this would mean the button is always visible, and for both iOS and Android it would be a more consistent experience with other Publish/Save actions.

Can you file this as a new task, and tag with MobileFrontend?

Change 498257 abandoned by Bartosz Dziewoński:
ve.init.mw.MobileArticleTarget: Keep toolbar always in view (restore old approach)

Reason:
We aren't going to use this for the time being.

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

In T218414#5048560, @alexhollender wrote:

Related to this topic: it seems like we'd benefit (both on iOS and Android) from moving the "Publish" button in the Talk > Reply UI to the header. For Android users this would mean the button is always visible, and for both iOS and Android it would be a more consistent experience with other Publish/Save actions.

Can you file this as a new task, and tag with MobileFrontend?

T219210

  1. After applying link to a text the page scrolls up a little bit, making the change that has just been made, out of view (Behind the keyboard).

I don't think this is a new issue in this patch, something is wrong with the behavior of scrolling selection into view already on master. (It's not visible in production because scrolling selection into view was actually completely non-functional, T218635.) But I don't think we had a task, can you file one? Let's investigate this later.

I think I am noticing this same issue (T218635, btw it seems this is not fixed yet on Beta cluster), sharing the screen capture here for your better understanding, I will file a task if it seems to you a separate issue:

Also, notice that it keeps scrolling the page up constantly as I try to scroll down to the linked text. Is it known or tracked somewhere? Should I file a task for this?

Hey @Ryasmeen we discussed this in Triage and determined we are going to test in production. So we would need you to test to prioritize a Post-Deployment test.

@Esanders @matmarex should this be in Product Owner Review now? I believe it is already on the train?

CC: @ppelberg and @marcella

@JTannerWMF: Now it can go to Product Owner Review :) It just landed on en.wiki earlier today with wmf.23 deployment. And I checked all the relevant tasks associated with this, and added comment where appropriate.
Marking it done from QA side.