1718982 - ESC key should not save modified data in `Edit bookmark` and `Edit bookmark folder` dialog

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect, P2)

Description

[Alice0775 White]

Reproducible: yes, Reproduced since Firefox 89

#1 Steps To Reproduce:

1. Right click on a bookmark. And choose Edit Bookmark... to open Edit Bookmark dialog
2. Click URL field and edit something(but valid url format)
3. Press ESC key to dismiss the dialog

#2 Steps To Reproduce

1. Right click on a bookmark folder. And choose Rename Folder... to open Edit bookmark folder dialog
2. Edit something
3. Press ESC key to dismiss the dialog

Actual results:
#1 The bookmark url is modified.
#2 The name of folder is modified.

Expected Results:
All changes should be abandoned.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

:alice0775, could you try to find a regression range using for example mozregression?

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]: unexpected data loss

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?tochange=16e0111f7c6d09f298dcafe0a79007391ba454fd&fromchange=158c17acab625c265019865ac6f33b2c415b37f7

Comment 3

[Alice0775 White]

Regression window(with forcibly enabled proton and proton modals):
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=84a8d70fe0be728adf4ae0559312b354d2bb0acc&tochange=8040add982c0c282025a03270a0f99fa375064c7

Comment 4

[:Gijs (he/him)]

Marco, I think this happens because the code at https://searchfox.org/mozilla-central/rev/77256682a1fe0e73c5ad34c593b1ecacfefdf4e1/browser/components/places/PlacesUIUtils.jsm#319,340 fails the topUndoEntry check, that is, the pre-dialog-shown and post-dialog-shown values are the same, when we use gDialogBox to show the dialog. This is not the case when opening the dialog as a modal window.

I don't have the foggiest idea why that might be the case, because I don't know how topUndoEntry is meant to work. Can you help?

I did notice something else weird: the steps from comment 0:

(In reply to Alice0775 White from comment #0)

1. Right click on a bookmark. And choose Edit Bookmark... to open Edit Bookmark dialog
2. Click URL field and edit something(bat valid url format)
3. Press ESC key to dismiss the dialog

commit the change to the URL field. But if between 2 and 3 I first focus a different field (and don't edit that), and then hit Esc to dismiss the dialog, things work (ie the change gets cancelled). So this feels like some kind of race between the cancellation code and a change event in the dialog being processed. However, when I add logging in the change handler and after the await for the dialog returns, the change event definitely fires before we hit the logging in PlacesUIUtils.jsm which indicates the dialog has stopped being open. So I don't understand where the delay is coming from.

I also tried to add logging for when the transaction finished by adding a .then() to the transaction handler in editBookmark.js' onLocationFieldChange, but that appears to never log, perhaps because the array of transaction promises gets cleared up and the code gets garbage collected? I'm not sure.

Comment 5

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #4)

Marco, I think this happens because the code at https://searchfox.org/mozilla-central/rev/77256682a1fe0e73c5ad34c593b1ecacfefdf4e1/browser/components/places/PlacesUIUtils.jsm#319,340 fails the topUndoEntry check, that is, the pre-dialog-shown and post-dialog-shown values are the same, when we use gDialogBox to show the dialog. This is not the case when opening the dialog as a modal window.

I'll take this a bit wider to add knowledge to the bug. The batching as described in the code above exists because the dialog is instant-apply, in practice we immediately apply changes to the UI when you make changes in the dialog. That creates quite some challenge because any other change can happen in the meanwhile and when the dialog is closed we end up undoing not just the changes in the dialog but also other concurrent changes. From some time we'd like to remove the instant-apply system and go back to an apply-on-confirm approach.

topUndoEntry is basically checking that some change happened before invoking undo. Why? Because if I open the dialog and close it without changing anything and I undo, I will undo whatever bookmark change I made before opening the dialog.

So, apparently by the time this code is now invoked the changes didn't happen yet... That seems understandable, the previous code was blocked in the events loop by the modal dialog until the changes were committed, the new code probably is not?
Before invoking PlacesTransactions.undo() we should ensure that whatever async change happened in the dialog is done, and here I don't see any await.
one could probably await Promise.all(gEditItemOverlay.transactionPromises); for the dialog pane... I didn't check but it sounds plausible, that array contains all the promises for the pending changes, and undo should happen after those.

Comment 6

[Marco Bonardo [:mak]]

I wonder if a similar trick to https://searchfox.org/mozilla-central/rev/352b525ab841278cd9b3098343f655ef85933544/browser/base/content/browser-places.js#368-372 could be applied here

Comment 7

[Alice0775 White]

FWIW, If you click the "Cancel" button instead of pressing the "ESC" key, the changes will be undone as expected.

Comment 8

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #5)

one could probably await Promise.all(gEditItemOverlay.transactionPromises); for the dialog pane... I didn't check but it sounds plausible, that array contains all the promises for the pending changes, and undo should happen after those.

The uninit code clears out the array though - https://searchfox.org/mozilla-central/rev/77095b434e16d81c2e43e71a120c61b08e365e3a/browser/components/places/content/editBookmark.js#579 . So I don't think that'll help out of the box. Do you know why the clearing happens, ie could we clear it but stash (a Promise.all of) the previous array somewhere else so the batch code can await it?

(In reply to Alice0775 White from comment #7)

FWIW, If you click the "Cancel" button instead of pressing the "ESC" key, the changes will be undone as expected.

Right - I suspect this is because the html:dialog we use for the implementation catches the "ESC" key and treats it as a request to close the dialog, which will hide it, which will have a different event ordering (of the blur/change of the input field vs. the event loop returning to the code I linked to in comment #4) than closing by clicking the button. Though if this is really about whether the transaction gets committed, the code is fragile either way. :-(

Comment 9

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #8)

The uninit code clears out the array though - https://searchfox.org/mozilla-central/rev/77095b434e16d81c2e43e71a120c61b08e365e3a/browser/components/places/content/editBookmark.js#579 .

Yeah, that sounds like a mistake, though even if we'd not clear it, an immediate initPanel would do and we'd be back to the same problem... not clearing on uninit and making initPanel async would help, but then all consumers would be visibly delayed (like the Library pane). We could not clear it on uninit anyway, that should not create any issue. In reality uninit should be an async operation, but again it would delay moving to the next item in the Library after a change.
Can we return a copy of the transactions array when it hides, before the panel is uninited, similarly to https://searchfox.org/mozilla-central/rev/a9db89754fb507254cb8422e5a00af7c10d98264/browser/components/places/content/bookmarkProperties.js#429, then await those promises here https://searchfox.org/mozilla-central/rev/a9db89754fb507254cb8422e5a00af7c10d98264/browser/components/places/PlacesUIUtils.jsm#336

This is fragile also because these panels were all designed with synchronous APIs in mind and then adapted later but without actually changing the design.

Comment 12

[Abraham Alif]

i feel like it still occurs to sometime

https://www.baperanews.com/
https://sites.google.com/view/berita-terbaru-viral-dan-terki/home

Comment 13

[Mark Banner (:standard8)]

This appears to have been resolved, probably by the recent changes in bug 1744551 to change how the dialog worked.