Page MenuHomePhabricator

Vector: "Stay on page" EditWarning ignored when pressing "Enter" in search box
Open, LowPublic

Description

The "edit warning" feature of the Vector skin does not pop up a warning when the user uses the SimpleSearch box. Use case:

  1. Edit any article.
  2. Make a change in the edit box but do not save.
  3. (Optional) Click the "History" link: confirm that you get a proper edit warning. Cancel it.
  4. Remaining on the edit page, type text into the search box and press Enter.
  5. You leave the edit page without a warning, even though the text is modified.

You have now lost your unsaved work.


Version: unspecified
Severity: normal

Details

Reference
bz30101

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 11:54 PM
bzimport set Reference to bz30101.

By "edit warning" do you mean the confirmation dialog for navigating away from the page?

Yes, that's right. I used the phrase "edit warning" because that's how it's referred to in the code: Vector/modules/ext.vector.editWarning.js, etc.

Created attachment 8846
Proposed patch

The warning is shown by the beforeunload handler, but this handler is removed when any form is submitted though it should be removed only if the editform is submitted.
The patch fixes this. Additionally I used mw.config.get for the global variable and changed some other stuff (mediaWiki -> mw, size() -> length, === for comparison to 0).

attachment vector.diff ignored as obsolete

Clicking "stay on page" on the dialog that pops up now after using the enter key in the search field

... doesn't stop you from leaving the page. Tested on FF5.

Works as expected otherwise.

Created attachment 8882
Proposed patch - part 2

Found the culprit in jquery.suggestions.js. When you press enter, the javascript tries to submit the form, so editwarning triggers correctly. Editwarning unbinds the onbeforeunload handler (caching reasons in Firefox). Then the default reaction to the enter key is done: the form is submitted again (probably browser specific).
This patch simply prevents this default.
Both patches together should solve the problem thoroughly.

attachment core2.diff ignored as obsolete

The proposed 2 patches work for me in Firefox and Chrome.

In Internet Explorer 8, however, the edit warning is not appearing at all (in Vector skin) at this point.

How about making this part of 1.19? It can lead to data loss (e.g., the user does a search while on the edit page, and unwittingly loses whatever she was working on).

My earlier comment #6 about Internet Explorer should be ignored. The edit warning is indeed appearing in IE.

sumanah wrote:

Michael, sorry for the wait; there's been a bit of a delay in the review of patches here. As we prepare to get a new version out, we're in a "code slush" during which we concentrate on reviewing code that has already been committed to our source code repository (see
http://thread.gmane.org/gmane.science.linguistics.wikipedia.technical/57950 for
more details).

I hope to get a reviewer to check your contribution soon. In the interim, if you want, you could mark your old patches obsolete and simply combine them into one patch and attach it, which would be slightly easier for us to test and apply.

Thanks for the patch!

sumanah wrote:

Dan, have you gotten a Developer access account yet?

https://www.mediawiki.org/wiki/Developer_access

Thanks Sumana. I have not requested developer access due to lack of time for any development. Maybe in the future.

Thank you for the patch Michael. I've applied part of this fix as the code
base has changed a bit since you posted. Seems to be working now in FF,
Chrome, and IE.

Status of the change can be viewed here.
https://gerrit.wikimedia.org/r/#change,5734

