Make WordPress Core

Opened 13 years ago

Last modified 4 weeks ago

#25696 assigned defect (bug)

Double clicking update on list table inline edit removes row from dom

Reported by: mt8.biz Owned by:
Priority: normal Milestone: Future Release
Component: Quick/Bulk Edit Version: 3.1
Severity: normal Keywords: has-test-info has-patch has-unit-tests
Cc: Focuses: ui, javascript

Description

Steps to reproduce:
Open "Quick Edit" -> Save the category(or tag) by double-click on save-button. ( or press twice ).

Expected behaviour:
Save the category(or tag) , and refresh the table-row.

Actual behaviour:
Save the category(or tag), But disappear the table-row.

Attachments (8)

inline-edit-tax.js.diff (763 bytes ) - added by mt8.biz 13 years ago.
25573.jpg (50.6 KB ) - added by mt8.biz 13 years ago.
inline.diff (938 bytes ) - added by shanept 13 years ago.
Changes to inline-edit-tax.js and inline-edit-post.js.
25696.png (11.6 KB ) - added by shanept 13 years ago.
edit-comments.js.diff (1.2 KB ) - added by mt8.biz 13 years ago.
some fix.
25696.patch (1.1 KB ) - added by afercia 11 years ago.
25696.2.patch (1.3 KB ) - added by azaozz 11 years ago.
25696.3.patch (7.6 KB ) - added by afercia 11 years ago.

Download all attachments as: .zip

Change History (35)

@mt8.biz
13 years ago

#1 @knutsp
13 years ago

Confirmed for 3.8-alpha. Only happens when double clicking.

#2 follow-up: @c3mdigital
13 years ago

  • Component AdministrationQuick/Bulk Edit
  • Summary When editing the Category (or Tag) by "Quick Edit", disappear the table-row.Double clicking update on list table inline edit removes row from dom
  • Version trunk3.1

I can reproduce in 3.6.1 On all List Tables that have quick edit. This is a result of how closures work in javascript and can be explained better here http://stackoverflow.com/a/10152046.

Maybe disabling the update button when the first click event fires?

#3 @helen
13 years ago

#25573 was marked as a duplicate.

#4 @helen
13 years ago

#25606 was marked as a duplicate.

#5 @helen
13 years ago

Closed #25573 (posts) and #25606 (comments), as this ticket seems to have attracted more eyes. Suggest working on a generalized solution, even if it has to go into multiple files.

Related: #25627

@shanept
13 years ago

Changes to inline-edit-tax.js and inline-edit-post.js.

@shanept
13 years ago

#6 @mt8.biz
13 years ago

Thanks For Manage Tickets. ( And Patch To English. )

Do you have problems with my patch?

#7 @mt8.biz
13 years ago

Changes to edit-comments.js .

I was referring to the patch of shanept.

Thanks.

@mt8.biz
13 years ago

some fix.

#8 @mt8.biz
13 years ago

  • Keywords needs-testing added

#9 @chriscct7
11 years ago

  • Keywords needs-refresh added

@afercia
11 years ago

#10 @afercia
11 years ago

  • Focuses ui javascript added
  • Keywords needs-refresh removed
  • Milestone Awaiting Review4.5
  • Owner set to afercia
  • Status newassigned

I'd propose a new approach, similar to what is used in heartbeat.js (actually heartbeat.js has two ways to handle this). Basically aborting the AJAX request if any previous one has not completed yet would solve the double click (and double Enter) issue. The same approach can be used to fix #12769.

The attached patch is for testing only for now, and focuses only on inline-edit-post.js. If OK, the same thing should be done for inline-edit-tax.js. Have to check comments yet.

Any feedback would be greatly appreciated, cc @azaozz :)

Note: the simpler way to reproduce this is:

  • Quick Edit a post
  • place the cursor on any input field or textarea, focus a select or check a checkbox
  • quickly press "Enter" twice
  • after saving, the Quick Edit form closes and the table row disappears

#11 in reply to: ↑ 2 @ericlewis
11 years ago

Replying to c3mdigital:

Maybe disabling the update button when the first click event fires?

This sounds like a good approach, as this is something we already do in other cases (e.g. Publish button on the Edit Post page is disabled after click and during autosaves).

#12 follow-up: @afercia
11 years ago

Disabling buttons is problematic for keyboard users since the buttons will be focused and then disabled, causing a focus loss.

#13 in reply to: ↑ 12 ; follow-up: @ericlewis
11 years ago

Replying to afercia:

Disabling buttons is problematic for keyboard users since the buttons will be focused and then disabled, causing a focus loss.

Good to know! Should button disabling be considered an anti-pattern for that reason?

#14 in reply to: ↑ 13 @afercia
11 years ago

Replying to ericlewis:

