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

After dragging is done, translate3d property still exists in dragged element's style attribute #604

Closed
xdivby0 opened this issue Jan 24, 2023 · 16 comments

Comments

@xdivby0
Copy link

xdivby0 commented Jan 24, 2023

After a dragged element is let go by mouseup event, the z-index: 99 and position: relative are removed from the style attribute, but the transform property is still there.

Yes, it's set to translate3d(0px, 0px, 0px), but it still messes up stacking context in cases where you have e.g. a dropdown inside those draggable elements. I fixed it temporarily with this code after dflexDnD.endDragging():

  setTimeout(()=>{
    dndCompRef.current.style.transform="";
  }, 50);

Which is of course just a hack. Instantly removing the attribute synchronously is somehow not working, probably overwritten later.

Maybe this is something that's intended for some reason I don't see yet. If it's not intended, I'd appreciate a quick "yes, this is a bug" from @jalal246 (or someone else more familiar with this codebase), maybe this is suited for my first OSS contribution :)

@jalal246
Copy link
Member

Hi, @xdivby0 thanks for opening this issue.

  • For the dragged element: The current behavior is cleaning up what's connected directly to the dragging process. If DFlex is adding z-index: 99 for the sake of dragging over other elements then it should be removed after the process ends.

  • For the affected elements: If the element is registered then it's expected to be transformed. So the default is translate3d(0px, 0px, 0px). Because you can always transform without composing the DOM. This means you can indefinitely transform an element inside its container or to another container without changing the DOM tree. This approach is meant to enhance drag-and-drop performance, making it insanely fast. You can try it by setting changing commit default value and setting it to false:

commit: {
  enableAfterEndingDrag: false,
},

When removing translate3d?

If the element is reconciled to the DOM tree. Then its state is empty. No transformation yet.

@xdivby0
Copy link
Author

xdivby0 commented Jan 24, 2023

Thanks for the quick response!

For me the DOM is updated with the changed order after I let go of the dragged element at the new position. Still the translate3d(0,0,0) is there, which messes up stacking context.

From my point of view, after I let go of the element and I have the new order of the elements via the mutation listener, I can remove the translate3d, at least it works for my use case. It automatically gets added by dflex after I try to move anything again (which still works and seems to be efficient). Wouldn't it make sense to just remove the transform property instead of just setting it to translate3d(0,0,0)?

@jalal246
Copy link
Member

For me the DOM is updated with the changed order after I let go of the dragged element at the new position. Still the translate3d(0,0,0) is there, which messes up stacking context.

If the element isn't reconciled then it stays as it is. For example: if before releasing the drag element with my-id is positioned correctly then it stays having its transformation data This element has been transformed and then went back to its origin. DFlex ignores elements that have the right position and minimizes interaction with DOM as possible.

Wouldn't it make sense to just remove the transform property instead of just setting it to translate3d(0,0,0)?

Yes if it's repositioned.

Think of it as a transformation state:

  1. Empty: The element hasn't transformed at all.

  2. Reset: The element traveled and went back to its origin. (your case)

  3. Stateful: The element transformed and waiting for DOM mutation.

It could be a bug if:

A. The element has translate3d(0,0,0) even if it didn't move its position at all. This is not the expected behavior.

B. If it's causing issues with your app for some reason that I am not aware of.

otherwise, it could be annoying but it's okay since the point is transformation after all.

@xdivby0
Copy link
Author

xdivby0 commented Jan 24, 2023

I am not sure if I understand 100% but here's 4 draggable elements before dragging them (the 4 columns are wrapped into the draggable Component I just copied from your example page):
image

