Page MenuHomePhabricator

Clicking the sticky header edit icon should not reload the page
Closed, DuplicatePublicBUG REPORT

Description

Follow up to T289723. When clicking the edit icons in the new sticky header, the entire page is reloaded. This defeats the point of the sticky header in some ways as it loses the editor's position in the document which might be the reason for clicking the edit icon.

A patch was proposed in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/726694 thinking this might be straightforward, but Bartosz pointed out it wouldn't be, having this to say:

I support doing this, but it will take more work than this to work well. I have two points:

  1. There are a few complicated behaviors to the edit tabs that this doesn't handle.

    Most importantly, we need to make sure that the sticky header icon won't open VE when it's disabled in user preferences, lest someone go for our heads again. [see inline]

    Furthermore, we need to implement/test the following: (I didn't test it all with this patch, but it doesn't look like you're handling most of it)
    • single edit tab (extra tricky stuff: permanently switching to the other mode if the user switches the mode in the editor; behavior of the single tab when VE is disabled but 2017WTE is enabled)
    • 2017 wikitext editor
    • DiscussionTools (trying to open VE/2017WTE while reply tool is opened should warn first, instead of losing your input)

      Also, if you click the sticky header icon while scrolled down, the page jumps to the top after the editor loads; it should stay (approximately) in the same place. (Somewhat related old feature request: T54577.)
  1. The above will be a lot of work. If you're investing effort into that already, I wonder if it could be done in such a way as to also support other ways of opening the editor, instead of just the Vector sticky header. We did something similar in DiscussionTools, where we intercept all action=edit links on the page (including those added by gadgets, or within the wikitext of the page). For the editor, this would support things like the "Edit page visually" button on https://en.wikipedia.org/wiki/Wikipedia:Sandbox, or the links added by gadgets that add a section 0 edit link or those that copy article tabs to the bottom of the page.

I haven't had a chance to read through this feedback, but detecting the phrase "a lot of work" figured that this likely deserves a task of its own, and some further analysis/discussion.

Approaches

PROTOTYPEDESCRIPTIONLINK
A)Open editor to top of articlehttps://patchdemo.wmflabs.org/wikis/c8f6e6f09f/w/index.php?title=New_York_(state)&vectorstickyheader=1&redirect=no
B)Open editor to top of nearest section visible when edit affordance was clickedhttps://patchdemo.wmflabs.org/wikis/34f1822ae5/w/index.php?title=Table_of_contents
C)Open editor to the nearest element visible when edit affordance was clickedhttps://patchdemo.wmflabs.org/wikis/000ff2c092/wiki/Cats

Event Timeline

Change 726694 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/VisualEditor@master] Vector sticky header edit icons should not trigger page load

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

Change 726694 abandoned by Jdlrobson:

[mediawiki/extensions/VisualEditor@master] Vector sticky header edit icons should not trigger page load

Reason:

Created a task to track this discussion.

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

Test wiki created on Patch Demo by Jdlrobson using patch(es) linked to this task:

https://patchdemo.wmflabs.org/wikis/c8f6e6f09f/w/

I set up a patch demo to test some of Bartosz's concerns (make sure you log in as Patch demo):
https://patchdemo.wmflabs.org/wikis/c8f6e6f09f/wiki/New_York_(state)?vectorstickyheader=1

Most importantly, we need to make sure that the sticky header icon won't open VE when it's disabled in user preferences, lest someone go for our heads again. [see inline]

There seems to be no problem with this requirement. When I disable VE the icon doesn't even show.

single edit tab (extra tricky stuff: permanently switching to the other mode if the user switches the mode in the editor; behavior of the single tab when VE is disabled but 2017WTE is enabled)

This also doesn't seem to be a problem?

2017 wikitext editor

> DiscussionTools (trying to open VE/2017WTE while reply tool is opened should warn first, instead of losing your input)

I'm not sure what these mean but I don't see why these would be a problem.

Also, if you click the sticky header icon while scrolled down, the page jumps to the top after the editor loads; it should stay (approximately) in the same place. (Somewhat related old feature request: T54577.)

That does seem to be a problem, but I'd imagine this could be handled by the VisualEditor click event handler checking the scrollTop when the editor is clicked or a keyboard shortcut is presed.

