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

fix(structure): annotations not opening in portable text editor #6198

Merged
merged 4 commits into from
Apr 3, 2024

Conversation

pedrobonamin
Copy link
Contributor

@pedrobonamin pedrobonamin commented Apr 3, 2024

With the changes pushed in #6129 a bug was introduced into the PTE editor.
Annotations wont open.

The issue is generated by the onBlur action, when it's triggered, it's setting as openPath the blurred path.
This specific change was introduced in that PR, possibly an oversight?

Description

What to review

Testing

Notes for release

Fixes an issue in PTE in which annotations were not opening by a bug introduced in v3.36.3

@pedrobonamin pedrobonamin requested a review from rdunk April 3, 2024 13:59
@pedrobonamin pedrobonamin requested a review from a team as a code owner April 3, 2024 13:59
@pedrobonamin pedrobonamin requested review from binoy14 and removed request for a team April 3, 2024 13:59
Copy link

vercel bot commented Apr 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Apr 3, 2024 4:17pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 3, 2024 4:17pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Apr 3, 2024 4:17pm

Copy link
Contributor

github-actions bot commented Apr 3, 2024

No changes to documentation

Copy link
Contributor

github-actions bot commented Apr 3, 2024

Component Testing Report Updated Apr 3, 2024 4:19 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 32s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 6s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 26s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 12s 4 2 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 12s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 31s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 0s 14 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 1s 18 0 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 11s 6 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 13s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 19s 9 0 0

@rdunk
Copy link
Member

rdunk commented Apr 3, 2024

Obviously lower priority than annotations, but just noting that this will reintroduce the scroll jumping issue with PTE in Presentation.

@skogsmaskin
Copy link
Member

skogsmaskin commented Apr 3, 2024

I just commented here on this.

Why do we open the blurred path @rdunk ?

This is why the edit annotation thing fails. The text node is "opened", and then the "markDef" (annotation) path is being closed (we don't allow multiple path trees being opened at once).

@skogsmaskin
Copy link
Member

Obviously lower priority than annotations, but just noting that this will reintroduce the scroll jumping issue with PTE in Presentation.

@rdunk - Could this scroll jumping perhaps be fixed by moving these lines down to line 59 ?

image

@pedrobonamin pedrobonamin requested a review from a team as a code owner April 3, 2024 16:07
@skogsmaskin
Copy link
Member

I added a component test for this that will fail with the problematic code.

@rdunk
Copy link
Member

rdunk commented Apr 3, 2024

@skogsmaskin I'll verify that as a workaround, I think we tested that before but I'll double check.

Done some digging and looks like I didn't catch this as our test document didn't contain an Annotation, only InlineBlock.

This line, which is unique to Annotation's onOpen callback means that the blur is triggered after the DocumentPane's onPathOpen/setOpenPath is called, for all the other PT objects, the blur is triggered before.

@rdunk
Copy link
Member

rdunk commented Apr 3, 2024

@rdunk - Could this scroll jumping perhaps be fixed by moving these lines down to line 59 ?

Looks like that doesn't solve it unfortunately.

@pedrobonamin
Copy link
Contributor Author

@rdunk I think regardless of the final fix that is needed for presentation to work, we shouldn't be setting the path to the path that was blurred, it sounds like a contradictory behaviour from this perspective. This change fixes the issue, I think we should deploy a new version with this fix today.

@rdunk
Copy link
Member

rdunk commented Apr 3, 2024

Agreed with merging this as this bug is much worse than the scroll issue.

The problem is if an initial openPath is defined that references some block in the PTE, without setting the openPath on blur, the openPath is maintained as its initial value, which is incorrect. It's not the focus path we are setting to the blurred path, just the openPath, which AFAICT is correct.

I think ultimately the bug isn't caused by setting that openPath on blur, it is caused by the Annotation triggering the blur event after it sets the openPath, which resets it.

@rexxars rexxars changed the title fix(structure): annotations not opening in PTE fix(structure): annotations not opening in portable text editor Apr 3, 2024
@pedrobonamin pedrobonamin added this pull request to the merge queue Apr 3, 2024
Merged via the queue into next with commit 907c904 Apr 3, 2024
36 checks passed
@pedrobonamin pedrobonamin deleted the edx-1231 branch April 3, 2024 17:20
rexxars pushed a commit that referenced this pull request Apr 3, 2024
* fix(structure): annotations not opening in PTE, blurred path is set as open path

* test(form/inputs): add test ids for annotation toolbar popover

* test(playwright-ct): add test for editing existing annotation

---------

Co-authored-by: Per-Kristian Nordnes <per.kristian.nordnes@gmail.com>
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.

4 participants