Page MenuHomePhabricator

Regression: Editing workflow broken for non-JS users and template editors on mobile
Closed, ResolvedPublic

Description

When I click edit button on any template namespace page, I am supposed to be taken to the full page text editor. This is supposed to be true for any page where the MobileFormatter is not ran (https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/README.md#wgmfmobileformatternamespaceblacklist)

When I go to https://en.m.wikipedia.beta.wmflabs.org/w/index.php?title=Module:T173949, for example, I see this screen:

On production, this workflow is much better - clicking edit takes me to the non-js editor:

To edit, I now must click "reload the page."
This means editing now takes 2 clicks and if editing on mobile, the 2nd click forces me out of the mobile view into the desktop site.

Non-js editor is broken

Although the editor loads successfully when you have JavaScript disabled or are using a grade C browser, completing an edit is impossible. When clicking publish changes, the user is redirected back to the editor without the editor being completed.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 20 2018, 10:11 PM

I wonder if https://phabricator.wikimedia.org/T197726 could be somehow also related (non-main namespace; mobile). Probably not.

Jdlrobson triaged this task as High priority.Jun 20 2018, 10:36 PM
Jdlrobson added projects: Mobile, MobileFrontend.
Jdlrobson updated the task description. (Show Details)
Jdlrobson renamed this task from Regression: Editing workflow broken for non-main namespace on mobile to Regression: Editing workflow broken for template namespace on mobile.Jun 20 2018, 10:43 PM
Jdlrobson lowered the priority of this task from High to Normal.EditedJun 20 2018, 10:49 PM

Looks like this relates to https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/439968/
Per T172948 the editor for templates should always be the fallback editor so I'm not sure what's going on here.

What's weird, is that if I have JS disabled or a grade C browser, this workflow although not ideal (takes 2 http requests rather than 1) does redirect to the right place. I'm guessing something in the VE logic is not kicking in?

On desktop I see the same problem but it says "Your skin is incompatible with VisualEditor. See https://www.mediawiki.org/wiki/VisualEditor/Skin_requirements for the requirements." This warning does not show up on mobile.

Could we load this code on mobile too and instead of printing the warning automatically redirect to mobileeditorsuppress=1 ?

Given the editor is accessible bumping down to normal, but it's not a great experience compared to how it was before, especially given this might hinder editing on a slow connection where the fallback editor is preferable.

Jdlrobson renamed this task from Regression: Editing workflow broken for template namespace on mobile to Regression: Editing workflow shows "The editor will now load" screen which doesn't reload for editing pages in template namespace on mobile.Jun 20 2018, 10:55 PM
Jdlrobson renamed this task from Regression: Editing workflow shows "The editor will now load" screen which doesn't reload for editing pages in template namespace on mobile to Regression: Editing workflow shows "The editor will now load" screen which doesn't reload for editing pages in template namespace on Minerva mobile and desktop.
Jdlrobson renamed this task from Regression: Editing workflow shows "The editor will now load" screen which doesn't reload for editing pages in template namespace on Minerva mobile and desktop to Regression: Editing workflow broken for non-JS users and template editors.Jun 20 2018, 11:01 PM
Jdlrobson raised the priority of this task from Normal to High.

Sorry for all the spam. Actually this is a little worse than I first thought.
Saving doesn't actually work. When I click save "publish changes" I get this screen, thus it's impossible to edit on non-JS (grade C) mode as well right now:

Unbreak now...?

It's not unbreak-now, I suppose, but it's unbreak-before-Tuesday, which is when this will start going out to production wikis.

If we can't figure out a proper fix, we can always just revert the change easily.

This may be a little unrelated, but I am very confused by the editor situation. It seems that we did not really think about the new cases that were introduced when Minerva and MobileFrontend was split. I feel like this is part of the reason for this bug.

The mobile wikitext editor is always used on Minerva skin, in both mobile and desktop mode, even though the desktop editor is much more useful on desktop. Even weirder is that the mobile visual editor is only used on mobile.

The desktop editor is always used on all other skins, even in mobile mode, even though it seems to me that the mobile editor would work fine with other skins, especially with single-column layouts like narrow Timeless. Maybe it wouldn't fit design-wise with MonoBook or something, but the code should work fine.

Can we instead change this behavior, so that the mobile editor is used on mobile regardless of skin, and the desktop editor is used on desktop regardless of skin?

(I can split this to a separate task if you want to discuss.)

It's not unbreak-now, I suppose, but it's unbreak-before-Tuesday

Agreed

This may be a little unrelated, but I am very confused by the editor situation.

Me too now :)

