Fix drop container transform #6097

JrDuComptoirDesPharmacies · 2022-04-28T15:50:21Z

What does this PR do?

Fixes the drop container position when it inlude in a transformed parent position (fixed position is not computed according viewport)

Before the fix: https://codesandbox.io/s/adoring-currying-ztj374
After the fix: https://codesandbox.io/s/grommet-v2-template-forked-vojosq

Where should the reviewer start?

See in src/js/components/Drop/DropContainer.js in place method

What testing has been done on this PR?

Maual testing through firefox/chrome using codesandbox from issue #5954

How should this be manually tested?

Click on the Fancy Selector and control the drop position

Do Jest tests follow these best practices?

screen is used for querying.
The correct query is used. (Refer to this list of queries)
userEvent is used in place of fireEvent.
asFragment() is used for snapshot testing.

Any background context you want to provide?

Description of the fixed position according to transformed parent:
https://developer.mozilla.org/fr/docs/Web/CSS/position#fixed

Description of way to find the containing block used for fixed element position computation :
https://developer.mozilla.org/en-US/docs/Web/CSS/Containing_block#identifying_the_containing_block

Similar to this issue: floating-ui/floating-ui#1577

Grommet solution inspired of this code: https://github.com/floating-ui/floating-ui/blob/a3299dc9c56a7c95183dfb3463f0542db4e945e2/packages/dom/src/utils/getOffsetParent.ts#L22

What are the relevant issues?

See #5954

Screenshots (if appropriate)

Before:

After:

Do the grommet docs need to be updated?

No

Should this PR be mentioned in the release notes?

Yes

Is this change backwards compatible or is it a breaking change?

Backwards compatible

…e element

src/js/components/Drop/DropContainer.js

…esPharmacies/grommet into fix-drop-container-transform

jcfilben

Looks good!

AntoineDuComptoirDesPharmacies · 2022-06-15T15:28:49Z

Hi @ericsoderberghp,

Any news about this PR ?
Do not hesitate to tell if you want us to change something.

Yours faithfully,
LCDP

ericsoderberghp · 2022-09-28T16:48:22Z

I think I at least partly understand what's going on here. We are skipping over parents that have some sort of CSS transformation so that we can adjust the drop position by the appropriate offset of the transformation. Two thoughts:

what if the immediate parent and the great-grandparent both have transformations? I think we will be missing the second one since we just stop at the grandparent?
the code to get computed style and look through various CSS properties feels a bit fragile and I'm wondering if there's a cleaner way to accomplish it

JrDuComptoirDesPharmacies · 2022-10-06T14:13:47Z

Thank you for your reply. To answer your two thoughts:

The principle is to try to find the first parent that has a transformation and position the component from there. More distant relatives are not taken into account for the position calculation. Here's an example that shows multiple nested transforms aren't a problem: https://codesandbox.io/s/quizzical-shadow-h5q68z?file=/index.js
For the second point, many projects rely on this type of detection to identify the containing block (https://developer.mozilla.org/en-US/docs/Web/CSS/Containing_block#identifying_the_containing_block), here are some examples :
- https://github.com/floating-ui/floating-ui/blob/3dd6b42489b2beaeb97379769a6e4d63333ee171/packages/dom/src/utils/is.ts
- https://github.com/sonnat/sonnat-ui/blob/a2c2582f4548c7ff354da56799c072924f376ff1/lib/utils/is.ts

Yours faithfully,
LCDP

GeryDuComptoirDesPharmacies · 2023-01-02T15:30:21Z

Hello,

Someone available to merge this PR ?

Best regards,
Géry

GeryDuComptoirDesPharmacies · 2023-02-09T15:36:19Z

Hi,
@ericsoderberghp still waiting for your approval.

Can you tell if changes are needed please ?

Best regards,
Géry

jonscottclark · 2023-02-10T04:29:16Z

Hello :) I have been testing this PR today, thank you to @JrDuComptoirDesPharmacies for your work on this!

These changes did help us solve a particular use case where we have a Drop being positioned incorrectly when inline === true, and when it its container is creating a new stacking context

I have some suggestions which fixes our issues entirely and hopefully you'll see them as useful additions to this PR.

