Fix URL link hidden by autofocus is still clickable

[codebuild-ro]

Fix: #3245

Remove pointerEvent: "none" from ThoughtAnnotationWrapper.
This way is fine, because of the rendering order.
So first ThoughtAnnotationWrapper is rendered and after that, ThoughtAnnotation is rendered. So ThoughtAnnotation overlaps the ThoughtAnnotationWrapper.
Before, I had mistake to understand the rendering order.

So there is no need to add property pointerEvent: "all" to icon components.

[raineorshine]

Hi, thanks for your submission.

This solves the problem, but I'm not sure yet if this is the best solution.

First, there is a bit of forensics that is needed to understand the current use of pointerEvents: all. What purpose does it serve? When was it added and why? Is it overriding an inherited value? The default value is auto so it's not obvious why it would need to be set to all. We always make sure to understand the cause of the problem first before attempting a fix. Git blame will help here.

Second, the assignment of responsibility seems wrong here. Why should UrlIconLink have to know whether it is hidden or not? If an ancestor controls its visibility, it seems like the ancestor should be responsible for this.

[codebuild-ro]

I removed this to reduce the depth of prop drilling of isVisible property.
Instead, I used that pointerEvents: 'all' in UrlIconLink.

※※※※※※※※※※※※※※※※※※※※※※※※※※※※※※※

em/src/components/ThoughtAnnotationWrapper.tsx

Line 45 in ab158c2

pointerEvents: 'none',

Please notice that Subthought has pointerEvents: 'none'

So the children inside ThoughtAnnotationWrapper cannot be clickable. To solve that, the previous developer added this property - pointerEvents: 'all' (to override inherited value).

em/src/components/ThoughtAnnotation.tsx

Lines 162 to 165 in ab158c2

	{
	// do not render url icon on root thoughts in publish mode
	url && !(publishMode() && simplePath.length === 1) && <UrlIconLink url={url} />
	}

[codebuild-ro]

UrlIconLink has UrlIcon as a child.
In UrlIcon component, there is a style pointerEvents: 'all'.
I extracted out pointerEvents property from that component to reduce the depth of prop drilling of isVisible.

This property is calculated here. And it is drilled down to children deeply (in original approach).

em/src/components/Subthought.tsx

Line 67 in ab158c2

const isVisible = zoomCursor || autofocus === 'show' || autofocus === 'dim'

[raineorshine]

Still, why should UrlIconLink have to know if it is visible or not? Components are not usually made aware of ancestor responsibilities.

The solution is not there yet. You will need to either find a way to remove the pointerEvents override mess, or add comments in the code that clarify their purpose and the implicit dependencies that exist. Just because the previous developer didn't document it properly doesn't take us off the hook.

In other cases we use PandaCSS to provide more typesafe overriding of inherited styles, e.g. in https://github.com/cybersemics/em/blob/main/src/recipes/toolbarPointerEvents.ts.

[raineorshine]

Hi, thanks for your submission. Is there any chance there is a simpler way to do this? The PandaCSS recipe does provide some type safety, but now we have a double override. Plus, & > * is a child selector, which creates a type unsafe connection to the DOM hierarchy. We typically try to avoid selectors like this.

I would like to think that a more elegant solution exists. Let me know if you have any other ideas!

It might also be worth investigating the original reason why pointerEvents: none was set on the ThoughtAnnotation. Maybe there is a different way to achieve that, and avoid the need for awkward overrides.

[raineorshine]

@codebuild-ro

[codebuild-ro]

Hi, thanks for your submission. Is there any chance there is a simpler way to do this? The PandaCSS recipe does provide some type safety, but now we have a double override. Plus, & > * is a child selector, which creates a type unsafe connection to the DOM hierarchy. We typically try to avoid selectors like this.

I would like to think that a more elegant solution exists. Let me know if you have any other ideas!

It might also be worth investigating the original reason why pointerEvents: none was set on the ThoughtAnnotation. Maybe there is a different way to achieve that, and avoid the need for awkward overrides.

@raineorshine, We need it now.
Why pointerEvents: 'none' is necessary:

• ThoughtAnnotationWrapper is absolutely positioned and overlays the Editable component
• It renders a hidden clone of the text to position decorative elements (superscript, icons) aligned with the editable text
• Without pointerEvents: 'none', the wrapper would intercept clicks, preventing interaction with the editable text below

We are doing this currently:

• The wrapper has pointerEvents: 'none' to let clicks pass through
• Interactive children (UrlIconLink, EmailIconLink) override with pointerEvents: 'auto' using the annotationPointerEvents({ override: - true }) recipe
• Only 2 components need this override (not excessive)

I have some ideas of alternatives:

1. Wrapping interactive children in a container - it causes layout broken
2. Setting pointerEvents: 'none' only on non-interactive elements - doesn't work because the wrapper div itself would still block clicks
3. Restructuring to avoid overlay - we need major architectural change

So current approach is reasonable. The override pattern is:

• Type-safe (using CVA recipe)
• Consistent with other patterns in the codebase (toolbarPointerEvents)
• Only affects 2 components

[raineorshine]

Why pointerEvents: 'none' is necessary:

• ThoughtAnnotationWrapper is absolutely positioned and overlays the Editable component
• It renders a hidden clone of the text to position decorative elements (superscript, icons) aligned with the editable text
• Without pointerEvents: 'none', the wrapper would intercept clicks, preventing interaction with the editable text below

Just confirming that it is still needed. When I remove pointerEvents: 'none', everything still seems to work. Do you know what I'm missing?

diff --git a/src/components/ThoughtAnnotationWrapper.tsx b/src/components/ThoughtAnnotationWrapper.tsx
index badeadd8be..b39800edc7 100644
--- a/src/components/ThoughtAnnotationWrapper.tsx
+++ b/src/components/ThoughtAnnotationWrapper.tsx
@@ -42,7 +42,6 @@ const ThoughtAnnotationWrapper: FC<
       aria-label='thought-annotation'
       className={css({
         position: 'absolute',
-        pointerEvents: 'none',
         userSelect: 'none',
         boxSizing: 'border-box',
         width: '100%',

So current approach is reasonable. The override pattern is:

• Type-safe (using CVA recipe)
• Consistent with other patterns in the codebase (toolbarPointerEvents)
• Only affects 2 components

That still doesn't address the double override and the child selector, mentioned above. So far I'm not sold on this solution.

[codebuild-ro]

When I remove pointerEvents: 'none', everything still seems to work. Do you know what I'm missing?

Did try to remove pointerEvents: 'none' from ThoughtAnnotationWrapper?

[raineorshine]

Correct

[raineorshine]

@BayuAri Ready for testing!

Since it is not mobile-specific, this requires testing on desktop Chrome only.

[BayuAri]

Issue is still reproducible.
Hidden url is still clickable.

Step to reproduce

1. Create some thought
2. Add a url as a subthought in one of them
3. Click a thought that has no url
4. Click url icon on the thought that has url as subthought from step 2

Current behavior

url is clickable and new browser is opened
See below video:

Hidden.URL.icon.is.stll.clickable.MOV

Expected behavior

Nothing should happen because the url is hidden under