The mobile wikitext editor is always used on Minerva skin, in both mobile and desktop mode, even though the desktop editor is much more useful on desktop.

Agree with this too.

so that the mobile editor is used on mobile regardless of skin

I like this in principle but is likely going to require some skinStyles.

and the desktop editor is used on desktop regardless of skin?

I don't think there is any blockers to this and at least what I was imagining we were working towards.

The problem from my POV is the definition of mobile mode. Historically, we tend to rely on the ResourceLoader targets system or skin when really we should be relying on screen resolution to determine that.

It sounds like we all might benefit from syncing about what we want to achieve long term and understanding this code a bit more given there have been quite a few patches/regressions now - T185729 > T196150 > T196915 . Right now let's address the broken state. Personally I worry about more patches given the experience so far and would favour a revert and try again approach.

I do agree with matmarex's suggested approach. The issue here seems to stem from a lack of clarity (on my part, on reviewer's part, on the code's part, etc) about exactly when things are providing a mobile experience. Wiping that clean and getting a simple rule for it would make sense.

I'd also be happy to scope everything that has changed so far behind some "only apply to mobile" wall, since that was mostly its intent, but as Joe says we don't have a clear way of specifying that currently. (Well, beyond the whole "people deliberately using Minerva on desktop is uncommon" aspect.)

The issue here seems to stem from a lack of clarity (on my part, on reviewer's part, on the code's part, etc) about exactly when things are providing a mobile experience. Wiping that clean and getting a simple rule for it would make sense.

👍. Some browser tests in Minerva documenting how things should behave would probably also be a good idea.

I'd also be happy to scope everything that has changed so far behind some "only apply to mobile" wall, since that was mostly its intent, but as Joe says we don't have a clear way of specifying that currently

Note, the mobile grade C browser workflow is still broken (unusable) with all these changes so scoping to mobile may not be enough here. I don't know enough about the mobile editing redirect code to know what's going on there and how we might fix that.

Given next train is monday and I'm off tomorrow what's the plan here? Revert them or add additional logic and commit to more vigorous testing?

Change 441569 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] Revert "Suppress display of wikitext editor on action=edit"

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

Yeah. Let's not leave this broken over the weekend. I personally will not have time to investigate before Tuesday and I assume y'all won't either.

Change 441569 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Revert "Suppress display of wikitext editor on action=edit"

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

Thanks for dealing with the immediate problem @matmarex.

I think we can resolve this task? https://en.m.wikipedia.beta.wmflabs.org/wiki/Module:T173949 is editable again now so the bug as reported is fixed. Should we continue discussion on T197834 ?

The issue here seems to stem from a lack of clarity (on my part, on reviewer's part, on the code's part, etc) about exactly when things are providing a mobile experience

Happy to chat through the code/editor this week. Should we set something up?

Jdlrobson lowered the priority of this task from High to Normal.Jun 25 2018, 5:35 PM
Jdlrobson moved this task from 2017-18 Q4 to Needs Prioritization on the Readers-Web-Backlog board.
Esanders renamed this task from Regression: Editing workflow broken for non-JS users and template editors to Regression: Editing workflow broken for non-JS users and template editors on mobile.Jun 26 2018, 12:58 PM
Jdlrobson moved this task from Needs triage to Triaged on the Mobile board.Jun 27 2018, 12:11 AM
Vvjjkkii renamed this task from Regression: Editing workflow broken for non-JS users and template editors on mobile to xjaaaaaaaa.Jul 1 2018, 1:02 AM
Vvjjkkii raised the priority of this task from Normal to High.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from xjaaaaaaaa to Regression: Editing workflow broken for non-JS users and template editors on mobile.Jul 2 2018, 11:27 AM
CommunityTechBot lowered the priority of this task from High to Normal.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.
matmarex closed this task as Resolved.Jul 3 2018, 9:30 PM

Yeah, let's resolve it.

As a follow-up, I filed T198765: Move mobile editor code from Minerva skin to MobileFrontend, use it for all skins on mobile (and none on desktop), and I'd like to work on it soon if you have no objections.

The issue here seems to stem from a lack of clarity (on my part, on reviewer's part, on the code's part, etc) about exactly when things are providing a mobile experience

Happy to chat through the code/editor this week. Should we set something up?

I guess the two pitfalls we identified are:

  • The code is split between MobileFrontend and Minerva
  • Mobile editor is disabled in some namespaces

Are there any other ones? I'll try to document as I go along working on T198765, and I think we'll be okay if you just look over that as part of the code review.