dioxus icon indicating copy to clipboard operation
dioxus copied to clipboard

use DragEvent not MouseEvent for drag events

Open rogusdev opened this issue 1 year ago • 1 comments

Resolves #3133 maybe?

rogusdev avatar Oct 28 '24 23:10 rogusdev

~Needs to convert the DragEvent into MouseEvent for some of the functions now~

Resolved

rogusdev avatar Oct 28 '24 23:10 rogusdev

Looks great! 👍 I just edited this to match the other web events that aren't using a wrapper struct (like WebDragData) and simplify because web_sys::DragEvent derefs to a web_sys::MouseEvent

matthunz avatar Oct 30 '24 21:10 matthunz

@matthunz sorry I did not see your changes before I force pushed my updates, because I found an issue. Please re-apply your updates you had done before, or just recommend them and I can add them.

@ealmloff your changes in https://github.com/DioxusLabs/dioxus/commit/cda8bb24f103c5de91f2859b40c884d8db73023c#diff-517df2174967af5678c41e224b2d9a1f1e63e7fcdfe46bc4108c14bdf4defff2R25 broke downcast, I suspect for all event types. This is because self.raw is no longer a web_sys::*Event, instead they are all Synthetic<web_sys::*Event> and Synthetic is crate visibility only. So the as_any called by downcast needs to change to return the actual web_sys event, and not the Synthetic wrapper, otherwise outside code cannot access the wrapped web_sys events.

Given the last bit, I am not sure if this needs a larger set of changes to all the events, or if this is fine for now, or what. Please advise.

rogusdev avatar Oct 30 '24 21:10 rogusdev

@ealmloff your changes in https://github.com/DioxusLabs/dioxus/commit/cda8bb24f103c5de91f2859b40c884d8db73023c#diff-517df2174967af5678c41e224b2d9a1f1e63e7fcdfe46bc4108c14bdf4defff2R25 broke downcast, I suspect for all event types. This is because self.raw is no longer a web_sys::*Event, instead they are all Synthetic<web_sys::*Event> and Synthetic is crate visibility only. So the as_any called by downcast needs to change to return the actual web_sys event, and not the Synthetic wrapper, otherwise outside code cannot access the wrapped web_sys events.

Given the last bit, I am not sure if this needs a larger set of changes to all the events, or if this is fine for now, or what. Please advise.

Yes, applying that change to all events would be great. Many people will be downcasting to the web-sys counterpart from previous releases, so if we can not change the type it downcasts to that is better

ealmloff avatar Oct 30 '24 22:10 ealmloff

Oh my fault I didn't mean to get in the way @rogusdev I just re-applied those changes I hope that's all good.

I also added your fix for downcasting to all other events

matthunz avatar Oct 30 '24 22:10 matthunz

@matthunz thanks! Your cleanup looks ~promising~ like a significant improvement! :) that said, I see a failing ci job now. Any idea what that is about?

rogusdev avatar Oct 30 '24 23:10 rogusdev

@matthunz I re-added the web_sys event reference for the drag.rs as_any-s because those got lost with the re-applying of the old commit

rogusdev avatar Oct 30 '24 23:10 rogusdev

I also notice that web/events/mouse has these tidbits:

impl HasFileData for Synthetic<MouseEvent> {}

impl HasDragData for Synthetic<MouseEvent> {
    fn as_any(&self) -> &dyn std::any::Any {
        &self.event
    }
}

Perhaps these were just because of the old dragevent implementation and we can get rid of these now?...

I suspect they are vestigial now, and only added before for the slightly inaccurate previous structure of things, bc MDN does not mention files or drag anything for mouseevent: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent

rogusdev avatar Oct 31 '24 00:10 rogusdev

@matthunz I re-added the web_sys event reference for the drag.rs as_any-s because those got lost with the re-applying of the old commit

Ah man I totally missed that thanks!

matthunz avatar Oct 31 '24 00:10 matthunz

@matthunz thanks! Your cleanup looks ~promising~ like a significant improvement! :) that said, I see a failing ci job now. Any idea what that is about?

Awesome! (: it looks like we might have a CI issue - looks like some links in various READMEs aren’t loading correctly for the runner

matthunz avatar Oct 31 '24 00:10 matthunz

Do you want to approve for me to merge as-is and you fix the readmes separately?

rogusdev avatar Oct 31 '24 01:10 rogusdev

Thanks, this looks much better. The readme links should be fixed automatically when the docsite is back to normal

ealmloff avatar Oct 31 '24 14:10 ealmloff