fix: fix jarring positioning when a closed component is first opened #5484

jcfranco · 2022-10-14T22:26:06Z

Related Issue: #5399

Summary

This ensures proper positioning of FloatingUIComponents by addressing a few items:

sets floating element's position to match the overlay-positioning value for initial positioning
avoids positioning components if an arrow should be included in positioning calculation phase

Additionally, this refactors the floating-ui.ts module to provide a convenience function to make it easier for components to implement reposition in a consistent, and performant fashion (e.g., debounce batch calls, avoid positioning on closed components).

Notes

When positioned for the first time, the transitioned element may transition in from a different direction (IMO, barely noticeable). Subsequent transitions should work as intended.
This issue affected any component with overlay-positioning="fixed" as the default is absolute and would trip up the initial positioning.
An additional utility for when a floating-UI element/component is closed was introduced to reset the styles for absolutely-positioned elements. This is necessary to prevent hidden floating elements from affecting layout. Worth noting that this will share the same issue as Modal does not emit when setting --calcite-duration-factor to 0 #5206, but shares the same workaround (to set a very small duration). This should be refactored once Modal does not emit when setting --calcite-duration-factor to 0 #5206 lands for a more consistent approach.
Spec tests have also been added to cover the latest changes.

Questions

reposition's signature was updated to include a delayed boolean to toggle between 1. immediate repositioning calls (e.g., when resizing the page) vs. 2. debounced calls (e.g., when multiple props may be updated that trigger repositioning).
a. Does delayed make sense as a name?
b. Should we doc this? One reason to do so is for wrapping components to be able to use debounced repositioning (2)

Note: I tried to set up a spec test for this, but ran into some issues:

~~ResizeObserver not being defined when using floating-ui utils~~ – enabled autoUpdate (floating-ui) for browser contexts only
~~jest.spyOn() not working on the floating-ui helper module (spies not being called)~~ – used jest's moduleNameMapper to load the CJS version of lodash in spec tests instead of the ESM one, which will cause an error)

~~I'll revisit this in a future PR.~~

Pending

updating spec tests to cover moving-offscreen changes (I'll tackle this in a follow-up PR)

cc @driskull

benelan · 2022-10-14T23:36:25Z

src/components/popover/popover.stories.ts

+
+      // need to set open after initial render to reproduce the issue
+      popover.open = true;
+    })();


I don't think this will work, it probably needs to be an interaction test eventually.

Switching over to steps.

src/utils/floating-ui.ts

benelan

LGTM!

jcfranco · 2022-10-17T20:47:59Z

Moving this back to draft. Ran an example where this is still noticeable.

I think we'll need to roll back the debouncing. Might not be impactful since floating-ui reverted the performance-affecting code.

jcfranco · 2022-10-17T21:20:02Z

I think I figured out the cause for the initial position flash. The repositioning logic is debounced, but the prop that toggles the floating-ui animation class is set as toggle is opened. Hence why the component is visible right before it shifts to the right spot. I see two options:

roll back debouncing
provide a hook for components to pass any classes to apply right when a component will be repositioned.

As hinted previously, going with 1 for now.

driskull · 2022-10-18T15:54:40Z

src/utils/floating-ui.ts

