Fix: Empty images around equations render as question marks on Safari… #7019
Conversation
Codebreaker42
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz,
ivailop7,
Sahejkm and
potatowagon
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hi @Codebreaker42! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
size-limit report 📦
|
facebook-github-bot
added
the
CLA Signed
| // We use an empty image tag either side to ensure Android doesn't try and compose from the | ||
| // inner text from Katex. There didn't seem to be any other way of making this work, | ||
| // without having a physical space. | ||
| <> | ||
| <img src="#" alt="" /> | ||
| {/* Add empty <img> tags only if the browser is not Safari */} | ||
| {!isSafari && <img src="#" alt="" />} |
There was a problem hiding this comment.
I think it makes sense to leave some comments about why the img tags are there in the first place, otherwise this looks like a reasonable enough approach
| @@ -21,12 +22,15 @@ export default function KatexRenderer({ | |||
| }>): JSX.Element { | |||
| const katexElementRef = useRef(null); | |||
|
|
|||
| // Detect if the browser is Safari | |||
| const isSafari = /^((?!chrome|android).)*safari/i.test(navigator.userAgent); | |||
There was a problem hiding this comment.
This may cause some issues with SSR, so it might be better to do this with state and a layout effect or something like that, although I'm not sure if this component is compatible with SSR in the first place. Risk is fairly low since this is not a published component.
There was a problem hiding this comment.
Thank you for the feedback! You're absolutely right that directly using navigator.userAgent in the component could cause issues with SSR. I've updated the implementation to detect if the browser is Safari using a useEffect hook and state. This ensures the detection logic runs only on the client side, mitigating potential SSR issues.
|
It looks like integrity checks are failing, you can run these locally with |
Thank you for pointing that out! I'll run the integrity checks locally using npm run ci-check to identify the issue. If it's a formatting issue, I'll fix it using npm run prettier:fix as suggested. I'll update the pull request once everything passes. Thanks again for the guidance! |
…to pass the CI checks
|
This was tested in the preview app from the latest commit https://lexical-playground-9mxzhz6fh-fbopensource.vercel.app |
[lexical-playground] Bug Fix: Remove empty images around equations causing question marks in Safari (#6824)
Description
Current Behavior
Equations rendered using the
KatexRenderercomponent include empty<img>tags as placeholders, which appear as question marks in Safari due to Safari’s handling of empty images.Changes Made
<img>tags around the equation rendering inKatexRenderercomponent.Closes #6824
Closes #6836
Test plan
Before
In Safari:
Rendered incorrectly due to
<img>placeholders.Screenshots/recordings showing the issue:
!Before Screenshot
After
In Safari:
<img>tags.Screenshots/recordings showing the fix:
!After Screenshot