fix pasting of markdown containing links#93
Merged
keithamus merged 7 commits intogithub:mainfrom Mar 21, 2024
Merged
Conversation
susanwere
commented
Feb 23, 2024
susanwere
commented
Feb 23, 2024
keithamus
suggested changes
Mar 6, 2024
Contributor
keithamus
left a comment
keithamus
left a comment
There was a problem hiding this comment.
Thanks for taking the time to make this PR @susanwere!
I've left some comments, because I think we need to go in a slightly different direction here. Let's try to ensure that:
- Any behaviour we add is well tested.
- We don't rely on the user agent to determine which behaviour we use.
Given the above two, I would be happy to re-review this if it:
- Retains the tests that are in place (even if they're altered).
- Adds new tests (to cover the new edge cases we've discovered).
- Removes the user agent sniffing and uses feature detection instead.
If you need any assistance with this please let me know, and I'd be happy to pair.
keithamus
approved these changes
Mar 20, 2024
Contributor
keithamus
left a comment
keithamus
left a comment
There was a problem hiding this comment.
This LGTM 👍 nice work!
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.
👋 This is a second attempt to fix https://github.com/github/issues/issues/5429
After merging this PR in master we realised that the fix didn't solve the issue. I did a second check and found the following issue which causes the bug.
While creating the tree walker, we specified that we only want to show nodes that are elements. This works well in the chrome browser because it generates
spanfor texts that don't have a html tag as shown in the screenshots below. For Firefox, texts that don't have a html tag are skipped because they are not identified as elements thereby causing the bug.The fix is to SHOW_ALL nodes for firefox and only SHOW_ELEMENTs for chrome.
Before
before.mov
After
after.mov