@@ -380,6 +380,9 @@ export function connectFloatingUI(

  disconnectFloatingUI(component, referenceEl, floatingEl);

+  // ensure position matches for initial positioning
+  floatingEl.style.position = component.overlayPositioning;


jcfranco · 2022-10-18T19:00:50Z

Tried option 1., but noticed there's a noticeable perf impact when dealing with multiple floating-ui-owning components (similar to #5286). Going to give option 2 a try.

driskull

Does delayed make sense as a name?

Yeah, I think so. Or maybe scheduleReposition? Like scheduleRender?

Should we doc this? One reason to do so is for wrapping components to be able to use debounced repositioning (2)

I think so.

src/utils/floating-ui.ts

* drop scale transformto have correct dimensions initially * reposition immediately on componentDidLoad

jcfranco · 2022-10-24T17:38:27Z

@driskull I updated this to handle the case where hidden popovers would affect layout. We can iterate on this if needed, but it seems to work without further affecting transitions. FWIW, I tried the suggested removal of visibility in our styles, but this still affected layout.

driskull · 2022-10-24T18:16:51Z

FWIW, I tried the suggested removal of visibility in our styles, but this still affected layout.

The suggestion to remove visibility is for hiding the FloatingUI when the reference is no longer in view. Currently, the floating-ui JS util and styles are competing and its not working.

…-ui-positioning # Conflicts: # src/utils/floating-ui.ts

* feat(button): add built-in translations * change watcher order * remove intl default test assertion * cleanup * handle edge case * fix build errors * docs(conventions): update style guide (#5530) * fix(slider): fix slider (single-value) error when clicking range (#5533) #5321 * 1.0.0-next.603 * ci(screener): reenable screener on pushes to master to update baseline (#5528) * ci(screener): renable screener on pushes to master to update baseline * cleanup Co-authored-by: Ben Elan <benelan@users.noreply.github.com> * fix: components should only react to primary button pointer events. (#5519) * fix: components should only react to primary pointer events. * cleanup. add pointerdown * fix test * make sure primary? * review fixes * 1.0.0-next.604 * feat(date-picker, input-date-picker): add numberingSystem property (#5488) * feat(date-picker): add numberingSystem property * docs(date-picker): cleanup lang/numberingSystem selection code * feat(input-date-picker): add numberingSystem property [WIP] * add stories * cleanup * fix spec test * cleanup * test(date-picker-day): fix failing disabled commonTest * merge master * cleanup * review cleanups * skip (de)localization when formatter is not initialized * cleanup and add a spec test * don't init formatter until necessary * final cleanup * replace stepped stories and change locale to lang * test(input-date-picker): add dark theme class to darkThemeRTL story Co-authored-by: Ben Elan <benelan@users.noreply.github.com> * 1.0.0-next.605 * docs: consistent api styling + define card thumbnailPosition (#5518) * docs(card): defined thumbnailPosition and consistent api styling across component * fix typo * remove extraneous dash * add backticks to comps * add b + c component consistency * edits * doc feedback * fix(value-list-item): Change drag handle color. (#5543) * 1.0.0-next.606 * fix: add custom logic for floating-ui positioning across shadow DOM in non-Chromium browsers (#5542) * fix: add custom code for floating-ui positioning across shadow DOM in non-Chrome browsers * tidy up * fix conditional to avoid setting up fix for non-browser environments * provide a way to opt-out of fix in favor of performance * fix: fix jarring positioning when a closed component is first opened (#5484) * fix: fix jarring positioning when a closed component is first opened * uncomment actual fix and switch story to be stepped * tweak story to capture initial positioning * fix typo * add missing setAttr argument * add delay before screenshot test setup * revisit approach to preserve debounced internal repositioning calls and correct positioning * drop unnecessary story * tidy up * fix hydrate build * restore leading option * update test * tweak positioning * drop scale transformto have correct dimensions initially * reposition immediately on componentDidLoad * update tests * revert index.html updates * use transitions to reset positioning * fix test. Co-authored-by: Matt Driscoll <mdriscoll@esri.com> * 1.0.0-next.607 * fix(calcite-loader, calcite-input-message): drop active in favor of hidden (#5537) * fix(calcite-loader, calcite-input-message): drop active in favor of hidden * changes applied to input-message * modify render assertion in the tests * remove redundant hidden prop along with documentation (global attribute) * add the link to mdn hidden attribute * 1.0.0-next.608 * feat(flow-item): Add calciteFlowItemScroll event (#5547) * feat(flow-item): Add scroll event. (#5546) * add a test * 1.0.0-next.609 * ci: adds w3c url for a11y issue filing (#5556) ci: update need info url for a11y * fix(date-picker): display correct date format order in header for zh-CN locale. (#5534) fix(date-picker): display correct date format order in header for zh-CN locale * 1.0.0-next.610 * fix(stepper-item): make sure numberingSystem is rendered on load (#5640) * fix(stepper-item): set numberingSystem in render block * cleanup * cleanup the cleanup Co-authored-by: Ben Elan <benelan@users.noreply.github.com> * 1.0.0-next.611 * feedback changes * remove browser build conditional * fix(slider): dragging range fires input event (#5641) #5449 * fix(tooltip): Prevent opening when closeOnClick is true and referenceElement is clicked quickly (#5643) * fix(tooltip): Stop hover event when closeOnClick is true and click occurs. (#5538) * add test * review cleanup * fix(types): fix type issue caused by unintentionally moving @floating-ui/dom as a dev dependency (#5649) * avoid fetching in hydrate builds * resolve conflicts * clean up * more cleanup Co-authored-by: Ben Elan <belan@esri.com> Co-authored-by: JC Franco <jfranco@esri.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Ben Elan <benelan@users.noreply.github.com> Co-authored-by: Matt Driscoll <mdriscoll@esri.com> Co-authored-by: Kitty Hurley <khurley@esri.com> Co-authored-by: Eliza Khachatryan <eli97736@esri.com> Co-authored-by: Alison Stump <alisonailea@users.noreply.github.com>

…placed floating-ui elements when associated-components are closed (#6709) **Related Issue:** #6404 ## Summary This removes a utility added in #5484 that would reset position of floating-UI elements using the absolute strategy to avoid scroll bars when hidden. Unfortunately, the util would lead to undesired position resets in some scenarios (e.g., quick toggling between transitions). Moving forward, it is recommended to use `overlay-positioning='fixed'`to escaping clipping containers and prevent hidden floating-ui elements from affecting layout/creating scrollbars.

fix: fix jarring positioning when a closed component is first opened

adb0d93

jcfranco requested a review from a team as a code owner October 14, 2022 22:26

jcfranco added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 14, 2022

github-actions bot assigned jcfranco Oct 14, 2022

github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Oct 14, 2022

jcfranco requested review from benelan and eriklharper October 14, 2022 23:27

benelan reviewed Oct 14, 2022

View reviewed changes

jcfranco added 2 commits October 14, 2022 16:49

uncomment actual fix and switch story to be stepped

048e245

tweak story to capture initial positioning

23cedd0

jcfranco added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 15, 2022

fix typo

d7369df

jcfranco added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 15, 2022

benelan approved these changes Oct 15, 2022

View reviewed changes

add missing setAttr argument

b376ae6

jcfranco added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 15, 2022

add delay before screenshot test setup

8612077

jcfranco added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 15, 2022

jcfranco marked this pull request as draft October 17, 2022 20:48

jcfranco removed the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 17, 2022

driskull reviewed Oct 18, 2022

View reviewed changes

jcfranco marked this pull request as ready for review October 19, 2022 05:43

jcfranco added 2 commits October 19, 2022 00:24

tidy up

ce4a5ce

fix hydrate build

92ba61c

jcfranco requested review from driskull and benelan October 19, 2022 18:27

jcfranco added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 19, 2022

driskull approved these changes Oct 19, 2022

View reviewed changes

src/utils/floating-ui.ts Show resolved Hide resolved

src/utils/floating-ui.ts Show resolved Hide resolved

src/utils/floating-ui.ts Show resolved Hide resolved

restore leading option

31ea11a

jcfranco added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 19, 2022

jcfranco added 3 commits October 20, 2022 21:18

update test

dead552

tweak positioning

1759253

* drop scale transformto have correct dimensions initially * reposition immediately on componentDidLoad

update tests

4076360

jcfranco added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 21, 2022

revert index.html updates

fb49e36

jcfranco mentioned this pull request Oct 24, 2022

fix: add custom logic for floating-ui positioning across shadow DOM in non-Chromium browsers #5542

Merged

jcfranco added 2 commits October 24, 2022 10:11

use transitions to reset positioning

477bd69

merge master

9e59210

driskull added 2 commits October 24, 2022 12:37

fix test.

72876cf

Merge branch 'master' into jcfranco/5399-fix-initial-jarring-floating…

c2efc24

…-ui-positioning # Conflicts: # src/utils/floating-ui.ts

benelan merged commit 4c939ea into master Oct 24, 2022

benelan deleted the jcfranco/5399-fix-initial-jarring-floating-ui-positioning branch October 24, 2022 20:13

github-actions bot added this to the Sprint 2022/10/24 - 2022/11/04 milestone Oct 24, 2022

jcfranco mentioned this pull request Mar 13, 2023

[Tooltip] Tooltip jumps to the top left when reference element is no longer visible #6397

Closed

jcfranco mentioned this pull request Apr 13, 2023

fix(combobox, dropdown, input-date-picker, popover, tooltip): fix misplaced floating-ui elements when associated-components are closed #6709

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix jarring positioning when a closed component is first opened #5484

fix: fix jarring positioning when a closed component is first opened #5484

jcfranco commented Oct 14, 2022 •

edited

Loading

benelan Oct 14, 2022

jcfranco Oct 14, 2022

benelan left a comment

jcfranco commented Oct 17, 2022

jcfranco commented Oct 17, 2022

driskull Oct 18, 2022

jcfranco commented Oct 18, 2022

driskull left a comment

jcfranco commented Oct 24, 2022

driskull commented Oct 24, 2022

fix: fix jarring positioning when a closed component is first opened #5484

fix: fix jarring positioning when a closed component is first opened #5484

Conversation

jcfranco commented Oct 14, 2022 • edited Loading

Summary

Notes

Questions

Pending

benelan Oct 14, 2022

Choose a reason for hiding this comment

jcfranco Oct 14, 2022

Choose a reason for hiding this comment

benelan left a comment

Choose a reason for hiding this comment

jcfranco commented Oct 17, 2022

jcfranco commented Oct 17, 2022

driskull Oct 18, 2022

Choose a reason for hiding this comment

jcfranco commented Oct 18, 2022

driskull left a comment

Choose a reason for hiding this comment

jcfranco commented Oct 24, 2022

driskull commented Oct 24, 2022

jcfranco commented Oct 14, 2022 •

edited

Loading