Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fixes Html element moving with scrollControls closes #1048 #1126

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TomasGonzalez
Copy link

@TomasGonzalez TomasGonzalez commented Nov 1, 2022

Why

Html component is currently having issues when used along the Scroll controls, resolves #1048

What

I modified the target DOMElement selection conditional to prioritize the parent element returned by gl.domElement.parentNode instead of the element returned by events.connected

Checklist

  • Documentation updated
  • Storybook entry added
  • Ready to be merged

@vercel
Copy link

vercel bot commented Nov 1, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
drei ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 1, 2023 at 3:47PM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 1, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5f5f5ed:

Sandbox Source
angry-fog-yemz08 Configuration
Ground reflections and video textures Configuration
arc-x-pmndrs-colors Configuration

@joshuaellis
Copy link
Member

joshuaellis commented Nov 1, 2022

Can you explain in further detail why you've done the changes you've made please?

@TomasGonzalez
Copy link
Author

TomasGonzalez commented Nov 1, 2022

Can you explain in further detail why you've done the changes you've made please?

When you set your Html element to transform, the way this works is that the element gets attached to a "target" DOM element, this target was being selected in this order portal.current || events.connected || gl.domElement.parentNode portal.current is an arbitrary value sent as a prop, so unless that was explicitly passed it would use the events.connected as default. The issue with that is that the DOM element returned by events.connected is already being used by scrollControls as it's position is modified by it. Removing events.connected from this condition, defaults to gl.domElement.parentNode which is the root element of the page, this fixes the issue because now the DOM element we are using to render the Html component is not a child of any arbitrary element but the root. And I see no case where it should actually be, and even if there was a case we could just assign it manually using the portal prop.

@drcmda
Copy link
Member

drcmda commented Nov 2, 2022

im not sure this is a fix. imo it breaks. the node that components should use is always connected. it only falls back to domElement bc of older r3f. domElement would also break portaling, right now html can run in split views because each can have its own connected node, but gl.domElement only exists once.

if you have a scroll container you cannot use client coordinates, you need offset (as in event.offsetX & event.offsetY

<Canvas eventPrefix="offset"

and it would also make sense to make the event source a shared parent, this is what would break here

<Canvas eventPrefix="offset" eventSource={document.getElementById("root")}

there is no one solution fits all. most of the time you want client, but in some situations you need offset.

@TomasGonzalez
Copy link
Author

Ok, that makes sense, I guess that In order to fix it I would have to refactor the way ScrollControls work instead? Prevent it to transform the root? and act on a child node? or I could also add a flag, here in Html component that allows the dev to chose whether or not to attach to the root or the event connected?

What do you suggest?

@drcmda
Copy link
Member

drcmda commented Nov 19, 2022

good question @TomasGonzalez did you figure something out?

@TomasGonzalez
Copy link
Author

good question @TomasGonzalez did you figure something out?

I'll give some thought to this this week, please don't close the PR yet.

@TomasGonzalez
Copy link
Author

TomasGonzalez commented Jan 31, 2023

@joshuaellis @drcmda I'm sorry but I've got no time to fully fix this issue by the method I mentioned #1126 (comment) but I found a very simple work around, you just have to pass {current: gl.domElement.parentNode} as a portal property (obtained from the useThree hook).
I have verified that this works just fine, and already have recommended it to other devs that have asked for help. So basically I reverted my changes to the Html components and updated the readme to add that pice of information. I hope this is a good enough fix until I can dedicate some time to elegantly fix this.

@TomasGonzalez
Copy link
Author

@drcmda @joshuaellis 👀

@LinyanZ
Copy link

LinyanZ commented Feb 18, 2023

It seems that this work around prevents you from scrolling when your mouse is over the Html element (If you apply fullscreen you cannot scroll at all). Did I miss anything?

import { Html, ScrollControls } from "@react-three/drei";
import { Canvas, useThree } from "@react-three/fiber";

const HtmlContent = () => {
  const { gl } = useThree();
  return (
    <Html transform portal={{ current: gl.domElement.parentNode }}>
      Test
    </Html>
  );
};

function App() {
  return (
    <Canvas style={{ width: "100vw", height: "100vh" }}>
      <ScrollControls page={2}>
        <HtmlContent />
      </ScrollControls>
    </Canvas>
  );
}

export default App;

@TomasGonzalez
Copy link
Author

It seems that this work around prevents you from scrolling when your mouse is over the Html element (If you apply fullscreen you cannot scroll at all). Did I miss anything?

import { Html, ScrollControls } from "@react-three/drei";
import { Canvas, useThree } from "@react-three/fiber";

const HtmlContent = () => {
  const { gl } = useThree();
  return (
    <Html transform portal={{ current: gl.domElement.parentNode }}>
      Test
    </Html>
  );
};

function App() {
  return (
    <Canvas style={{ width: "100vw", height: "100vh" }}>
      <ScrollControls page={2}>
        <HtmlContent />
      </ScrollControls>
    </Canvas>
  );
}

export default App;

The work around simply prioritized the Dom element that was used in versions where it used to work... I don't see how this could mess up the scroll (If it was working already)

@bryndsey
Copy link

I've also been experiencing issues with the Html component interfering with normal scrolling within a ScrollControl.

My understanding about what's happening is that when using a portal to the DOM element mentioned in the workaround, events that occur within the Html don't propagate to the Three scene anymore. This affects not only scroll, but also pointer events that should go to any parent elements of the Html (e.g. if the Html is a child of mesh, the mesh will no longer receive click events when the Html is clicked).

There might be a way to correct this using the eventSource of the Canvas, but I wasn't able to figure out how to do that.

However, I did figure out another workaround that seemed to generally get the behavior I was expecting. Rather than using gl.domElement.parentNode (or parentElement), I did the following:

import { Html, ScrollControls, useScroll } from "@react-three/drei";
import { Canvas, useThree } from "@react-three/fiber";

const Scene = () => {
  const scrollData = useScroll();
  return (
    // Portal to a fixed-position container created by the ScrollControl component
    <Html transform portal={{ current: scrollData.fixed }}>
      Test
    </Html>
  );
};

function App() {
  return (
    <Canvas>
      <ScrollControls pages={2}>
        <Scene />
      </ScrollControls>
    </Canvas>
  );
}

With this, event propagation seems to work as expected again, and it handles "nested" scrolling properly (i.e. if the HTML content can scroll, you can scroll within it without moving the ScrollControl until reaching the top/bottom of the scroll, and then the ScrollControl receives the scroll again).

The only issue I've seen so far is that it seems to act very strangely on Firefox - the HTML content will jump around while scrolling. It works fine on Chrome and Safari though.

Here is a codesandbox that demonstrates this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ScrollControls scroll Html element even outside of <Scroll> in Typescript
5 participants