(Quote from comment #4)

Clicking "stay on page" on the dialog that pops up now after using the enter
key in the search field

... doesn't stop you from leaving the page. Tested on FF5.

Still reproducible, please also apply the second part of the proposed patch. (Due to bug 37479 you have to press enter in an empty search field.)

(In reply to comment #13)

(Quote from comment #4)

Clicking "stay on page" on the dialog that pops up now after using the enter
key in the search field

... doesn't stop you from leaving the page. Tested on FF5.

Still reproducible, please also apply the second part of the proposed patch.
(Due to bug 37479 you have to press enter in an empty search field.)

When you tested it it may not have been deployed yet. Merging is not the same as deploying (every 2 weeks, the latest merge state in master is deployed).

Can you try again now?

(In reply to comment #14)

When you tested it it may not have been deployed yet. Merging is not the same
as deploying (every 2 weeks, the latest merge state in master is deployed).

Can you try again now?

I tested it on mw.org, where 1.20wmf5 was already deployed at that time. And yes, I can still reproduce it on any WMF wiki.

  1. Enable EditWarning in your preferences.
  2. Open a page in edit mode.
  3. Type something to change the text.
  4. Press <Enter> in the empty search field.
  5. A message will pop up, asking you if you really want to leave the page (as expected), click "Stay on page"
  6. Despite your choice you will leave the page, and go to Special:Search.

Reproducible at least with Firefox (if I remember correctly, FF submits a form when you press enter; in SimpleSearch there is a missing event.preventDefault(), so the search form is submitted twice, while edit warning only catches it once).

sumanah wrote:

I just reproduced this on test.wikipedia.org, which is running MW core from a day ago and Vector code from Sept. 30th.

In fact the case is slightly worse now than it was in June. (A regression, but not necessarily a regression within the last 2 weeks so I'm not keywording it as a regression.) We once more have the situation as described in the original bug report:

  1. Enable EditWarning in your preferences (Special:Preferences#mw-prefsection-editing, "Warn me when I leave an edit page with unsaved changes").
  2. Open a page in edit mode.
  3. Type something to change the text.
  4. Press <Enter> in the empty search field.
  5. The "do you really want to leave?" message DOES NOT pop up; you will leave the page, and go to Special:Search.

This will lead to data loss by people who aren't quick-thinking enough to immediately hit the Back button. Thus increasing the priority.

I can only reproduce this original bug in Firefox.
I have submitted Michael's second patch in https://gerrit.wikimedia.org/r/#/c/36123/
Which, I have testing and seems to fix this bug.

This change is not yet merged, though I have pulled it to the micro-design instance for testing.
http://micro-design.wmflabs.org/index.php?title=Main_Page&action=edit

sumanah wrote:

This is now fixed in Vector. Thanks for the fix, Rob!

Problem still happens in Monobook in Firefox.

Also there's an edge case where it still happens in Vector, as Rob and I just discovered:

  1. Enable EditWarning in your preferences

(Special:Preferences#mw-prefsection-editing, "Warn me when I leave an edit page
with unsaved changes").

  1. Open a page in edit mode.
  2. Type something to change the text.
  3. Press <Enter> in the empty search field.
  4. The "do you really want to leave?" message pops up. Hit Leave Page.
  5. On the new page, hit Back.
  6. Now click in the search field and hit <Enter>. Even though the article text has changed and not been saved, the "wait, really?" dialog box does NOT pop up, defying the promise of the edit warning preference! This is because the page is loaded and we don't detect a text change.

Now this is lower-priority, I think.

Change-Id: I3cb40f70bf332e4aa53cd656b1c847b5f4637360

(In reply to comment #18)

This is now fixed in Vector. Thanks for the fix, Rob!

Problem still happens in Monobook in Firefox.

I can no longer reproduce the bug in Vector, Monobook both in both Chrome and Firefox.

Also there's an edge case where it still happens in Vector, as Rob and I just
discovered:

  1. Enable EditWarning in your preferences

(Special:Preferences#mw-prefsection-editing, "Warn me when I leave an edit
page
with unsaved changes").

  1. Open a page in edit mode.
  2. Type something to change the text.
  3. Press <Enter> in the empty search field.
  4. The "do you really want to leave?" message pops up. Hit Leave Page.
  5. On the new page, hit Back.
  6. Now click in the search field and hit <Enter>. Even though the article

text has changed and not been saved, the "wait, really?" dialog box does
NOT pop up,
defying the promise of the edit warning preference! This is because the page
is loaded and we don't detect a text change.

Confirmed that this bug is still happening in Chrome and Firefox in both Vector and Monobook.

However the patch submitted on this bug (in Gerrit as I3cb40f70bf) does NOT fix it (no visible change in behaviour).

maddiemadan wrote:

I cant reproduce the original bug but when i press enter in an empty search box while editing a page, then even if i choose stay on page, it still go to the search page discarding all the changes.
Reproduced on FireFox

Confirming comment 21 with Firefox 25.0 with settings described in comment 18.

Using the wikitext editor on test2.wikipedia.org in Firefox I can't reproduce this anymore.

  1. https://test2.wikipedia.org/wiki/Sandbox.
  2. Edit, wikitext source mode.
  3. Type something in the editor.
  4. Focus search field
  5. Press enter.

It asks "This page is asking you to confirm that you want to leave - data you have entered may not be saved." as expected. Where option "Stay on page" leads to the user not being navigated to the search results, but instead stays on the edit page.

The original bug has been fixed, all that remains is (comment T32101#355872 above):

  1. Edit a page
  2. Leave without saving, confirm the warning
  3. Use your browsers 'back'-button to continue editing
  4. Leave again without saving

Expected result: A warning is shown as before
Actual result: You leave without a warning

But in this scenario at least you can go back to editing without loosing your edit, if only you remember you haven't saved your change.