Use a Popover component for the share annotation floating "bubble" by acelaya · Pull Request #7159 · hypothesis/client

acelaya · 2025-06-12T13:37:50Z

Depends on #7160
Depends on hypothesis/frontend-shared#2000
Depends on hypothesis/frontend-shared#2011

The share annotation "bubble" is currently an absolute-positioned Card. This PR replaces that with a Popover.

popover-arrow-sidebar-2025-06-17_15.43.14.mp4

Some issues found:

~~Popover is displayed underneath the anchor when there's space. We would like to change that behavior.~~ Addressed in Allow to customize initial Popover placement frontend-shared#2000
~~The arrow is not rendered, because the dynamic positioning of the Popover would require the anchor position to be calculated in real time.~~ Addressed in Allow an arrow to be added to popover component frontend-shared#2011

codecov · 2025-06-12T13:42:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.45%. Comparing base (d78dcb6) to head (ad2ef1a).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7159      +/-   ##
==========================================
- Coverage   99.45%   99.45%   -0.01%     
==========================================
  Files         270      270              
  Lines       10958    10955       -3     
  Branches     2623     2621       -2     
==========================================
- Hits        10898    10895       -3     
  Misses         60       60

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

acelaya · 2025-06-17T14:25:03Z

  const toggleSharePanel = () => setOpen(!isOpen);
  const closePanel = () => setOpen(false);

-  usePopoverShouldClose(shareRef, closePanel, { enabled: isOpen });


This is not needed anymore. Everything it does is handled by the Popover component internally.

robertknight · 2025-06-18T14:03:59Z

I noticed a slight rendering glitch where there are white pixels behind the top-left and top-right rounded corners:

I see this in Chrome, Safari and Firefox.

robertknight · 2025-06-18T14:13:18Z

I noticed a slight rendering glitch where there are white pixels behind the top-left and top-right rounded corners:

Setting border-radius: 0.25rem on the outer popover element, to match the inner rounded contents, resolves the issue. I don't know why yet.

acelaya · 2025-06-18T14:21:41Z

I noticed a slight rendering glitch where there are white pixels behind the top-left and top-right rounded corners:

Setting border-radius: 0.25rem on the outer popover element, to match the inner rounded contents, resolves the issue. I don't know why yet.

Good catch. This is because of the changes I did here https://github.com/hypothesis/frontend-shared/pull/2011/files#diff-812511814bb9e545b0977fdf9aac8521d21ec33f5087d9cf4e4f5c518e6b16a1L381-L384

I have found other issue also caused by those changes hypothesis/frontend-shared#2013

acelaya · 2025-06-18T15:10:23Z

I'm going to put this back in draft, as the latest frontend-shared version has a couple regressions in the popover.

I'll get back to this once those are fixed.

acelaya · 2025-06-19T09:25:47Z

Putting this back as ready to review now that Popover regressions are fixed.

robertknight · 2025-06-19T09:52:45Z

I found an issue that the Popover styling is broken when the browser doesn't support native popovers, tested via asNativePopover={false}.

Unfortunately native popovers are still a little too new for us to rely on them being always available.

robertknight

Apart from the issue with non-native popovers, this looks good. I noted that there is a workaround for copy-to-clipboard which is no longer needed and can be removed.

acelaya · 2025-06-19T12:34:03Z

