Page MenuHomePhabricator

Update page dynamically after save on mobile
Closed, ResolvedPublic

Description

We currently update pages dynamically after changes are saved on desktop using the HTML from the save success response. However, we don't do this on mobile. Instead, the page is reloaded. [i][ii]

There is an added complication that the returned HTML needs to be modified by MobileFrontend, but hopefully that can be done in ApiVisualEditorEdit.

Deployment Timing

The patches to resolve T219420 will ride the train that is scheduled to begin rolling out on 24 May 2022.

To create sufficient time for pre-deployment testing, the Editing Team plans to merge said patches on Tuesday, 17 May 2022

Use Cases

Beyond increasing the overall fluidity of the article "publishing moment," being able to dynamically update pages after an edit is published on mobile will enables us to offer people:

  • New comment detection within the Reply Tool on mobile (T301929)
  • Smoother experiences for publishing new comments and topics with DiscussionTools (T301839 + T301840)

Requirements

Approaches

This section currently contains ideas that were "loosely sketched" during the Editing Team's 16 Feb 2022 team meeting.

  • Produce HTML with mobile transformations after saving
  • Set up interactive elements (collapsible sections, TOC, page issues boxes) again after loading new contents
  • For the reply tool: Maybe add a ?expandall=1 hack so that we can just redirect to the #commentid and the browser will cope without any post-load JS?

Deployment plan

Week of 4 April

  • Non-user-facing API changes will be deployed via the train
  • Editing Team will monitor feedback channels (e.g. Phabricator/Slack/etc.) for issues related to API change

Monday, 11 April

  • Editing Team will review: 1) QA capacity and 2) the issues – if any – that have been filed over the preceding week
  • Editing Team will decide when the user-facing changes will be deployed
    • Note: once the user-facing changes are merged, the Editing Team will carry out the QA described in the ===QA section below.

QA

  1. Save a page and ensure the published page appears and interacts as expected (section collapsing, table of contents collapsing)
  2. Open the editor again and ensure VE works as expected and the page saves again (check 1. again)
  3. Mobile content transformations (like collapsible sections or lead paragraph transform) failing to apply on mobile in some scenarios (e.g. in apps, when previewing in wikitext editor)
  4. Mobile content transformations unexpectedly applying on desktop in some scenarios (e.g. after saving in visual editor, when using non-default skin)

Done

  • The way(s) in which this functionality could be implemented, and the trade offs associated with them, are documented in the ===Approaches section above
  • A decision is made about if/when the Editing Team will prioritize work on implementing one of the ===Approaches

i. https://youtu.be/LKs_94Pyorw
ii. https://youtu.be/lpusO5JFlGI

Related Objects

Event Timeline

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

We added the output hooks to the Parse API back in October 2020: T266195, so that's a bulk of the work done.

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

[mediawiki/extensions/MobileFrontend@master] Update contents of page dynamically after VE edit

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

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

[mediawiki/extensions/VisualEditor@master] Implement replacePageContent in MobileArticleTarget

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

Esanders updated the task description. (Show Details)
Esanders moved this task from Upcoming to Doing on the Editing-team (FY2021-22 Kanban Board) board.

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

[mediawiki/extensions/MobileFrontend@master] Apply transformations to ApiParse output using onOutputPageBeforeHTML

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

For the reply tool: Maybe add a ?expandall=1 hack so that we can just redirect to the #commentid and the browser will cope without any post-load JS?

I don't think this is necessary, MobileFrontend remembers which sections you've opened, so this should be automatically restored.

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

[mediawiki/core@master] ApiParse: Enable "skin mode" when mobileformat=1 is set

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

Set up interactive elements (collapsible sections, TOC, page issues boxes) again after loading new contents

I'll link the patches for this to this task as well, for future reference.

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

[mediawiki/extensions/MobileFrontend@master] Init toggling on wikipage.content hook

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

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

[mediawiki/extensions/MobileFrontend@master] Hide ToC in preview using CSS

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

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

[mediawiki/skins/MinervaNeue@master] Move TOC button fixes to wikipage.content hook

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

I've tested the three patches above by using the following snippet in the browser console while viewing a page with a bunch of headings:

p = await new mw.Api().post( { action:'parse', page: 'Headings', mobileformat: 1, useskin: 'minerva' } );
$( '#mw-content-text' ).html( p.parse.text['*'] );
mw.hook( 'wikipage.content' ).fire( $('#mw-content-text') );

(The page should look exactly the same as before after reloading it this way – before the changes, TOC and headings would get messed up)

Note that everything works correctly only with all 3 patches applied, otherwise various elements don't behave properly.