The above will be a lot of work.

There doesn't seem like a lot of work to me but I may be missing something important. From my perspective the patch I posted seems to be doing most of the work here. A follow-up could make sure we consider scroll position when edit is clicked.

if it could be done in such a way as to also support other ways of opening the editor, instead of just the Vector sticky header

I think this is just a case of using a generic class rather than the ID which is not much work.

I don't have the time to document it in detail right now, but I assure you I am not making it up, and the problems are real even though you don't see them. I tested your patch with single edit tab mode and it doesn't work (the page is always reloaded), and with 2017 wikitext editor and it also doesn't work (it opens the visual editor instead!). I hope these links help provide context, but if you need more, I'll try to find time next week to document everything in detail with videos.

No worries. We have a sync with your team next week. I'll try to get more informed going into that meeting.

I dont doubt the problems are real, and it's a misunderstanding our side but I would be surprised if this was a difficult thing to fix given the sticky header logic follows the state of the page. That said I appreciate there is likely some VisualEditor specific context I don't have. Hope you have a good weekend!

Change 726694 restored by Jdlrobson:

[mediawiki/extensions/VisualEditor@master] Vector sticky header edit icons should not trigger page load

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

I tested your patch with single edit tab mode and it doesn't work (the page is always reloaded),
and with 2017 wikitext editor and it also doesn't work (it opens the visual editor instead!).

Was able to replicate with:

wfLoadExtension('VisualEditor');
$wgVisualEditorEnableWikitext = true;
$wgVisualEditorUseSingleEditTab = true;

Revised patch now addresses the behaviour.

While I agree the experience around clicking edit and scrolling to the top is not great, I think we can iterate off what we have here.

Relevant VisualEditor patch was merged, it was accidentally associated with T293159 instead of this task:

Change 726694 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Give skins/extensions ability to trigger VisualEditor

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

Change 732456 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Sticky header edit icons trigger via JavaScript

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

Change 732839 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] WIP: Clicking edit jumps to closest section

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

Jdlrobson added a subscriber: ovasileva.

Following our meeting today I've put an invite in David's calendar to work out how to solve the issue in https://gerrit.wikimedia.org/r/732456
Before that, David said it probably makes sense for the editing team to talk amongst themselves about the right solution before sitting down with me.
I trust the editing team's judgment here, as you know this code best, so I am okay with whatever you decide, but my hope is when I sit down with David we can pair on the right solution and hammer out an implementation.

Problem

The patch makes the sticky header edit icon trigger a click event on the edit buttons in the tabs at the top of the page ( for now).

The problem with this, is if there is no click handler bound to these elements, nothing will happen.

I currently have a hack to workaround this which waits 300ms and then checks the classes on the body:
https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/732456/5/resources/skins.vector.es6/stickyHeader.js#124

We can do better than this.

Possible solutions

Ideas that come to mind:

  1. VisualEditor could trigger a hook that we subscribe to, so that we know which edit icons have click handlers setup
  2. VisualEditor could always add a click handler e.g. $edit.on('click', (ev) => location.href = ev.getAttribute('href') ) } and we check the ve-available class on the body.
  3. We could wire this code up in VisualEditor on a generic selector.
  4. VisualEditor adds classes to the elements that we check when the sticky header edit icon is clicked.

I think it really comes down to what the editing team think is the most future proof and easiest way to do this.

Change 732456 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Sticky header edit icons trigger via JavaScript

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

After talking to @DLynch we have 4 practical options

  1. Jump to top of page on edit click (current behaviour)
  2. On edit scroll smoothly to nearest section and open editor there https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/732839
  3. Variant of 3, where we attempt to scroll to the same position, but to nearest visible element. This will not work on all pages so it would be a question of testing how often it does. David is working on a patch that will improve this: https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/734885
  4. Delay roll out of edit button in sticky header

Meta
I've added an ===Approaches section to the task description that includes links to prototypes demonstrating the approaches @DLynch and @Jdlrobson converged on in T293158#7472086.

Change 732839 abandoned by Jdlrobson:

[mediawiki/skins/Vector@master] Clicking edit jumps to closest section

Reason:

Not working on this right now.

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

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/c8f6e6f09f/w/

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/34f1822ae5/w/