Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix quick-repo-deletion not aborting on Firefox #4651

Merged
merged 2 commits into from
Aug 14, 2021
Merged

Fix quick-repo-deletion not aborting on Firefox #4651

merged 2 commits into from
Aug 14, 2021

Conversation

cheap-glitch
Copy link
Member

Fixes #4599

Test URLs

A fork you own or an empty repo with no stars

Screenshot

capture-1628591720.mp4

Turns out the z-index was not the problem, rather it was the controller that wasn't aborted.
Tested on Firefox only

@yakov116 yakov116 added the bug label Aug 10, 2021
@fregante
Copy link
Member

fregante commented Aug 10, 2021

Can we also just change it to a global click handler as I suggested in the issue? This should just never happen even if in the future z-index is the issue, because in the #4599’s screenshot it definitely is.

@cheap-glitch
Copy link
Member Author

cheap-glitch commented Aug 11, 2021

Can we also just change it to a global click handler as I suggested in the issue?

👍

Edit: done and tested both with a normal and a broken z-index

because in the #4599’s screenshot it definitely is.

No, it's just what happens when the <details> element is closed but the controller isn't aborted (because the button style isn't reset properly).

capture-1628684186.mp4

@fregante
Copy link
Member

fregante commented Aug 12, 2021

I think we should preserve the previous toggle/abort state sync. The "close on click anywhere" should be on top of that.

Now I see these problems:

  • if the user lets it complete, the first click after completion will be ignored, even if they click on the menu
  • if the overlay is closed in any way other than click (Enter, esc? Whatever closes the detail), the click might not be caught

The code might not end up super clean since we need several listeners all two-way-linked to an AbortController, but this feature is something we need to take extra care of, perhaps even adding an additional "press any key to stop" listener.

@fregante fregante marked this pull request as draft August 13, 2021 10:39
@fregante fregante marked this pull request as ready for review August 14, 2021 03:07
@fregante fregante changed the title Fix quick-repo-deletion not aborting Fix quick-repo-deletion not aborting on Firefox Aug 14, 2021
@fregante fregante merged commit e962ab6 into refined-github:main Aug 14, 2021
@fregante
Copy link
Member

This probably still works better than before. We can publish this in the upcoming version and improve it afterwards.

@fregante fregante added the Please! ♥︎ Particularly useful features that everyone would love! label Aug 14, 2021
@cheap-glitch cheap-glitch deleted the fix-quick-repo-deletion branch August 14, 2021 07:09
kidonng pushed a commit to kidonng/refined-github that referenced this pull request Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Please! ♥︎ Particularly useful features that everyone would love!
Development

Successfully merging this pull request may close these issues.

Aborting after clicking "Delete fork" not possible
3 participants