Change 764900 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Move TOC button fixes to wikipage.content hook

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

Change 763616 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Init toggling on wikipage.content hook

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

Change 763615 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Hide ToC in preview using CSS

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

Status update: we're happy with the patches at https://gerrit.wikimedia.org/r/q/topic:editor-update, and we're planning to merge them piece-by-piece over a few weeks (to make it easier to debug or revert in case something doesn't work perfectly), depending on what we have time to test. We started last week with the three above, we'll probably do the rest this week (to ride next week's train).

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

[mediawiki/core@master] ApiParse: Enable "skin mode" when mobileformat=1 is set

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

Change 770974 merged by jenkins-bot:

[mediawiki/core@master] ApiParse: Enable "skin mode" when mobileformat=1 is set

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

Change 763622 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Apply transformations to ApiParse output using onOutputPageBeforeHTML

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

Things to watch out for next week, related to the merged patches above:

  • Mobile content transformations (like collapsible sections or lead paragraph transform) failing to apply on mobile in some scenarios (e.g. in apps, when previewing in wikitext editor)
  • Mobile content transformations unexpectedly applying on desktop in some scenarios (e.g. after saving in visual editor, when using non-default skin)
kostajh added subscribers: Etonkovidova, kostajh.

Adding Growth-Team as our team will want to do some QA on this (cc @Etonkovidova). In particular, we probably need to rework some of our code invoked from postEditMobile hook, as it assumes that the page is reloaded after edit. I've made a task for us to look at our code specifically in connection with the patches here, T305543: Verify Growth editing workflows on mobile.

@kostajh Thanks for letting us know, should we delay until you've had time to look at the code? (or can we help?)

T301839 no longer depends on this (it only depends on MobileFrontend changes that were already merged, not on the VisualEditor changes that are waiting), so I'm removing the dependency. That will not affect your (Growth team's) tools built on top of VisualEditor. Feel free to take your time with review (only T301839 is high priority for us).

T301839 no longer depends on this (it only depends on MobileFrontend changes that were already merged, not on the VisualEditor changes that are waiting), so I'm removing the dependency. That will not affect your (Growth team's) tools built on top of VisualEditor. Feel free to take your time with review (only T301839 is high priority for us).

Thanks @matmarex! We'll probably wait a week so @Etonkovidova is available to check for any unexpected side effects it might have on Growth code.

ppelberg added a subscriber: ppelberg.

hi @Etonkovidova! The Editing Team is assuming this set of patches being merged is blocked T305543 being resolved.

A resulting question for you/the Growth Team: does y'all beginning work on T305543 depend on the patches that will resolve T219420 first being merged?

Hi @ppelberg, I have started investigating the implications of this change (will add more notes to T305543: Verify Growth editing workflows on mobile) but have not made any changes on our end yet. Would the mobile change be merged but not enabled to users first or would it be in effect right when the patches are merged? Ideally we would test our changes against the new VE changes first.

@mewoph the remaining two patches will immediately enable the new behavior. There's not any config for it, it just changes how things behave.

Thanks for confirming @DLynch! I'll test our changes against the two patches.

Change 763603 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Update contents of page dynamically after VE edit

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

Change 763604 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Implement replacePageContent in MobileArticleTarget

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

matmarex edited projects, added Editing QA; removed Patch-For-Review.

All patches merged, this is ready for testing on the beta cluster (it should be updated automatically soon).