Good to know! Should button disabling be considered an anti-pattern for that reason?

It always depends, case by case. But when disabling, hiding, removing from the DOM, a focused element then focus should be moved to the most logical place. Better: avoid a focus loss in the first place :)
Also, the Quick Edit form can be submitted pressing Enter on any field (also the textarea, select, checkboxes) so I'm not sure disabling the button would really solve the issue.

This ticket was mentioned in Slack in #core-editor by afercia. View the logs.


11 years ago

#16 @azaozz
11 years ago

25696.patch is a good start. Think we will need to abort the request when the users click Cancel so if they click on another Quick Edit it won't block it.

We also may need better "busy" indication. Clicking on another Quick Edit while the request hasn't been completed yet or had some sort of error is frustrating: nothing happens. Wondering if we should be showing wait or progress cursor?

@azaozz
11 years ago

#17 @azaozz
11 years ago

In 25696.2.patch: also add xhr.abort() when the user clicks Cancel.

This ticket was mentioned in Slack in #core-editor by azaozz. View the logs.


11 years ago

@afercia
11 years ago

#19 @afercia
11 years ago

Refreshed patch, taking care also of Terms and Comments. Some notes:

  • when saving maybe it should just return false instead of using xhr.abort()
  • should users be allowed to open a new Quick Edit form while a previous request has not completed yet? this would be easy to restore, not sure if safe
  • removed the toggle function, it was used to open the form on double click, seems no more used since [9439] and removed in [9726] unless I'm missing something
  • the parameter passed to save() and edit() is always a DOM object so calling it id is a bit confusing

Clicking on another Quick Edit while the request hasn't been completed yet or had some sort of error is frustrating: nothing happens.

Yep, see point 2. Currently, clicking to open a new form won't do anything, see below:

https://cldup.com/A5ZYGWuqWz.png

Theoretically, the whole thing could be refactored in order to allow a row to be updated "in the background" when it's closed because users clicked on a new Quick Edit link. Not sure it would be so safe and easy to implement :)

#20 @afercia
11 years ago

To recap: the patch still applies cleanly, needs a decision about this point:

should users be allowed to open a new Quick Edit form while a previous request has not completed yet?

in response to:

Clicking on another Quick Edit while the request hasn't been completed yet or had some sort of error is frustrating: nothing happens.

Note: this should fix also #12769.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


11 years ago

#22 @chriscct7
11 years ago

  • Milestone 4.5Future Release

#23 @afercia
8 years ago

  • Owner afercia removed

#24 @SirLouen
16 months ago

  • Keywords needs-refresh has-test-info added; has-patch needs-testing removed

Reproduction Report

Description

✅ This report validates that the issue can be reproduced.

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 137.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-One 2.5
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Test Steps

  • Go to Posts > Categories
  • Create a Category
  • Click on Quick Edit for that category
  • Double-click on the Update Category button
  • 🐞 The category disappears

Actual Results

  1. ✅ Error condition occurs (reproduced).

Additional Notes

  • @afercia it seems that your last patch is not applying any more, if you are still around can you check it when possible?

This ticket was mentioned in PR #12869 on WordPress/wordpress-develop by @arkaprabhachowdhury.


5 weeks ago
#25

  • Keywords has-patch has-unit-tests added; needs-refresh removed

Fixes #25696\n\n## What changed\n- Ignore repeated post and taxonomy Quick Edit saves while the current AJAX request is active.\n- Clear the guard when the request completes, including failed requests.\n- Add QUnit coverage for both editors.\n\n## Why\nDouble-clicking Quick Edit can submit duplicate requests; the first response removes the inline-edit row and the second response then operates on missing markup.\n\n## Validation\n- Node syntax checks passed for the changed JavaScript.\n- git diff --check passed.\n- PHPCS/PHPCBF and JS lint could not run because this clean worktree lacks installed vendor/node tool dependencies.\n\nTrac: https://core.trac.wordpress.org/ticket/25696

@arkaprabhachowdhury commented on PR #12869:


5 weeks ago
#26

@irozum Thanks for the review. I added the requested @property {boolean} saving entry to the top-level inlineEditPost namespace docblock and pushed it in commit 3e3dcd3e73.

#27 @arkaprabhachowdhury
4 weeks ago

Opened [PR #12869](https://github.com/WordPress/wordpress-develop/pull/12869) to address this ticket. It prevents duplicate Quick Edit save requests for posts and taxonomies while an AJAX request is active, clears the guard when the request completes—including failed requests—and adds QUnit coverage for both editors.

I picked this up because the ticket’s double-click reproduction still maps to a current client-side race: the first response removes the inline-edit row, while the second response then operates on missing markup. This PR is submitted for code review, with the existing ticket discussion remaining here.

Note: See TracTickets for help on using tickets.