Now as you can see if I open the dropdown from the second element, it displays correctly. When I drag the second element just a tiny bit and let it snap back to its original place (or just change it's place with the third element for example), this is the result:

image
This caused by the one or several of the elements now having the transform: translate3d(0,0,0) value. If I go in via the developer tools and remove the style, it's working correctly again.

If nothing speaks against it, couldn't dflex just remove the transform property after being finished (no matter if the element snapped back to its origin or new place)? It would certainly have fewer side-effects like creating new stacking context or maybe other issues

@jalal246
Copy link
Member

Very interesting case to be honest. I'd love to take a look into the code (drop-down styling) if that's possible. Of course, cleaning element styling and attributes is possible.

@xdivby0
Copy link
Author

xdivby0 commented Jan 25, 2023

I think it's a bit niche case, yes. Still, why deal more side-effects than necessary :) If you think I could try preparing a PR, tell me, I'll gladly do that.

For the code of the dropdown: I've made a typed component for the dropdown, actually it's a searchable dropdown (just super-basic for now). It's highly specialized though because it's made for a specific form-library (remix-validated-form) for the Remix framework (which is in turn based on React).

So here's the probably relevant part of the component for styling and html structure. It uses tailwindcss in case you don't recognize the class names:

    <div className="relative">
      <label className="text-primary-800/70" htmlFor={props.name}>{props.label}</label>
      // hidden input is needed to actually select the id, not the display value
      <input type="hidden" name={props.name} value={value}
             {...getInputProps({ id: props.name })}
      />

      // here comes the actual visible text container in which you type in something for searching
      <div className={`relative`}>
        <input
          onChange={(e) => {
            setVisibleElements([...props.data].filter(x => props.searchFilter(x, e.target.value)));
            setText(e.target.value)
          }}
          value={text}
          onFocus={() => setFocus(true)}
          onBlur={() => setTimeout(()=>setFocus(false), 100)}
          className={`w-full block bg-white font-semibold px-2 py-1 rounded mt-1 outline-none
      ${error ? "border-red-400" : "border-transparent focus:border-primary-500"} border-2 `}
        />

        // the part that actually drops down and gets problematic when the stacking context is manipulated
        {focus &&
          <div className="z-10 translate-x-1 max-h-48 overflow-y-auto mt-2 absolute bg-white p-1.5 shadow-md rounded-md">
            {visibleElements.map(element =>
              <div className="px-4 py-2 rounded cursor-pointer hover:bg-primary-100" onClick={()=>{
                setValue(element[props.selectAttribute]);
                setText(element[props.renderAttribute])
              }}>
                {
                  element[props.renderAttribute]  // renderAttribute is "name" in this case, while selectAttribute is "id". Both are possible fields of the elements displayed
                }
              </div>)}
          </div>
        }
      </div>

      // rest is error/hints
      {props.helperText &&
        <p className={`text-primary-800/60 text-sm`}>{props.helperText}</p>
      }
      {error &&
        <span className="text-red-500">{error}</span>
      }
    </div>

If you have any questions or are interested in the whole component for actually using it somewhere, I can give that to you too, just didn't paste it here because not relevant for the issue :)

@jalal246
Copy link
Member

why deal more side-effects than necessary

It's the opposite. The rule is to minimize the DOM interaction that's why transformation is there. Less is more performative.

If there is a repo or codesandbox where I can try it live would be easier to deal with it. For the contribution, I'd love to. Maybe adding a new case to the playground to test it and deal with it.

@xdivby0
Copy link
Author

xdivby0 commented Jan 25, 2023

Oh I didn't mean to not transform at all, I love how you implemented that!

Just after the dragging interaction is finished (with endDragging) you shouldn't leave the translate3d(0,0,0) there. What already happens is that dflex cleans up the other attributes like z-index: 99 and position: relative after one specific drag interaction has ended and the DOM has been mutated.

I am not sure if you know what that does, so this may seem obvious, sorry in this case:
For the average user leaving translate3d(0,0,0) doesn't seem like an issue cause it's not actually translating by any amount. You'll notice however that even if you translate something by 0, it creates a new z-index stacking context. It is a side-effect that dflex can afterwards clean up and only add it back after a user starts dragging something else again (just like dflex is already doing with the other two attributes mentioned in the previous paragraph).

Didn't you know that, or am I just not seeing something?

@jalal246
Copy link
Member

I got your point but for me, I am not aware of the issue that's creating. As the DFlex element is expected to be transformed. The assumption is that all elements should be expected to have their transformation whether it's zero or not.

This is a normal scenario:

image

So the ground zero is translate3d(0,0,0). I am digging into the stacking context. Thanks for the reference. Still, is it a better approach to preserve stacking context for each element or just create/remove a new one with every interaction? We could make it customized depending on the case where some cases require a total clean-up.

What I usually do is create a route in the playground to deal with different cases and then develop and test the solution Cypress/Playwright. That's why playground has some unsolved issues #597

@xdivby0
Copy link
Author

xdivby0 commented Jan 25, 2023

Making it a flag when registering the element in the store or some similar customization would defo work for me, I am just not sure if it would mean unnecessary complexity in dflex's code + for devs using it since I can't imagine a use case where it would be beneficial to not remove the transform translate property and add it with every new interaction again.

I am not sure if I find the time to set up a playground example to demonstrate, but I'll try to do today or tomorrow.

@xdivby0
Copy link
Author

xdivby0 commented Jan 25, 2023

This is a normal scenario:

image

Wait, is this only while you are still holding down the mouse button? I have similar transforms while I am dragging / holding the mouse down. But once I let go, everything is reconciled with the DOM once and then the transform translate values are all 0. Is that weird or normal?

@jalal246
Copy link
Member

Nope. This is after releasing the drag. There's an option where you can control the reconciliation. You can set it to false and trigger store.commit() when needed.

@jalal246
Copy link
Member

Elements' positions and transformations are preserved internally. It's a hard approach to implement but it's also fast. Even changing the dragged position is conditional depending on each scenario. For example, the default is not to clone and ghost unless it's necessary.

@xdivby0
Copy link
Author

xdivby0 commented Jan 25, 2023

Okay, I didn't touch that option, so the default is to always reconcile after a drag has finished, correct?

In that case, shouldn't the default also be to clean up the translate property after reconciling (because of course if the elements still have values inside the translate property, we can't just remove it). Maybe we can make removing the translate property part of the reconcilation?

@jalal246
Copy link
Member

Okay, I didn't touch that option, so the default is to always reconcile after a drag has finished, correct?

Yes.

Maybe we can make removing the translate property part of the reconcilation?

Yes if it's necessary. Because changing the context of the element has a cost. The browser will repaint again if the user decides to do another round of drag and drop. That's why it's preserved even when you reconcile. because it's more friendly to the browser.

@xdivby0
Copy link
Author

xdivby0 commented Jan 25, 2023

Yes if it's necessary. Because changing the context of the element has a cost. The browser will repaint again if the user decides to do another round of drag and drop. That's why it's preserved even when you reconcile. because it's more friendly to the browser.

I see - You know better than me, if the performance allows updating so many translates per second while actively dragging, does it really have an impact to change the style once for each mousedown/up?

If it does, then this should probably be a flag you can enable if you need it. If it doesn't make an impact really, either change the behavior to always remove on reconcile OR make it a flag but with the default behavior of not leaving those transform: translate3d(0,0,0) traces after reconcilation.

If you do come to a decision, I'd love to try if I can prepare a PR with the necessary changes :)

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

No branches or pull requests

2 participants