Minor issues I've noticed while reviewing:

  • After creating a new page, the page is reloaded with ?venotify=created parameter added to the URL, but it is not handled on mobile to display the appropriate notification.
  • After saving from ?action=edit, the browser back button must be used twice to return to the editor, because there is an extra entry in the browser history. (Note that there are various pre-existing issues with the mobile editor and back/forward navigation, resulting from weird interactions with mobile overlays, and maybe it's better not to touch this to avoid making that worse.)

Also, Jon noted some missing features in how the page content is updated:

  • Updating the last modified bar which shows a relative time – For now we just remove the bar
  • Adding a category to a page which had no categories before (AMC mode only)
  • If a page has no languages and an edit adds one

I felt that none of these are blockers, and overall the improvement in user experience more than makes up for them, but we might go back and improve some of them if we have the time.

All patches merged, this is ready for testing on the beta cluster (it should be updated automatically soon).

Excellent. A couple of clarifying questions below...

Minor issues I've noticed while reviewing:

  • After creating a new page, the page is reloaded with ?venotify=created parameter added to the URL, but it is not handled on mobile to display the appropriate notification.

Can you share an example of what "appropriate notification" means in this context?

  • After saving from ?action=edit, the browser back button must be used twice to return to the editor, because there is an extra entry in the browser history. (Note that there are various pre-existing issues with the mobile editor and back/forward navigation, resulting from weird interactions with mobile overlays, and maybe it's better not to touch this to avoid making that worse.)

Not doing anything about this for now sounds good to me.

Also, Jon noted some missing features in how the page content is updated:

  • Updating the last modified bar which shows a relative time – For now we just remove the bar

Would it be accurate for me to understand the "last modified bar" as the element highlighted in the screenshot below? If so, I agree this should NOT block the deployment. I'll file a follow-up task to get this working.

IMG_CC53556B2A00-1.jpeg (2×1 px, 602 KB)

  • Adding a category to a page which had no categories before (AMC mode only)

Would it be accurate for me to understand this issue as the following?

  1. Ensure AMC is turned on
  2. On mobile, open the full-page source editor or visual editor on a page that does NOT contain any categories
  3. Add at least one category to the page
  4. Publish the change you made in "3."

5.❗️ Notice the category you published in "4." does NOT appear on the page

  • If a page has no languages and an edit adds one

Can you describe this issue in a bit more detail?

Minor issues I've noticed while reviewing:

  • After creating a new page, the page is reloaded with ?venotify=created parameter added to the URL, but it is not handled on mobile to display the appropriate notification.

Can you share an example of what "appropriate notification" means in this context?

"This page has been published", you can see this by adding the param to any page, e.g. https://en.wikipedia.org/wiki/President_of_Somalia?venotify=created

Also, Jon noted some missing features in how the page content is updated:

  • Updating the last modified bar which shows a relative time – For now we just remove the bar

Would it be accurate for me to understand the "last modified bar" as the element highlighted in the screenshot below? If so, I agree this should NOT block the deployment. I'll file a follow-up task to get this working.

IMG_CC53556B2A00-1.jpeg (2×1 px, 602 KB)

Yes

  • Adding a category to a page which had no categories before (AMC mode only)

Would it be accurate for me to understand this issue as the following?

  1. Ensure AMC is turned on
  2. On mobile, open the full-page source editor or visual editor on a page that does NOT contain any categories
  3. Add at least one category to the page
  4. Publish the change you made in "3."

5.❗️ Notice the category you published in "4." does NOT appear on the page

The source editor (mobile frontend editor) always reloads the page after editing, so not an issue there.
It is not currently possible to edit categories in mobile VE, so also not an issue (yet).

  • If a page has no languages and an edit adds one

Can you describe this issue in a bit more detail?

The language icon only (below) appears when interlanguage links are present for the current page. This isn't really an issue for us as we moved interlanguage links on Wikipedia to Wikidata in 2013, so your edit will not affect the number of interlanguage links.

image.png (83×362 px, 3 KB)

All patches merged, this is ready for testing on the beta cluster (it should be updated automatically soon).

Minor issues I've noticed while reviewing:

  • After creating a new page, the page is reloaded with ?venotify=created parameter added to the URL, but it is not handled on mobile to display the appropriate notification.
  • After saving from ?action=edit, the browser back button must be used twice to return to the editor, because there is an extra entry in the browser history. (Note that there are various pre-existing issues with the mobile editor and back/forward navigation, resulting from weird interactions with mobile overlays, and maybe it's better not to touch this to avoid making that worse.)

Also, Jon noted some missing features in how the page content is updated:

  • Updating the last modified bar which shows a relative time – For now we just remove the bar
  • Adding a category to a page which had no categories before (AMC mode only)
  • If a page has no languages and an edit adds one

I felt that none of these are blockers, and overall the improvement in user experience more than makes up for them, but we might go back and improve some of them if we have the time.

There's another issue I wrote about on the patch, and @mewoph made a screencast of in betalabs:

  1. Load an article, wait for JS to load
  2. Click edit pencil
  3. Make an edit and save it
  4. I see "Success your edit was published", no page reload.
  5. The editor is open with the updated content

I assume that needs to be fixed before these patches arrive at the production wikis?

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

[mediawiki/extensions/MobileFrontend@master] Wait for router.back() in overlay teardown to happen before setting location

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

Change 793080 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Wait for router.back() in overlay teardown to happen before setting location

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

There's another issue I wrote about on the patch, and @mewoph made a screencast of in betalabs:
(…)

Thanks, should be resolved now by the patch above.

There's another issue I wrote about on the patch, and @mewoph made a screencast of in betalabs:
(…)

Thanks, should be resolved now by the patch above.

https://photos.app.goo.gl/YAgpSRhHYFgoFdtP8 when the issue in T219420#7938690 was still present
After fix: https://photos.app.goo.gl/YXrjkfQLH7quSG5JA

Observations