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

Make snippet buttons invisible rather than hidden to prevent issues #479

Merged
merged 4 commits into from
Sep 28, 2024

Conversation

piemonkey
Copy link
Collaborator

@piemonkey piemonkey commented Sep 10, 2024

Overview

On Chrome, clicking the icon de-selected the snippet, which caused the button to disappear and the onClick to never get called. This makes the button transparent instead, which means the onClick still gets called.

connected issues and PRs:

Jira ticket: https://binnenland.atlassian.net/browse/GN-5019
GN PR: lblod/frontend-gelinkt-notuleren#716

Setup

N/A

How to test/reproduce

Seems to only be easily reproducible in GN/RB and on Chrome. After inserting a snippet, the buttons that appear when the snippet is selected are now functional wherever you click on them. Before clicking on the icon would not work. Moving the selection away from the snippet should hide the buttons.

Challenges/uncertainties

Seems like weird timing issues and is very hard to debug.

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check if dummy app is correctly updated
  • Check cancel/go-back flows
  • changelog
  • npm lint
  • no new deprecations

On Chrome, clicking the icon de-selected the snippet, which caused the
button to disappear and the onClick to never get called. This makes the
button transparent instead, which means the onClick still gets called.
Copy link
Collaborator

@elpoelma elpoelma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really able to reproduce the bug which this PR should fix. I'm running chrome 129.0.6668.58 on fedora.

@piemonkey
Copy link
Collaborator Author

@elpoelma Hmm, I was able to reproduce it, but I used the snippet that was used in the video in the Jira ticket, and didn't test extensively with other snippets. Can you try again using that one? If you can't find it, the snippet was a relatively long paragraph of text, long enough to increase the size of the snippet node from the minimum. Perhaps that is a necessary condition for this bug to appear? I used Chromium 128.0.6613.137 on Arch.

@elpoelma
Copy link
Collaborator

elpoelma commented Sep 24, 2024

@piemonkey I tested it with the same 'lorem ipsum' snippet, but still could not reproduce the issue. I'll see if I can try to reproduce it with the previous chrome version, but that would be a big coincidence.

Update: tested it with chromium 128, but still can't reproduce the issue

@abeforgit
Copy link
Member

abeforgit commented Sep 24, 2024

I can easily reproduce it in dev GN fyi
trigger is indeed text that runs all the way to the buttons, and then clicking exactly on the icon (not next to it):

image

@abeforgit
Copy link
Member

it seems I managed to make the button black in the screenshot, this went away though
I'm pretty sure this is unrelated

Copy link
Member

@abeforgit abeforgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I can still trigger the bug even with these changes

Chromium 128.0.6613.84

  • in GN: make doc with any kind of snippet placeholder
  • insert any snippet
  • fill the snippet with a bunch of text, make sure the text runs right to the end of the line, until it reaches the buttons, e.g.:
    image
  • click any of the icons, same issue as before

@abeforgit
Copy link
Member

The trick seems to be setting contenteditable='false' on the div that's wrapping the buttons. I think it prevents the selectionchange event from firing, which the editor plugs into to update it's selection (which ultimately causes the issue, cause the new selection falls outside of the snippet node, causing the buttons to be deactivated)
@elpoelma do you see any issues with this

@abeforgit
Copy link
Member

@piemonkey bonus fix you can add:
simply add this.controller.focus() to openModal() BEFORE this.showModal = true
this will cause the editor to refocus after you close the modal, allowing one to continue typing.

(the reason you set it before is because the AU modal uses focus-trap, which takes over focus control and gives it back to the last element that was focused before it activated)

@elpoelma
Copy link
Collaborator

The trick seems to be setting contenteditable='false' on the div that's wrapping the buttons. I think it prevents the selectionchange event from firing, which the editor plugs into to update it's selection (which ultimately causes the issue, cause the new selection falls outside of the snippet node, causing the buttons to be deactivated) @elpoelma do you see any issues with this

I think this is the best way to approach this issue. As a rule of thumb, with ember block nodes with content, it's better to only put contenteditable: false on elements that really need it (such as these buttons). You (almost) never want to have nested contenteditable: false > contenteditable: true structures. (inline nodes with content are an exception, as well as any nested prosemirror editors)

@piemonkey
Copy link
Collaborator Author

@abeforgit I've made the button container not contenteditable. I couldn't reproduce the bug with my previous change though, so could you test whether it fixes it for you?

@elpoelma
Copy link
Collaborator

@abeforgit @piemonkey can confirm that the contenteditable: false solution fixes the issue for me.

@abeforgit abeforgit merged commit 4755750 into master Sep 28, 2024
3 checks passed
@abeforgit abeforgit deleted the fix/snippet-buttons branch September 28, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants