Allow Styling Phoenix Uploads On Drag And Drop

[probably-not]

This extends the current Phoenix drop target functionality for Phoenix Uploads by adding (and removing) the phx-drop-target-active class to a dropzone element whenever a file is being dragged over it.

The current Phoenix Uploads functionality is very limited - it does not notify anywhere on the DOM that something is being dragged in, making it impossible with plain Phoenix LiveView to enable things like styling elements when files are dragged over a dropzone. This is a very important functionality for good UI, allowing us to show the user that drag and drop is enabled, and comes standard in many uploading libraries that enable uploads for frontend frameworks.

This PR extends the current LiveSocket listeners by adding 2 more:

• dragenter
• dragleave

These listeners add and remove the phx-drop-target-active class respectively, which enables adding a TailwindCSS variant like so:

@custom-variant phx-drop-target-active (.phx-drop-target-active&, .phx-drop-target-active &);

This variant can be used similarly to phx-click-loading and the other recommended variants that Phoenix ships with, for example like so:

<div phx-drop-target={@upload.ref} class="phx-drop-target-active:scale-105">
  <.live_file_input upload={@upload} class="file-input-sm" />
</div>

In this example, when a file is dragged over the dropzone element, the element grows in size (a very naive example, but it shows the effect of the variant).

With this class being added and through Tailwind's arbitrary state selectors, we could also do things like style the entire page or sibling elements based on whether this class exists on the dropzone.

[probably-not]

I'm not sure why that mix test is failing - looks like it's related to some warnings as errors for a few of the Elixir files? I haven't touched them so I think it's not necessarily relevant to this PR.

[probably-not]

@SteffenDE @chrismccord Bumping this - we'd really like to see it go in (and backported to the 1.1 branch so we can get to using it ASAP). It would be a huge benefit and enable a ton of extra UI/UX functionalities in LiveView for anyone doing Uploads without needing to pull in any external libraries and integrating them with the LiveView model.

[SteffenDE]

I wanted to get feedback from Chris, since I don't have much experience with the upload code. Overall, looks good to me. One suggestions about constant usage and maybe change of naming.

[probably-not]

@chrismccord Bumping this again - any chance we can get this in soon?

[probably-not]

@SteffenDE Is there anyone else that can handle a review here? I see Chris assigned himself but that was a month ago and it doesn't look like he's responding to pings. Can we maybe get a review from someone else in order to get this in? I'd love to get it in soon, this would remove an entire class of libraries that we are currently pulling in for styling on uploads.

[SteffenDE]

Sorry for the delay. Let's call the class phx-drop-target-active and the constant PHX_DROP_TARGET_ACTIVE_CLASS and imho we're good to go.

[probably-not]

Awesome, thanks!

I'll start adjusting the docs that need adjusting. Regarding tests - any hints/preferences as to how to go about adding any (if necessary)?

[SteffenDE]

You could try adding a playwright test, but I don’t think it’s necessary, as the code is straightforward enough :)

[probably-not]

@SteffenDE I've update the docs - let me know if this is alright or if I should make changes to the docs 👍

[SteffenDE]

A couple more comments.

Thinking about this, I'm not 100% convinced any more that we need to add this as first class feature to LiveView, since you can implement it in a couple of lines in your app.js:

window.addEventListener("dragover", (e) => {
  const phxDropTarget = (e.target.closest("[phx-drop-target]"));
  if (phxDropTarget) {
    liveSocket.js().addClass(phxDropTarget, "phx-drop-target-active");

    phxDropTarget.addEventListener("dragleave", () => {
      liveSocket.js().removeClass(phxDropTarget, "phx-drop-target-active");
    }, { once: true });
  }
});

The benefit is that LiveView has one less event listener that's executed for all dragovers, even when users don't use the feature. Wdyt? 🤔

[probably-not]

Thinking about this, I'm not 100% convinced any more that we need to add this as first class feature to LiveView, since you can implement it in a couple of lines in your app.js

I don't agree with this - mostly because I personally believe that this can most likely be stated about almost every feature in LiveView that relates to the JS side of things, except for the patching and socket behaviour that LiveView gives. For example - why provide the InfiniteScroll hook as a LiveView first-class citizen, or the FocusWrap hook and component? Both of these can be written by anyone with JS knowledge - but they are fairly integral to building Full Stack apps, so it's much better for the developer community that the framework provides it out of the box.

File Uploads are a pretty universal feature - especially in recent times with the advent of conversational apps that want people to upload data to expand context for their AI agents. Being able to natively style things out of the box, without having to start understanding the overhead of which event listeners to listen to, what the implementation needs to be, etc. gives a huge boost to any framework.

