Conversation
Contributor
|
Thanks a lot for your PR. I see the issues this is trying to fix but I'm not yet convinced this is the right fix:
That said I'm open to be convinced otherwise - @mhaack @shsteimer what do you think? |
Contributor
|
Something we were discussing to add regardless is an indicator to show whether the page has been saved. |
Author
|
thanks @hannessolo - I can relate, expectations might vary a lot and probably indicator + a warning are the right middle ground. also, nothing new as noted on #1097, but maybe @mhaack saw something that I didn't |
Author
|
webkit tests fail, so I'll push a fix for that either way so this PR is in consistent state |
commitActiveEditors' test modelled edition as the cell element, but jspreadsheet sets it to [cell, innerHTML, colIndex, rowIndex] while a cell editor is open. closeEditor expects the cell (edition[0]). The wrong model let the code pass the whole array to closeEditor without the test noticing, which is the webkit save-before-navigation failure.
…er nav commitActiveEditors passed the whole edition array to closeEditor, which expects the cell element (edition[0]) and calls edition.getAttribute. Passing the array threw, so beforeNavigate never reached flushPendingSave and the last edit was lost. It only surfaced on webkit: chromium and firefox blur-close the open cell editor before the click handler runs, so commitActiveEditors sees no active edition and the blur-triggered save flushes. Webkit leaves the editor open, so the bad call ran and threw. Passing edition[0] commits the open editor, which fires onafterchanges and lets the pending save flush.
The parent-folder link commits the open editor and flushes the save, then navigates to the parent folder. The test reloaded the sheet immediately after the click, racing that async save+navigation: webkit lost the save, and once the save was awaited, firefox landed on the parent folder instead of the sheet. Waiting for the app to leave the sheet view guarantees the save landed (beforeNavigate awaits it) before reopening the sheet to assert. Verified locally against the sheetfix preview on chromium, firefox and webkit, for both the da-sites/da-status and da-testautomation/da-e2e-tests org/site configs (the two CI suites).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Commit the active spreadsheet cell and flush the pending autosave before navigating to the parent folder. Queued saves retain the sheet path where the edit occurred.
Fixes #1097.
Live test
Edited the final cell, immediately navigated to the parent folder without tabbing out or waiting for the debounce, then reopened the sheet. The edited value was retained.