-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add Safari workaround inside shadow DOM. #5648
Add Safari workaround inside shadow DOM. #5648
Conversation
🦋 Changeset detectedLatest commit: 346ec53 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MahmoudElsayad, I've added a few minor comments/questions. Also please add a changeset per the PR instructions, a minor release is fine.
const el = ReactEditor.toDOMNode(editor, editor) | ||
const root = el.getRootNode() | ||
const safariVersion = navigator.userAgent.match(/Version\/(\d+\.\d+)/) | ||
const isSafariVersionLessThan17 = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New browser version checks should live inside slate-react/src/utils/environment.ts so they're easy to find/consist/reused and then imported. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in c6dbf93.
const root = el.getRootNode() | ||
const safariVersion = navigator.userAgent.match(/Version\/(\d+\.\d+)/) | ||
const isSafariVersionLessThan17 = | ||
IS_WEBKIT && safariVersion && parseFloat(safariVersion[1]) < 17.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in c6dbf93.
IS_WEBKIT && | ||
root instanceof ShadowRoot | ||
) { | ||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is ignore needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover aded41d
attributes, | ||
children, | ||
}: RenderPlaceholderProps) => ( | ||
attributes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure this linting change was intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, it wasn't intentional. Corrected here -> aded41d
let active = document.activeElement | ||
|
||
// eslint-disable-next-line no-constant-condition | ||
while (true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why while(true)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is for allowing deep traversal through nested shadow roots to find the actual active element. I think b1710e0 version is better as it places the continuation conditions in the loop to make it visible and understandable.
ok, it's close... prettier is still complaining. I can look at it soon if you don't have time. |
Description
Safari doesn't support access for selection inside shadow root, this PR implements a workaround that will be applied only for Safari inside shadow root to improve the editor experience inside Safari.
Issue
Fixes: #5144 #5321
Example
Context
This PR introduces a workaround for the absence of the getSelection method in Safari versions prior to 17 when used inside a shadow DOM, which causes the behavior in the example above. By implementing a workaround, this fix ensures consistent behavior.
Checks
yarn test
.yarn lint
. (Fix errors withyarn fix
.)yarn start
.)yarn changeset add
.)