Page MenuHomePhabricator

fix column resizing in run view
ClosedPublic

Authored by prha on Aug 9 2019, 10:21 PM.

Details

Reviewers
bengotow
alangenfeld
Group Reviewers
Restricted Project
Commits
R1:ebaa4272c8ef: fix column resizing in run view
Summary

The HTML5 attribute draggable on a styled component container was causing the mouseup event to get swallowed up. Renaming the prop (used to key dynamic styles) caused the mouse events to fire as expected.

Refactored some of the document listeners so that they are tied to the component
lifecycle, instead of to UI events.

Issue: https://github.com/dagster-io/dagster/issues/1637

Test Plan

Resized columns in Firefox/Safari/Chrome.

Diff Detail

Repository
R1 dagster
Branch
prha/column_resizing
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

prha created this revision.Aug 9 2019, 10:21 PM
prha added a reviewer: Restricted Project.Aug 9 2019, 10:26 PM
prha edited the summary of this revision. (Show Details)Aug 12 2019, 4:19 PM
max added a subscriber: max.Aug 12 2019, 6:59 PM

oohh πŸ‘€

alangenfeld resigned from this revision.Aug 12 2019, 9:28 PM
alangenfeld added a subscriber: alangenfeld.

defer to blessed UI wizard @bengotow

bengotow added inline comments.Aug 13 2019, 2:20 PM
js_modules/dagit/src/runs/LogsScrollingTableHeader.tsx
75

Hey! Hmm, so I'm not sure about this - just a couple thoughts:

  • We're calling this interaction a drag in the code, but it doesn't really have much to do with the DOM drag/drop concept, where there's a payload being carried with the drag and you specify drop targets. I think that implementing the drag event without the dragover event or drop event is essentially a sort of no-op. We start a drag session but then don't allow the user to drop anywhere and when you mouse up it's officially cancelled. This will work, but the mousedown / mousemove / mouseup combo /should/ work in every browser. I think the fact that it was getting stuck in the dragging phase is more likely something else going on...
  • I /think/ what's happening here is that you're ending the mouse interaction over another element that handles mouseup and stops propagation, so the mouseup never bubbles up to document and calls this.onDragEnd. I think that the fix is probably to make these addEventListener and removeEventListener calls pass true as the third parameter, which specifies that the event handlers should be dispatched in the capture phase (on the way DOWN to the target element on the page, rather than on the way back UP. Essentially that would result in this mouseup handler being called first before anything else has a chance to cancel the event.)
  • I think that removing these event listeners on componentWillUnmount is a good idea (syncing it up with the component lifecycle), but I think it's slightly preferable to wait to attach the handlers until the drag interaction starts because mousemove event handlers come with a slight performance overhead.

Overall though, if this fixes the issue none of these are huge blockers!

bengotow accepted this revision.Aug 13 2019, 2:21 PM

Going to approve with comments, because I'm not certain that my suggested fix will work. If it doesn't, this seems like a good solution.

This revision is now accepted and ready to land.Aug 13 2019, 2:21 PM
prha updated this revision to Diff 3679.Aug 14 2019, 12:04 AM

updated styled.div attributes to not collide with HTML5 drag attrs

prha retitled this revision from handle variety of browser drag events for column resizing to fix column resizing in run view.Aug 14 2019, 12:08 AM
prha edited the summary of this revision. (Show Details)
prha added a comment.Aug 14 2019, 12:12 AM

Yeah, kept digging into why the mouseup event wasn't firing in Chrome/Safari. Turns out, they swallow the mouseup event for elements with the draggable attribute that has initiated a drag event.

We were passing draggable in to the styled component to change the cursor style. Renaming to isDraggable fixes the issue.

This revision was landed with ongoing or failed builds.Aug 14 2019, 12:17 AM
This revision was automatically updated to reflect the committed changes.