I found an issue that the Popover styling is broken when the browser doesn't support native popovers, tested via asNativePopover={false}.
Unfortunately [native popovers](https://caniuse.com/?search=popover) are still a little too new for us to rely on them being always available.

I have added the classes w-[360px] max-w-[calc(100vw-3.5rem)] to the popover, only when the browser does not support the popover API.

You can test it by applying this diff:

diff --git a/src/sidebar/components/Annotation/AnnotationShareControl.tsx b/src/sidebar/components/Annotation/AnnotationShareControl.tsx
index 7fdd127cb..cc33cd2ba 100644
--- a/src/sidebar/components/Annotation/AnnotationShareControl.tsx
+++ b/src/sidebar/components/Annotation/AnnotationShareControl.tsx
@@ -130,11 +130,12 @@ function AnnotationShareControl({
         align="right"
         placement="above"
         arrow
+        asNativePopover={false}
         classes={classnames({
           // Set explicit width for browsers not supporting native popover API
           'w-[360px] max-w-[calc(100vw-3.5rem)]':
             // eslint-disable-next-line no-prototype-builtins
-            !HTMLElement.prototype.hasOwnProperty('popover'),
+            true,
         })}
       >
         <div className="p-2 flex flex-col gap-y-2">

acelaya · 2025-06-19T12:36:19Z

+          // Set explicit width for browsers not supporting native popover API
+          'w-[360px] max-w-[calc(100vw-3.5rem)]':
+            // eslint-disable-next-line no-prototype-builtins
+            !HTMLElement.prototype.hasOwnProperty('popover'),


When the Popover API is not supported by the browser, it ends up being too narrow. These classes work around it.

non-native-popover-2025-06-19_14.35.27.mp4

See #7159 (comment) for details

Where does the width come from when the native popup API is supported?

The Popover defines this class

The problem is that here the full width is computed based on the common container, which is as wide as the button.

Ugh, sorry, that's for when it is not supported. I misunderstood your question.

This is the right one

Why do we use different values for native and non-native popovers? This inconsistency seems liable to trip us up elsewhere in future.

It's hard to say at this point, as implementing the Select, and then extracting the Popover, required many rounds, and it's possible I overlooked something. It's even likely these classes were required to make it look consistent for that specific use case.

The main difference is that, since the native popover renders in the top layer, relative widths are hard to normalize for both native and non-native popovers, which end up looking different depending on their containers.

The main difference is that, since the native popover renders in the top layer, relative widths are hard to normalize for both native and non-native popovers, which end up looking different depending on their containers.

The current max width for native popovers is expressed in terms of viewport units, so I would have thought it would work the same for native and non-native popovers.

Edit: No sorry, I misread this. It is expressed in terms of the parent width. I presume that "100%" are referring to different things for native and non-native popovers. If we used "100vw" instead, that would work the same in both cases.

If we used "100vw" instead, that would work the same in both cases.

That may very well be the case. I can explore this option.

robertknight

I suggested a small change to set the width using w-max. Otherwise I think we should get this merged. Ideally we could improve Popover in future to minimize behavioral differences.

acelaya force-pushed the share-native-popover branch from ac5ca91 to 4200071 Compare June 12, 2025 13:39

acelaya force-pushed the share-native-popover branch from 4200071 to 3d5ee9b Compare June 13, 2025 07:42

acelaya changed the base branch from main to remove-share-links June 13, 2025 07:42

Base automatically changed from remove-share-links to main June 16, 2025 07:23

acelaya force-pushed the share-native-popover branch 2 times, most recently from 23d3161 to f5beeaf Compare June 17, 2025 14:24

acelaya commented Jun 17, 2025

View reviewed changes

acelaya force-pushed the share-native-popover branch 3 times, most recently from 8f54d92 to a7240d5 Compare June 18, 2025 12:24

acelaya marked this pull request as ready for review June 18, 2025 12:24

acelaya marked this pull request as draft June 18, 2025 12:25

acelaya force-pushed the share-native-popover branch from a7240d5 to f61e2ee Compare June 18, 2025 12:26

acelaya marked this pull request as ready for review June 18, 2025 12:26

acelaya requested a review from robertknight June 18, 2025 12:28

acelaya marked this pull request as draft June 18, 2025 15:10

acelaya mentioned this pull request Jun 19, 2025

Fix regressions introduced in Popover hypothesis/frontend-shared#2014

Merged

acelaya force-pushed the share-native-popover branch from f61e2ee to b92f5f8 Compare June 19, 2025 09:25

acelaya marked this pull request as ready for review June 19, 2025 09:25

robertknight reviewed Jun 19, 2025

View reviewed changes

Comment thread src/sidebar/components/Annotation/AnnotationShareControl.tsx Outdated

acelaya force-pushed the share-native-popover branch from b92f5f8 to 2476e66 Compare June 19, 2025 12:32

acelaya commented Jun 19, 2025

View reviewed changes

acelaya requested a review from robertknight June 19, 2025 12:38

acelaya force-pushed the share-native-popover branch from 2476e66 to c10df18 Compare June 19, 2025 12:39

robertknight approved these changes Jun 19, 2025

View reviewed changes

Comment thread src/sidebar/components/Annotation/AnnotationShareControl.tsx Outdated

Use a Popover component for the share annotation floating dialog

ad2ef1a

acelaya force-pushed the share-native-popover branch from c10df18 to ad2ef1a Compare June 19, 2025 14:46

acelaya merged commit 8c97e1f into main Jun 19, 2025

acelaya deleted the share-native-popover branch June 19, 2025 14:49

Uh oh!

Conversation

acelaya commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

acelaya Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertknight commented Jun 18, 2025

Uh oh!

robertknight commented Jun 18, 2025

Uh oh!

acelaya commented Jun 18, 2025

Uh oh!

acelaya commented Jun 18, 2025

Uh oh!

acelaya commented Jun 19, 2025

Uh oh!

robertknight commented Jun 19, 2025

Uh oh!

robertknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acelaya commented Jun 19, 2025

Uh oh!

acelaya Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

robertknight Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

acelaya Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

acelaya Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertknight Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

acelaya Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

robertknight Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acelaya Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

robertknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

acelaya commented Jun 12, 2025 •

edited

Loading

codecov Bot commented Jun 12, 2025 •

edited

Loading

acelaya Jun 17, 2025 •

edited

Loading

acelaya Jun 19, 2025 •

edited

Loading

robertknight Jun 19, 2025 •

edited

Loading