Page MenuHomePhabricator
Create Task
Maniphest T154869

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

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

SubjectRepoBranchLines +/-
Add test for adding statement via keyboardmediawiki/extensions/Wikibasemaster+106 -0
Don’t always save statement when enter is pressed insidemediawiki/extensions/Wikibasemaster+19 -2
Fix keyboard navigation when editing statementsmediawiki/extensions/Wikibasemaster+1 -0
Customize query in gerrit

Related Objects
Search...

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
Lea_Lacroix_WMDE added a parent task: T40968: Keyboard-navigability of the repo UI.Jan 9 2017, 2:51 PM
Liuxinyu970226 subscribed.Jan 12 2017, 10:15 AM
thiemowmde triaged this task as High priority.Jan 31 2017, 1:52 PM
thiemowmde added subscribers: • Jonas, thiemowmde, Lydia_Pintscher.
Lydia_Pintscher added a parent task: T154484: [Story] Improved keyboard navigation.Feb 19 2017, 10:20 AM
Yair_rand subscribed.Oct 19 2017, 11:07 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
gerritbot subscribed.Sep 26 2018, 10:44 AM

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

gerritbot added a project: Patch-For-Review.Sep 26 2018, 10:44 AM
Lydia_Pintscher added a project: Wikidata-Campsite.Sep 26 2018, 1:28 PM
Lydia_Pintscher moved this task from Incoming to Ready to estimate on the Wikidata-Campsite board.
gerritbot added a comment.Sep 26 2018, 2:44 PM

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

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

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-10-02 (1.32.0-wmf.24)).Sep 26 2018, 3:00 PM
Addshore added a project: Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)).Oct 2 2018, 1:26 PM
Addshore moved this task from To Do (prioritised from top to bottom) to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.
Addshore moved this task from Ready to estimate to In Progress on the Wikidata-Campsite board.Oct 2 2018, 1:34 PM
Addshore assigned this task to Lucas_Werkmeister_WMDE.Oct 2 2018, 1:55 PM
Addshore added subscribers: Lucas_Werkmeister_WMDE, Addshore.

@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?

Lucas_Werkmeister_WMDE added a comment.Oct 2 2018, 1:59 PM

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

Addshore moved this task from Peer Review to Test (Verification) on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Oct 2 2018, 1:59 PM
Addshore moved this task from incoming to in progress on the Wikidata board.
Lucas_Werkmeister_WMDE added a comment.Oct 4 2018, 2:19 PM

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

Lydia_Pintscher moved this task from Test (Verification) to To Do (prioritised from top to bottom) on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Oct 15 2018, 2:58 PM

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 subscribed.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?

Lydia_Pintscher added a comment.Oct 20 2018, 6:56 PM

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.

Addshore removed a project: Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)).Nov 1 2018, 11:13 AM
Addshore edited projects, added Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)); removed Wikidata-Campsite.
Lucas_Werkmeister_WMDE added a comment.Nov 5 2018, 11:29 AM

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).

Addshore moved this task from To Do (prioritised from top to bottom) to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Nov 5 2018, 4:26 PM
Lucas_Werkmeister_WMDE removed Lucas_Werkmeister_WMDE as the assignee of this task.Nov 6 2018, 5:32 PM

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.

WMDE-leszek moved this task from Doing to To Do (prioritised from top to bottom) on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Nov 7 2018, 11:12 AM
Lucas_Werkmeister_WMDE claimed this task.Nov 7 2018, 4:43 PM
Lucas_Werkmeister_WMDE moved this task from To Do (prioritised from top to bottom) to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.
Lucas_Werkmeister_WMDE added a comment.Nov 7 2018, 4:59 PM

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 :(

gerritbot added a comment.Nov 7 2018, 5:32 PM

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

Lucas_Werkmeister_WMDE moved this task from Doing to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Nov 7 2018, 5:33 PM
gerritbot added a comment.Nov 7 2018, 7:46 PM

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

Liuxinyu970226 unsubscribed.Nov 8 2018, 4:19 AM
Tarrow moved this task from Peer Review to Test (Verification) on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Nov 8 2018, 3:01 PM
gerritbot added a comment.Nov 8 2018, 3:29 PM

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

gerritbot added a comment.Nov 8 2018, 3:35 PM

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

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

ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.4; 2018-11-13).Nov 8 2018, 4:00 PM
Lucas_Werkmeister_WMDE added a comment.Nov 8 2018, 4:50 PM

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.

Lydia_Pintscher closed this task as Resolved.Nov 9 2018, 2:10 PM
Lydia_Pintscher moved this task from Test (Verification) to Done on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.

\o/

Lucas_Werkmeister_WMDE added a comment.Nov 14 2018, 11:46 PM

The fix is live on Wikidata now! \o/

Lucas_Werkmeister_WMDE mentioned this in T50136: [Task] Browsertests for focus flow.Jan 23 2019, 1:37 PM
matej_suchanek merged a task: T178547: [Regression] When you tab to "add qualifier" link when adding a statement and press "Enter", the statement is saved.Feb 7 2019, 6:19 PM
matej_suchanek added a subscriber: jhsoby.
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL