ThoughtAnnotation: calculate position based on selection range #3357
Conversation
|
Looking at the failing tests, I see an example that I didn't think was possible. 
An annotation on an empty thought is going to cause trouble because there's no text to use for the selection range. The old version used the Since empty thoughts shouldn't be able to be multiline, hopefully it's possible to use the bounds of the editable itself instead of setting the selection. |
Looks good. Thanks for keeping track of the different elements that rely on the ThoughtAnnotation. Flag anything that ends up looking intractable!
Since this is such an edge case, I would also be okay simply hiding the url icon on empty thoughts. Seems easier than branching the measurement logic. |
|
I've been working on getting the snapshot tests to pass by tweaking the annotation positioning, but I've come across something that might be a bit of a blocker. 
Since the long link is ellipsized, the client rect of the selection range ends up being well off-screen, 1325px in this case. If you want, I could compare the x position to the viewport width and use the right edge of the editable's bounding box in that case. |
|
Yes, that would be great. |
|
By tweaking the positioning carefully, I got all but one of the Puppeteer tests to pass. If you're OK with this discrepancy, then we're good to go: 
Something started happening where the non-Puppeteer tests started failing, mostly because It looked like faux caret positioning was pretty good by default in the new version. I wasn't sure if what I was seeing was correct, and then I decided to comment out all faux caret code and see how it looked with the native caret. It looks like Apple may have mostly fixed the caret drift! Here's a video from Video.48.movAssuming that I did all of that correctly, it looks like it wiggles a bit but is on par with our faux caret solution, seen below for comparison: Video.49.mov |
It behaves so similarly to the faux caret that I have a sneaking suspicion that I might wake up tomorrow and realize that it is actually our faux caret, just styled slightly differently. We shall see. |
Yes, that's fine. Thanks!
JSDOM shims selection behavior (not mocked) so we can normally rely on window.getSelection as usual (example). But JSDOM does not do paint/layout, so
Amazing! I love deleting code! 😈 Based on some initial tests, I still see some discrepancies with the native behavior sadly, though it has improved greatly. If you could create a PR with the |
|
I fixed the positioning of the start-of-thought faux caret, and I think this is ready for review. |
src/components/ThoughtAnnotation.tsx
Outdated
|
|
||
| // We're trying to get rid of contentWidth as part of #3369, but currently it's the easiest reactive proxy for a viewport resize event. | ||
| const contentWidth = viewportStore.useSelector(state => state.contentWidth) | ||
| const cursor = useSelector(state => state.cursor) |
There was a problem hiding this comment.
I want to check in about the potential performance implications of this. This would cause every visible ThoughtAnnotation instance to re-render whenever the cursor changes. Does this differe from main? Can this be narrowed down?
There was a problem hiding this comment.
Yes, thanks, I think I can re-use isEditing to re-render the annotations when a URL ellipsizes instead of doing it every time the cursor changes.
| <span | ||
| className={css( | ||
| { | ||
| visibility: 'hidden', |
There was a problem hiding this comment.
I believe the goal would be to remove the hidden <span> entirely since it is no longer used to position the annotations. Is this possible, or are there other things that depend on it?
If it is still required, maybe we can put together a rough plan of what it would take to remove it.
I'm also open to doing this in another PR if having an extra step in the commit history after merge would be helpful.
There was a problem hiding this comment.
If I remove the span, then it messes up the superscripts:
I thought it might be as simple as adding an explicit height to the container, such as height: multiline ? '1.25em' : '2em'. That did fix the superscripts, but it caused other layout problems:
I went ahead and removed the textMarkup and placeholder props so that it will only ever render '​'. In terms of removing the span entirely, I think that there are two moving parts:
- Setting the height of the container correctly so that it leaves room for annotations without pushing lower thoughts out of place
- Figuring out if anything breaks now that we are relying on typographic CSS such as verticalAlign: 'text-top' while no longer including any text in the annotation.
I wouldn't mind doing this in a separate PR so that we can get this one merged, but it's up to you.
|
Thanks. I'm bringing the discussion here as there seems to be more at stake than originally anticipated.
This is giving me some doubts about the current solution and whether I want to merge. The main point of this task was to remove the dependency on the the invisible placeholder element and reduce complexity. Now we have a hybrid solution which relies partly on DOM measurement and partly on the placeholder, which seems to me less good than either pure approach. We began with the theory that the superscript can be rendered a fixed number of em up from the bottom-right corner of the editable, without any placeholders. Is this not true? Maybe better understanding the path to true decoupling from the placeholder will help us get to the right solution. |
|
Actually, I realized that I can use |
There was a problem hiding this comment.
I can confirm that Edited thought superscript position incorrect on first paint
is fixed.
I'm still seeing the broken behavior after toggle context view: Deactivated context view superscript position incorrect on first paint.
The test case from #3357 (comment) still looks solid.
It seems like when virtualized thoughts re-enter the viewport after the context view thoughts leave, they come back with the annotation in the wrong position. I'm not sure why that would be the case, but hiding the annotation until positioning occurs seems to reliably fix the issue. |
|
@fbmcipher Could you do a performance comparison between this and I recommend a test case that expands a context of 50–100 thoughts with superscripts. |
|
@raineorshine Sure thing – I'll run a couple quick tests and post my results here tomorrow. |
raineorshine
left a comment
raineorshine
left a comment
There was a problem hiding this comment.
I can confirm that Edited thought superscript position incorrect on first paint is fixed.
I'm still seeing the broken behavior after toggle context view: Deactivated context view superscript position incorrect on first paint.
The test case from #3357 (comment) still looks solid.It seems like when virtualized thoughts re-enter the viewport after the context view thoughts leave, they come back with the annotation in the wrong position. I'm not sure why that would be the case, but hiding the annotation until positioning occurs seems to reliably fix the issue.
Thanks! That's better.
A few things I found:
- It looks like some superscripts are being hidden that don't need to be hidden. It should only affect children that are being animated in/out, not the cursor thought, siblings, or ancestors.
- Could you add a fade in to the superscripts? I'd like to smooth out the transition so it's a little less abrupt.
- I found one other case that needs to be accounted for. This reminds me of all the cases that needed to be identified for useMultiline. Anything that changes the position or width of the thought can potentially invalidate the superscript position. Maybe we can borrow from
useMultilineand even abstract out the shared elements?
It's too bad that ResizeObserver is too slow to use for every visible thought, as the ad hoc React dependencies are pretty error prone.
Steps to Reproduce
- x
- =children
- =pin
- true
- a
- This is a longer thought that will extend onto two lines if I keep typing and it may have a superscript at any point.
- y
- a
- This is a longer thought that will extend onto two lines if I keep typing and it may have a superscript at any point.
- Set the cursor on
x/a. - Activate Table View
Current Behavior
The superscript of the long thought has the wrong position.
Expected Behavior
The superscript position should be updated correctly.
It seems like the context animation is applied to all thoughts, not just ones that are entering or leaving the context view. Probably because it's tough to determine which thoughts are animating out and leaving the DOM. Do you think it's worthwhile to try to narrow that down so that the animation is not applied to thoughts that are higher up than the cursor thought? |
|
@raineorshine Please find a description of the performance test I ran, along with the results below. Steps to ReproduceExpand a context of 50-100 thoughts with superscripts.
Test Case
Method
Results
Observations
My conclusionThough there is admittedly a weakness in this test (the slow Context View animation creates undesirable fluctuation in our results), even excluding the noticeable outlier the time taken for the layout to settle after triggering Context View is measurably faster in this PR than in This provides reasonable confidence that the PR does improve performance in practice. |
It seems like using belowCursor works pretty well to narrow down which annotations are recalculated, apart from some unpleasant prop drilling. However, there are still thoughts down below that shouldn't recalculate, as you can see if you start from the repro in #3357 (comment) and toggle the context view on Recording.2025-12-31.145250.mp4 |
I think it should be all set now. Instead of tracking |
raineorshine
left a comment
raineorshine
left a comment
There was a problem hiding this comment.
After playing with this a bit, I unfortunately have come to the conclusion that rendering the superscripts after a delay is too disruptive to the UX. This is true whether the superscripts snap in or fade in smoothly.
Theoretically it's possible to measure the bounding box synchronously in useLayoutEffect and render the superscript before the first paint, though it will have a performance impact. I'd love to get your input on how viable this might be before we spend additional time.
I did another review below, but they are moot points if we're unable to efficiently render the superscript synchronously.
Thanks so much for your work.
I'm still seeing the issue described at the end of this comment. Are you able to reproduce? #3357 (review)
Note 1: This was my interpretation of the test based on the instructions given and the fact that the
positionAnnotationcallback executes after the Context View animation completes. Let me know if I got it wrong.
I don't think this changes the results necessarily, but I meant a simple cursor navigation in normal view that expands a context. Activating the Context View has its own performance issues, so I would avoid that and test superscript rendering without the context view animation. Sorry for the confusion!
- Measure the time from tapping the Context View icon to the point where layout fully settles, using frame-by-frame timings and snapshots taken by the Chrome Performance Profiler.
I'm not sure that measuring the time in between the start and finish really measures performance. That's just measuring animation duration, which should generally be constant. If performance drops, the browser will drop frames without changing the animation duration. Only if the CPU is thrashing will the animation actually slow down. I think we need to be measuring either the CPU usage in ms or the average frame rate, ideally without the confounding variable of the Context View animation.
You're welcome, it was a worthwhile effort nonetheless.
I can test fairly quickly tomorrow, but I have a few concerns:
|
|
Oops, I meant selection range, not bounding box! The question is whether it's possible or not to use the same technique that we are using here, but before the first paint. |
I updated it to Recording.2026-01-02.104150.mp4I verified that changing the animation duration to 0 fixes it, as does removing the It also seems like the bounding box returned by editableRef.current.getBoundingClientRect() is also moving. Recording.2026-01-02.105041.mp4In this case, when I set the animation duration to 0, it jumps from a different initial position to the new position. If I remove |
|
It seems like there are too many issues for this to be a viable approach. This was a costly experiment, but I guess we couldn't have known until we tried. Thank you for your effort. |
4 participants
Fixes #3352
As discussed, getting the bounding rect of a selection range seems to work well for positioning the thought annotation without duplicating the thought in an invisible container.
TODO:
get the positioning right for superscripts, urls, emailget the positioning right for the end-of-thought faux caretcreate a separate selection to position the start-of-thought faux caret correctlyget editable's bounding rect for empty thoughts with annotationsI remember that we had an issue when we were building the faux caret where the end-of-thought position was being reported inaccurately by iOS Safari. This may also impact the positioning of superscript, which would mean we need to update some snapshots. It most likely won't create a visual discrepancy in the same way that replacing the real caret with the faux caret often does.