When the containingBlock is scrollable (via overflow: auto, for example) when setting the position of the Drop, the current scrollLeft and scrollTop of that container need to be factored into the equation, otherwise the placement is incorrect if the containingBlock has been scrolled at all.

Imagine this scenario:

We have a button that triggers a Drop with inline: true
Target is 200px from viewport edge in a container that can scroll horizontally
Let's assume we're aligning the Drop and the target on their left edges, so the Drop gets a left position of 200px
Now the container is scrolled to the left by 50px, therefore the target is 50px nearer to the viewport edge, so viewportOffsetLeft is 50px lower but also its calculated getBoundingClientRect().left is 50px lower. The resulting left rule that we assign the Drop is therefore 100px. Since the target is actually 150px from the left of the viewport, the fixed position should be left: 150px.

In DropContainer.js, when setting the final position values we need to use these new calculations:

container.style.left = `${left - viewportOffsetLeft + (containingBlock?.scrollLeft ?? 0);

// ...

if (top !== '') {
  container.style.top = `${top - viewportOffsetTop +
    (containingBlock?.scrollTop ?? 0)}px`;
}
if (bottom !== '') {
  container.style.bottom = `${viewportOffsetBottom - bottom -
    (containingBlock?.scrollTop ?? 0)}px`;
}

The Drop does not update its position if the horizontal position of the target has changed, i.e. if it is within a containingBlock that has been scrolled to the left or right. (This is somewhat unrelated to the initial changes, but it does fix a positioning bug):

In DropContainer.js:

let scrollParents;
+ let scrollParentsHorizontal;

const addScrollListeners = () => {
  scrollParents = findScrollParents(dropTarget);
+ scrollParentsHorizontal = findScrollParents(dropTarget, true);
  scrollParents.forEach((scrollParent) =>
    scrollParent.addEventListener('scroll', place),
  );
+ scrollParentsHorizontal.forEach((scrollParent) =>
+   scrollParent.addEventListener('scroll', place),
+ );
};

I tested these changes with both true and false values for inline. (For what it's worth, we have much better control over layering when inline: true)

Let me know if you have any feedback, would love to see this merged

@jcfilben @ericsoderberghp I just joined your Slack community today 🥳

AntoineDuComptoirDesPharmacies · 2023-02-10T11:01:59Z

Hi @jonscottclark,

Thank you for this additional use case, it seems logic to realign the drop when the horizontal scroll is changing too.

I tried to modify the CodeSandbox to represent your use case without success.
Could you Fork and update this CodeSandbox so we can see the problem please ?
https://codesandbox.io/s/grommet-v2-template-forked-sgxv28?file=/index.js

Once done, we will compare the behavior with and without your fix and we will add your changes to the Pull Request.

Thanks in advance !
Yours faithfully,
LCDP

jonscottclark · 2023-02-10T15:32:04Z

@AntoineDuComptoirDesPharmacies Good idea, I'll attempt to make a minimum reproduction of the situation I'm describing.

jonscottclark · 2023-02-10T16:07:11Z

@AntoineDuComptoirDesPharmacies

Here's the simplified scenario: https://codesandbox.io/s/grommet-v2-template-forked-fqhfx5?file=/index.js

We have a very wide Box with items in it - scroll horizontally to see all of the items.

After clicking a button the incorrect "top" positioning is as expected - the currently broken positioning that your PR already fixes.

Now try clicking on "Open Drop 1", then start scrolling vertically and horizontally. You will see the necessity of checking for containingBlock.scroll<Top|Left>! :)

If you change inline to false, you lose the horizontal scroll updating which is what I address in the 2nd item, however, I'm not super happy with the implementation (more resize listeners) - but it does what we need it to! Open to suggestions on how to improve it.

AntoineDuComptoirDesPharmacies · 2023-02-10T16:50:55Z

@jonscottclark Thank you for this perfect reproducer, we can notice that the bug you outlined is resolved with your suggested modifications.

We have just added your modifications to the current PR.

Yours faithfully,
LCDP

jonscottclark · 2023-02-14T06:17:25Z

@AntoineDuComptoirDesPharmacies test snapshot looks like it can be safely updated

The Chromatic step is failing due to the appId not being recognized:

Authenticating with Chromatic
    → Connecting to https://index.chromatic.com
    → No app with code '9q99in2ygnh' found

…transform

AntoineDuComptoirDesPharmacies · 2023-02-14T14:18:42Z

Thank you for the ping @jonscottclark.

We merged the current master branch in the PR so tests pass now.

Yours faithfully,
LCDP

jonscottclark · 2023-02-23T18:02:16Z

Hey @ericsoderberghp , friendly ping to see if you have time to look at this again. Thank you in advance :)

jonscottclark · 2023-08-04T19:55:14Z

@AntoineDuComptoirDesPharmacies ~~Could you perhaps publish a version of @lcdp/grommet that is up to date with the main branch here, just so I am certain I'm not seeing unrelated issues during testing?~~

nvm - cloning your fork instead :)

jonscottclark

I tested this locally by cloning the LDCP fork, rebasing against the grommet main branch (only conflicts were package.json), running a local build and then linking the /dist output locally to our project. I found that one change was needed to make this PR work perfectly for some real-world use cases 🎉

See here that we reference dropTarget, (the prop): https://github.com/grommet/grommet/pull/6097/files#diff-a8c7783b048816fb6a81c4d4d49839bb165a273f34164cc9fa580226391aa51eR294-R295

We need to instead first check if dropTarget?.current is defined, like we do in the place() function: https://github.com/grommet/grommet/pull/6097/files#diff-a8c7783b048816fb6a81c4d4d49839bb165a273f34164cc9fa580226391aa51eR146

If we move L146 out of place() and put it at L96, then rename dropTarget to target in addScrollListeners(), everything works 🙌

This works with inline: true and inline: false.

This is with { drop: { checkContainerBlock: true }}. With this option off, the positioning is okay with inline: false, but with inline: true there's erroneous positioning due to the omission of the containing block's position - for the reasons I list here: #6097 (comment)

So it seems this work actually contains two things:

A fix to the scroll listeners not always sending a valid element to findScrollParents
The opt-in behaviour to calculate drop positioning based on possible containing blocks.

For what it's worth, and to continue my rambling, we will 100% be opting in to this behaviour (and hopefully it becomes a default), and also using inline: true for all of our Drop instances (so that we can tell rendered Drop content to layer beneath some ancestor sticky elements - with inline: false the Drop content will always go overtop of everything)

Thank you, LDCP folks for the initial PR and continued attention, and Grommet folks for the work you do 🤝

jonscottclark · 2023-08-08T17:32:07Z

@GeryDuComptoirDesPharmacies I should clarify that I approved the PR (instead of saying Request Changes) because it works for me assuming this comment is implemented:

See here that we reference dropTarget, (the prop): #6097 (files)

We need to instead first check if dropTarget?.current is defined, like we do in the place() function: #6097 (files)

If we move L146 out of place() and put it at L96, then rename dropTarget to target in addScrollListeners(), everything works 🙌

Just didn't want that point to get lost in the text :) Thanks

GeryDuComptoirDesPharmacies · 2023-08-09T08:29:04Z

@jonscottclark We will implement what you ask for.
Actually we are a little bit overwork. In the end of the month, it will be done.

Thanks for your time.

…transform

AntoineDuComptoirDesPharmacies · 2023-08-14T09:42:15Z

Hi @jonscottclark,

We have included your remarks and rebase the branch on last master's commit.

Yours faithfully,
LCDP

GeryDuComptoirDesPharmacies · 2023-08-23T15:41:27Z

Hi,

Review is done, let's merge it this time please 🙏🏾
Let's don't waste time as we have rebase from master.

@ericsoderberghp @jonscottclark @jcfilben

Thanks.

jcfilben · 2023-08-23T19:02:20Z

Hi @GeryDuComptoirDesPharmacies I am still seeing issues with the drop positioning behavior.

Aug-23-2023.12-58-33.mp4

Since the primary issue this PR is trying to solve is the containing block issue I would be okay moving forward with only the containing block code and removing the scrollParentsHorizontal code.

jonscottclark · 2023-08-23T21:40:33Z

@jcfilben is the issue you mention the seemingly slow updating of the drop position?

jcfilben · 2023-08-23T22:33:11Z

@jcfilben is the issue you mention the seemingly slow updating of the drop position?

Yes, in addition I would expect the drop container to scroll out of view when the target is out of view

jonscottclark · 2023-08-24T17:57:05Z

@jcfilben Okay - I see there's a lot of stuff happening in the place() function that is related to top/bottom positioning but not the same equivalent for left, so I suppose that needs a bit more attention in order to support the horizontal scrolling case.

I'm okay with removing the scrollParentsHorizontal support from the scope of this PR so it can progress, and I can possibly open a PR to add support for scrollParentsHorizontal later, as trying to add that functionality to this PR will delay it even further.

If you agree, then perhaps we can get a check from @ericsoderberghp once it's removed since everything else is looking great in this PR.

cc @GeryDuComptoirDesPharmacies

jcfilben · 2023-08-24T22:25:26Z

@jcfilben Okay - I see there's a lot of stuff happening in the place() function that is related to top/bottom positioning but not the same equivalent for left, so I suppose that needs a bit more attention in order to support the horizontal scrolling case.

I'm okay with removing the scrollParentsHorizontal support from the scope of this PR so it can progress, and I can possibly open a PR to add support for scrollParentsHorizontal later, as trying to add that functionality to this PR will delay it even further.

If you agree, then perhaps we can get a check from @ericsoderberghp once it's removed since everything else is looking great in this PR.

cc @GeryDuComptoirDesPharmacies

Yes, I'm supportive of removing the scrollParentsHorizontal code and handling that issue in a separate PR

…R as it requires more work to be smooth See: grommet#6097 (comment)

AntoineDuComptoirDesPharmacies · 2023-08-28T07:41:15Z

@jcfilben @jonscottclark
O.K to us, the PR have been updated with horizontal scroll fix removed.

Yours faithfully,
LCDP

GeryDuComptoirDesPharmacies · 2023-08-31T08:37:57Z

Hi everyone,

Everything is fine now. Can we merge please ?

@jcfilben @jonscottclark @ericsoderberghp

jcfilben · 2023-08-31T15:36:54Z

src/js/components/Drop/DropContainer.js

    const portalContext = useContext(PortalContext);
    const portalId = useMemo(() => portalContext.length, [portalContext]);
    const nextPortalContext = useMemo(
      () => [...portalContext, portalId],
      [portalContext, portalId],
    );
    const dropRef = useForwardedRef(ref);
+    const target = dropTarget?.current || dropTarget;


Why does this need to be defined outside of the useEffect?

It do not need to be defined outside.
I followed the recommendations of @jonscottclark here : #6097 (review)

I just moved it into the useEffect to save some computation.
Yours faithfully,
LCDP

jcfilben

Looks good!

jcfilben · 2023-09-06T19:22:39Z

Looks like there are some merge conflicts that need to be resolved before merging

…transform # Conflicts: # src/js/components/Drop/DropContainer.js

AntoineDuComptoirDesPharmacies · 2023-09-07T07:56:56Z

@jcfilben conflict have been resolved

JrDuComptoirDesPharmacies · 2023-09-08T07:31:49Z

🥳

JrDuComptoirDesPharmacies and others added 8 commits April 27, 2022 16:46

+ Fix the drop container position when containing block is an absolut…

942dbe6

…e element

+ Use container instead of target to get the getContainingBlock

29cc0b3

Fix jest tests

5bd341a

+ Fix tests

bf94853

+ Resolve merge conflicts

7514553

Merge branch 'master' into fix-drop-container-transform

f6f6fe8

Merge branch 'master' into fix-drop-container-transform

c067c85

Merge branch 'master' into fix-drop-container-transform

9ddb1a3

jcfilben reviewed May 3, 2022

View reviewed changes

src/js/components/Drop/DropContainer.js Outdated Show resolved Hide resolved

src/js/components/Drop/DropContainer.js Outdated Show resolved Hide resolved

jr2dallas and others added 2 commits May 13, 2022 10:55

+ Ignore IE & Firefox

228fb79

Merge branch 'fix-drop-container-transform' of github.com:LeComptoirD…

7b09e84

…esPharmacies/grommet into fix-drop-container-transform

jcfilben approved these changes May 19, 2022

View reviewed changes

jcfilben requested a review from ericsoderberghp May 19, 2022 22:08

Merge branch 'grommet:master' into fix-drop-container-transform

bfe7e60

jcfilben added the needs attention To alert grommet team that a PR has been waiting for the author for a while label Dec 21, 2022

Add @jonscottclark fix about horizontal Drop position

5266868

Merge remote-tracking branch 'origin/master' into fix-drop-container-…

8b252e7

…transform

jonscottclark approved these changes Aug 4, 2023

View reviewed changes

AntoineDuComptoirDesPharmacies added 2 commits August 14, 2023 11:27

Take into account @jonscottclark remarks

415e90b

Merge remote-tracking branch 'origin/master' into fix-drop-container-…

1287730

…transform

Remove scrollParentsHorizontal feature to be done in another ticket/P…

d7b16c3

…R as it requires more work to be smooth See: grommet#6097 (comment)

jcfilben reviewed Aug 31, 2023

View reviewed changes

Take into account @jcfilben remark

edd5969

jcfilben approved these changes Aug 31, 2023

View reviewed changes

jcfilben requested review from halocline and taysea August 31, 2023 15:59

halocline approved these changes Aug 31, 2023

View reviewed changes

taysea approved these changes Sep 1, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into fix-drop-container-…

1029622

…transform # Conflicts: # src/js/components/Drop/DropContainer.js

jcfilben merged commit e8b328b into grommet:master Sep 8, 2023

This was referenced Oct 27, 2023

added documents for enhancements grommet/grommet-site#491

Merged

DropContainer "place" method use wrong boundaries #5954

Closed

Fix drop container transform #6097

Fix drop container transform #6097

Uh oh!

Conversation

JrDuComptoirDesPharmacies commented Apr 28, 2022

What does this PR do?

Where should the reviewer start?

What testing has been done on this PR?

How should this be manually tested?

Do Jest tests follow these best practices?

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

Uh oh!

Uh oh!

Uh oh!

jcfilben left a comment

Choose a reason for hiding this comment

Uh oh!

AntoineDuComptoirDesPharmacies commented Jun 15, 2022

Uh oh!

ericsoderberghp commented Sep 28, 2022

Uh oh!

JrDuComptoirDesPharmacies commented Oct 6, 2022

Uh oh!

GeryDuComptoirDesPharmacies commented Jan 2, 2023

Uh oh!

GeryDuComptoirDesPharmacies commented Feb 9, 2023

Uh oh!

jonscottclark commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AntoineDuComptoirDesPharmacies commented Feb 10, 2023

Uh oh!

jonscottclark commented Feb 10, 2023

Uh oh!

jonscottclark commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AntoineDuComptoirDesPharmacies commented Feb 10, 2023

Uh oh!

jonscottclark commented Feb 14, 2023

Uh oh!

AntoineDuComptoirDesPharmacies commented Feb 14, 2023

Uh oh!

jonscottclark commented Feb 23, 2023

Uh oh!

jonscottclark commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonscottclark left a comment • 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!

jonscottclark commented Aug 8, 2023

Uh oh!

GeryDuComptoirDesPharmacies commented Aug 9, 2023

Uh oh!

AntoineDuComptoirDesPharmacies commented Aug 14, 2023

Uh oh!

GeryDuComptoirDesPharmacies commented Aug 23, 2023

Uh oh!

jcfilben commented Aug 23, 2023

Uh oh!

jonscottclark commented Aug 23, 2023

Uh oh!

jcfilben commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonscottclark commented Aug 24, 2023

Uh oh!

jcfilben commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AntoineDuComptoirDesPharmacies commented Aug 28, 2023

Uh oh!

GeryDuComptoirDesPharmacies commented Aug 31, 2023

jonscottclark commented Feb 10, 2023 •

edited

Loading

jonscottclark commented Feb 10, 2023 •

edited

Loading

jonscottclark commented Aug 4, 2023 •

edited

Loading

jonscottclark left a comment •

edited

Loading

jcfilben commented Aug 23, 2023 •

edited

Loading

jcfilben commented Aug 24, 2023 •

edited

Loading