Page MenuHomePhabricator

Adding/removing qualifier/reference using the keyboard (by pressing enter) saves the statement too
Closed, ResolvedPublic

Description

To reproduce:

  • Load https://www.wikidata.org/wiki/Q4115189
  • Start adding a new statement, pick a property and enter a valid value for it
  • Add a qualifier
  • Press tab until you get to the "remove" link
  • Press enter to trigger the link

Expected behaviour: The qualifier is removed, like when you click on the "remove" link with the mouse
Actual behaviour: The qualifier is removed and then any changes are saved, as if you'd clicked "remove" and then "save"

This seems to happen whenever any "remove" link within the statement is triggered and the statement is in a savable state (i.e. if the save button is disabled because there are no changes or because some of the qualifiers or references are incomplete, it will not save it). It does not happen with any of the "add" links within the statement.

Details

Related Gerrit Patches:
mediawiki/extensions/Wikibase : masterAdd test for adding statement via keyboard
mediawiki/extensions/Wikibase : masterDon’t always save statement when enter is pressed inside
mediawiki/extensions/Wikibase : masterFix keyboard navigation when editing statements

Event Timeline

Nikki created this task.Jan 8 2017, 2:53 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 8 2017, 2:53 PM
thiemowmde triaged this task as High priority.Jan 31 2017, 1:52 PM

This is now also the case for the "add" buttons, making it impossible to add more than one qualifier in an edit using the keyboard, and making adding references require a sequence of several edits to add a single reference.

Lucas_Werkmeister_WMDE renamed this task from Removing qualifier/reference using the keyboard (by pressing enter) saves the statement too to Adding/removing qualifier/reference using the keyboard (by pressing enter) saves the statement too.Feb 27 2018, 3:22 PM

Change 462914 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Fix keyboard navigation when editing statements

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

Change 462914 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Fix keyboard navigation when editing statements

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

@Lucas_Werkmeister_WMDE as far as I can tell from your patch this is done? and we should move it to verification on the camp board?

Yes, and it should be on test.wikidata.org by tonight, as far as I understand.

Hm, this isn’t working yet on Wikidata (test and real) even though it’s deployed on both as far as I can tell :/

I tried it on the live site as well and it's not working for me either when trying to follow the steps in the description.

Tarrow added a subscriber: Tarrow.Oct 16 2018, 8:21 AM

I just tried both test and wikidata.org and the patch works as expected for me. Perhaps it is some caching issue?

I just tried it again on the item linked in the description and it still doesn't work. This doesn't seem like a caching issue to me. Browser specific? I'm using Chromium to test.

I tried it in private Firefox and Chromium windows, and while I feel like the issue occurs slightly less frequently than before, it’s definitely not fixed. But the funny part is, on my local wiki I can now reproduce the bug again as well… so at least we don’t have a mismatch between development and production, I suppose? Hopefully that’ll make the bug easier to fix (again) (for real this time).

So I’m now at the “how did my previous ‘fix’ ever work” stage of debugging :) I think the saves are coming from this code in ControllerViewFactory.js:

view.element.on( 'keydown.edittoolbar', function ( event ) {
    if ( view.option( 'disabled' ) ) {
        return;
    }
    if ( event.keyCode === $.ui.keyCode.ESCAPE ) {
        controller.stopEditing( true );
    } else if ( event.keyCode === $.ui.keyCode.ENTER ) {
        controller.stopEditing( false );
    }
} );

I. e., if the “enter” key is pressed anywhere within the toolbar and a save is possible, do it. Removing the keycode.ENTER part does appear to fix this issue, but it creates another one: this “press enter in any of the inputs to save” behavior is actually extremely useful, and you instantly notice it when it’s gone. We can’t get rid of that entirely.

So I think the original browser keydown event is being translated into at least two events:

  • addtoolbaradd.addtoolbar on the qualifier, whose event handler I updated in my previous change to return false, which in jQuery means “stop propagation” and “prevent default action”.
  • keydown.edittoolbar on the whole statement, whose event handler turns this into a “save”.

And we need to figure out some way in which the first handler can prevent the second one being triggered. Apparently, the “stop propagation” and “prevent default action” aren’t enough for that, because it’s actually two different synthetic events generated by jQuery (I think). The good news is that at least addtoolbar fires before edittoolbar, so we don’t need to violate causality to fix this. But I don’t know what the fix is going to look like.

Unassigning myself for now, in case anyone else wants to take a stab at this (though I’ll probably pick it up again sooner or later).

Nikki added a comment.Nov 7 2018, 10:32 AM

Would it be possible to check which type of HTML element is focused? We want pressing enter to save in an input field and trigger the corresponding action on links. The only link which should save a statement when pressing enter is the save link itself.

I do wonder what changed to break it in the first place. It used to work fine, and then the "remove" link broke before the "add" link did which was rather odd.

Would it be possible to check which type of HTML element is focused? We want pressing enter to save in an input field and trigger the corresponding action on links. The only link which should save a statement when pressing enter is the save link itself.

That’s an interesting idea… somewhat hacky, but should work. Thank you :)

I do wonder what changed to break it in the first place. It used to work fine, and then the "remove" link broke before the "add" link did which was rather odd.

I should’ve git bisected this back when it first appeared, but now there’s no way to do this, too many other changes happened in the meantime :(

Change 472208 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Don’t always save statement when enter is pressed inside

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

Change 472238 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] WIP: add test for editing qualifiers via keyboard

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

Change 472208 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Don’t always save statement when enter is pressed inside

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

Change 472238 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add test for adding statement via keyboard

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

Assuming nothing goes wrong, this should be deployed to Wikidata in two weeks. It’s also live on Beta (though you might need to purge some JS caches; I tested it successfully with ?debug=true in the URL, but that only works reliably in Chromium), if you want to try it out earlier.

The fix is live on Wikidata now! \o/