Actually, your last comment is a perfect example of this!. I've been using LiveView in our production codebase for the last 2 years and we have around 5000 lines of JS/TS code for interop on the browser, and I didn't know that I have to use liveSocket.js() in order to ensure that the classlist won't be patched when a patch happens. So if I tried implementing it, I would have ended up having a lot of issues and most likely opening up issues here trying to understand why it's happening.

At the end of the day, you're the maintainer and I fully respect any decision you might want to make here 😄 I know that every feature that's added just adds a maintenance burden on you and I definitely don't want to make your job more difficult 😅 If you believe that it's a good idea to keep this not as a first class citizen I'm happy to close this PR and maybe make a library with an igniter script for this or something - but my view is that the less things that people need to find and install in order to get a full stack web app, the better 🤷‍♂️

[SteffenDE]

Right, it's more complex than I thought, so that's a valid argument for it being in LiveView. My only concern is the extra event listeners that will be called even when not using this feature and require traversing the DOM for closestPhxBinding. But I guess it's fine. I just worry we'll have to optimize this in the future in some way.

[probably-not]

My only concern is the extra event listeners that will be called even when not using this feature and require traversing the DOM for closestPhxBinding. But I guess it's fine. I just worry we'll have to optimize this in the future in some way.

I wonder if we could optimize this somehow... maybe some sort of WeakMap to cache the DOM traversal for a certain parent tree? Something that would let us not have to continuously traverse but also point to the closest binding. Any thoughts?

I'm going to ruminate on this for a bit - I'm sure there is an optimization in here that would work. I'll look at this over the weekend and see what I can come up with, maybe I can cobble together something that would be better.

Possible "quick-win" solution to avoid this affecting 100% of LiveView users - make it a configuration on the LiveSocket initialization, like the metadata object for the different events, a flag that enables/disables it?

[SteffenDE]

Let's optimize if necessary. Thank you! :)

[probably-not]

Thanks @SteffenDE!!

[DVSLabs]

It's nice but perhaps the naming for the classes could be a bit more consistent. phx-drop-target-active vs drop-target doesn't make much sense.
❤️

[SteffenDE]

@DVSLabs the classes LiveView set all follow that form (phx-*), I'm not sure what you mean?

https://github.com/phoenixframework/phoenix/blob/3755ea508b1c7489aaa6594039ad62ba8d46e98c/installer/templates/phx_assets/app.css#L95-L97

[DVSLabs]

@SteffenDE sorry I should have been more descriptive.

Those constants are defined in https://github.com/phoenixframework/phoenix_live_view/blob/v1.1.18/assets/js/phoenix_live_view/constants.js#L6_L15

The way that styling works for events is that you can do a class="phx-click-loading:opacity-50" which will apply a 50% opacity when the event is active and removes the class when inactive.

This change works differently because it doesn't remove the variant. If you hover over a drop zone it will apply phx-drop-target-active defined on https://github.com/phoenixframework/phoenix_live_view/blob/v1.1.18/assets/js/phoenix_live_view/constants.js#L16 but it will not remove the class. Instead you have to style a "base" drop target using drop-target defined on http://github.com/phoenixframework/phoenix_live_view/blob/v1.1.18/assets/js/phoenix_live_view/constants.js#L30.

In other words you style the drop target like class="phx-drop-target-active:scale-105 drop-target:scale-100". If you don't add drop-target:scale-100 in this example once you hover over the drop target it will apply scale-105 and when you leave the drop zone the scale-105 will remain.

[SteffenDE]

but it will not remove the class

The class should be removed on drop and on dragleave. If it's not that's a bug.

[DVSLabs]

@SteffenDE I'll work on a minimal POC to eliminate any complicating factors from my own testing.

[DVSLabs]

@SteffenDE it's actually a browser bug with Firefox 😅 sorry for the noise

bug.mov

[SteffenDE]

I assume you can "fix" it by dragging over it again when it's not obscured any more? We could handle it by looking at mouse moves, but probably too expensive for an edge case that seems unlikely to happen frequently in practice. If it's more common though we might need to look into working around it..

[DVSLabs]

User can fix it with a dragleave event somewhere in the browser but not another window. I will raise it with Mozilla.

[DVSLabs]

https://github.com/phoenixframework/phoenix_live_view/blob/v1.1.18/assets/js/phoenix_live_view/live_socket.js#L699-L702

On Chrome clientX and clientY both return 0 when the co-ordinates are outside the browser. On Firefox they return actual values based on global position.

There's an old bug report on Mozilla when they used to report 0 https://bugzilla.mozilla.org/show_bug.cgi?id=505521.

According to that thread the spec is vague on what to report so it's likely both browser are compliant. Probably should be fixed here. What do you think?

If you want to play around with it I've attached some POCs, just be sure to add @custom-variant phx-drop-target-active (.phx-drop-target-active&, .phx-drop-target-active &); to app.css.

https://gist.github.com/DVSLabs/b8790a2d287e9db2f9c618f56f8daf9a