Fix toolbars that shouldn't appear in certain cases #4077
Conversation
WarrenLee19
requested review from
trueadm,
zurfyx,
fantactuka,
acywatson,
tylerjbainbridge and
thegreatercurve
as code owners
|
Hi @WarrenLee19! 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 cla@meta.com. Thanks! |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Can you revise the 8 failing checks please? |
|
I'm going to modify these eight |
…exical into fix-toolbar-by-lee
|
@zurfyx Hi, I have modified the judgment conditions. You can reprocess my pr. thanks |
|
Can you double check TextFormatting.spec.mjs? Seems like the tests in this file are failing consistently You can run |
|
ok, let me try |
|
|
|
@zurfyx hi, fixed it in TextFormatting.spec.mjs, please reprocess |
| if ($isRangeSelection(selection) && !selection.isCollapsed()) { | ||
| const anchorOffset = selection.anchor.offset; | ||
| const focusOffset = selection.focus.offset; | ||
| if ( | ||
| selection.anchor.key !== selection.focus.key && | ||
| selection.dirty && | ||
| focusOffset === 0 && | ||
| anchorOffset > 0 | ||
| ) { | ||
| setIsText(false); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
The problem with the current solution is that when your select spans multiple characers, it gets removed as soon as you take the first action. It should remain around.
Additionally, we probably want to handle this case in a more generic manner. We know that this happens when the selection is not collapsed and when there's no content.
Unfortunately, the selection.getTextContent will includes new lines spacers in the output, that's intentional because this function is usually used to export plain text content. We can either rebuild this or assume that this is one only edge case for now and run a simple regex to drop it. I'd probably go for the easy path for now and we can revisit this later since it's quite involved (if it makes any sense to invest in this at all).
const rawTextContent = selection.getTextContent().replace(/\n/g, '');
if (!selection.isCollapsed() && rawTextContent === '') {
setIsText(false);
return;
}
…on and reset the test file）
|
@zurfyx hi, i has been modified based on your method and reset the TextFormatting.spec.mjs. thanks |
|
Thank you Warren! |
For example,
when the number of lines in the editor is more than 2,
gently double-click the end of the line, a toolbar will appear, and this toolbar will not process any text content.
In this case, it is obviously unnecessary to display the toolbar.
2023-03-09.09.38.27.mov