Closed Bug 919965 Opened 11 years ago Closed 11 years ago

DnD feedback in the panel is confusing

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [Australis:M9][Australis:P2])

Attachments

(1 file, 14 obsolete files)

50.17 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
Attached video placeholder-confusion.ogg (obsolete) —
I'm not really sure how to describe this better than in the summary, but here's how I feel about it as a user (who is an engineer and tries to figure out what what I see means). 1. the placeholder somewhat follows my drag action, but not completely (e.g. when I drag below the existing buttons, somehow there remains a placeholder in the middle of the existing buttons?) 2. sometimes when I drop something, it ends up in the place of the placeholder, and sometimes it doesn't. There doesn't seem to be any logic to when it happens either way, and this confuses me. 3. Sometimes the placeholder animates to one place, then ends up in another (this may or may not be easily visible in the video, sorry). Mike, if you look at the video, can you see what's going on and/or what we can do to improve this?
Flags: needinfo?(mdeboer)
Component: Theme → Toolbars and Customization
Whiteboard: [Australis:M?][Australis:P?]
I think I have an idea how to improve this... allow me to experiment a bit, tape it and post it back here.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
Attached patch WIP to try out (obsolete) — Splinter Review
Gijs, this is a quick WIP patch I made that makes the DnD UX quite a bit better for me. Could you try it out for me?
Flags: needinfo?(gijskruitbosch+bugs)
Attached video placeholder_still_confused.ogg (obsolete) —
Seems to help a bit with it ending up where you'd expect when hovering over items, but there's still quite some issues: 1) dragging to the end of the area still shows randomly located placeholders, rather than a single placeholder at the end. 2) sometimes (like in this screencast), placeholders briefly appear 'somewhere', only to disappear again (at roughly 8s in, notice the placeholder in the middle of the bottom row, the add-ons button having moved to the fourth row, and the placeholder at the end therefore having moved to the center of the fourth row). 3) sometimes placeholders animate in the wrong direction. STR: a) start with the default layout b) drag preferences/options to the bottom right, to put it at the end c) drag it up one row, don't drop it (wait for the placeholder to follow you - if you look closely you can see it's animating in from the right, outside of view) d) drag it left one item, don't drop it - the placeholder slides out to the right, instead of sliding to the left, and then 'appears' under the cursor e) drag it to the far left - the placeholder now slides right from the middle to the edge, looking particularly odd From a cursory glance at the patch, I figured I should note that: container.insertBefore(aspiringChild, null) works the same as container.appendChild(aspiringChild), so there's no need to nullcheck stuff in some of the cases where you do.
Attachment #809101 - Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [Australis:M?][Australis:P?] → [Australis:M?][Australis:P2]
Taking per discussion with Mike on IRC yesterday (or was it Monday...?).
Assignee: mdeboer → gijskruitbosch+bugs
Depends on: 930161
Depends on: 933731
So I tried to use a similar technique as before, but with separate placeholders. Unfortunately, I'm still seeing a lot of glitches with this patch and it's not clear to me if/how those could be fixed. Jared and I talked about using animated transforms (transposes) instead, so I'll look into doing that next, but I wanted to checkpoint this just in case.
This is mostly done, except I'm still not happy with how it picks its closest element to insert onto (in particular, the fact that it uses the original positions for this which means e.g. hover targets for the wide buttons are currently quite small - I guess it'll be better in the new layout, but still), and the buttons animate back to their original position on drop. All the *horizontal* bits of the transform should snap - vertical only transitions are OK to animate, and look kind of pleasing, actually. Unfortunately the first isn't really trivial to fix, because using the up-to-date client rects while elements are in the middle of transitioning does Bad Things, like going into perpetually wiggling states where it can't decide what you're trying to hover. Not sure if this is even fixable at all. Need to think about it more tomorrow with an un-fried brain.
Attachment #828561 - Attachment is obsolete: true
Comment on attachment 831157 [details] [diff] [review] improve dnd feedback in the panel Review of attachment 831157 [details] [diff] [review]: ----------------------------------------------------------------- Jared and/or Mike, if you have time to simply apply this patch and tell me how it feels / if you run into anything, that might be helpful, but don't bother looking at the code too much yet.
Attachment #831157 - Flags: feedback?(mdeboer)
Attachment #831157 - Flags: feedback?(jaws)
I believe this is ready for review. It's worked very well in my testing, but of course it's quite possible I've missed something...
Attachment #831476 - Flags: review?(jaws)
Attachment #831157 - Attachment is obsolete: true
Attachment #831157 - Flags: feedback?(mdeboer)
Attachment #831157 - Flags: feedback?(jaws)
Comment on attachment 831476 [details] [diff] [review] improve dnd feedback in the panel Annnnd I just realized this breaks tests.
Attachment #831476 - Flags: review?(jaws)
Problem was simply that the _set/_cancelDragActive bits were called before initializing the position managers. Nullchecking to fix this made everything pass.
Attachment #831479 - Flags: review?(jaws)
Attachment #831476 - Attachment is obsolete: true
Comment on attachment 831479 [details] [diff] [review] improve dnd feedback in the panel Review of attachment 831479 [details] [diff] [review]: ----------------------------------------------------------------- Redirecting review as I don't think I'll have time to get to this today or tomorrow.
Attachment #831479 - Flags: review?(jaws) → review?(mdeboer)
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 831479 [details] [diff] [review] improve dnd feedback in the panel Review of attachment 831479 [details] [diff] [review]: ----------------------------------------------------------------- Fantastic work with this behavior. It's miles ahead of the current implementation. So granting f+ for that. Can't wait to review more thoroughly when you restructured things a bit. ::: browser/components/customizableui/src/CustomizeMode.jsm @@ +1359,5 @@ > contents.removeChild(oldPlaceholders[0]); > } > + }, > + > + _initializePositionManagement: function() { I think this can move to a dedicated module as well. You can also do the _nodePositionManagers bookkeeping there. So, the new module would be a AreaPositionManager factory. @@ +1414,5 @@ > function dispatchFunction(aFunc) { > Services.tm.currentThread.dispatch(aFunc, Ci.nsIThread.DISPATCH_NORMAL); > } > + > +function AreaPositionManager(aContainer) { Please put this is a separate, lazy-loadable module. Just like you did with PanelWideWidgetTracker.jsm @@ +1509,5 @@ > + } > + } > + // If we're moving items before a wide node that were already there, > + // it's possible it's not necessary to shift nodes > + // including & after the wide node. nit: superfluous whitespace
Attachment #831479 - Flags: review?(mdeboer) → feedback+
Extracted to a jsm. Took the opportunity to make a global CustomizableUI property out of the wide panel class and the number of columns. I don't think we should make that a pref - all our code assumes a wide widget is however many columns the panel has, and the styling assumes that it's equivalent to three, so we shouldn't pretend that it's easily changeable. This fixes bug 885574.
Attachment #832344 - Flags: review?(mdeboer)
Attachment #831479 - Attachment is obsolete: true
Now with less transitioning on drop.
Attachment #832890 - Flags: review?(mdeboer)
Attachment #832344 - Attachment is obsolete: true
Attachment #832344 - Flags: review?(mdeboer)
Mike told me there's an issue on Windows (null access) and perf issues. I'll upload a patch to fix these shortly. I have to admit I couldn't reproduce the perf issues, but I profiled and made a bunch of tweaks which made the profile a lot emptier, so I hope the next iteration is better. Here's an interdiff: https://pastebin.mozilla.org/3609978
So I ran into some test issues again with the previous interdiff, but those are fixed in this patch, too.
Attachment #832957 - Flags: review?(mdeboer)
Attachment #832890 - Attachment is obsolete: true
Attachment #832890 - Flags: review?(mdeboer)
Not sure how the previous thing passed tests. But here we go, this is nicer.
Attachment #832959 - Flags: review?(mdeboer)
Attachment #832957 - Attachment is obsolete: true
Attachment #832957 - Flags: review?(mdeboer)
Comment on attachment 832959 [details] [diff] [review] improve dnd feedback in the panel Review of attachment 832959 [details] [diff] [review]: ----------------------------------------------------------------- Next one will be r+, because it's all minor stuff. Blown away with the amount of work you put into this; very, very cool! ::: browser/components/customizableui/src/CustomizeMode.jsm @@ +1068,5 @@ > > __dumpDragData(aEvent); > > + // When leaving customization areas, cancel the drag on the last dragover item > + if (this._dragOverItem && aEvent.target == aEvent.currentTarget) { Can you explain in the comment above why `aEvent.target == aEvent.currentTarget` is needed? @@ +1381,5 @@ > return place; > } > > function __dumpDragData(aEvent, caller) { > + if (!gDebug) { YES! ::: browser/components/customizableui/src/DragPositionManager.jsm @@ +15,5 @@ > + > +function AreaPositionManager(aContainer) { > + // Caching the direction and bounds of the container for quick access later: > + let window = aContainer.ownerDocument.defaultView; > + this.dir = window.getComputedStyle(aContainer).direction; this._dir (not used outside this object) @@ +18,5 @@ > + let window = aContainer.ownerDocument.defaultView; > + this.dir = window.getComputedStyle(aContainer).direction; > + let containerRect = aContainer.getBoundingClientRect(); > + this._containerSides = {left: containerRect.left, right: containerRect.right}; > + this.inPanel = aContainer.id == CustomizableUI.AREA_PANEL; this._inPanel (not used outside this object) @@ +26,5 @@ > + > +AreaPositionManager.prototype = { > + update: function(aContainer) { > + let window = aContainer.ownerDocument.defaultView; > + this._store = new WeakMap(); Can you make this name more specific? Something like `this._nodePositionStore` (you really only ref it in `_lazyStoreGet`, so this longer name should be fine) @@ +35,5 @@ > + if (isNodeWide) { > + this._wideCache.add(n.id); > + } > + let coordinates = this._lazyStoreGet(n); > + // We keep a baseline horizontal distance between non-wide nodes around for use nit: I think it's easy to keep this comment within 80 cols @@ +37,5 @@ > + } > + let coordinates = this._lazyStoreGet(n); > + // We keep a baseline horizontal distance between non-wide nodes around for use > + // when we can't compare with previous/next nodes > + if (this._horizontalDistance === null && last && !isNodeWide) { !this._horizontalDistance @@ +40,5 @@ > + // when we can't compare with previous/next nodes > + if (this._horizontalDistance === null && last && !isNodeWide) { > + this._horizontalDistance = coordinates.left - last.left; > + } > + if (isNodeWide) { writing this in ternary style should be fine: ```js last = isNodeWide ? coordinates : null; ``` @@ +46,5 @@ > + } else { > + last = null; > + } > + } > + }, It's a cheap readability win to add a newline between member function definitions. Can you add those throughout? @@ +49,5 @@ > + } > + }, > + find: function(aContainer, aX, aY) { > + let closest = null; > + let minV = Number.MAX_VALUE, minH = Number.MAX_VALUE; please make this: ```js let minV = Number.MAX_VALUE; let minH = Number.MAX_VALUE; ``` @@ +53,5 @@ > + let minV = Number.MAX_VALUE, minH = Number.MAX_VALUE; > + for (let node of aContainer.children) { > + let coordinates = this._lazyStoreGet(node); > + let hDiff = coordinates.x - aX, > + vDiff = coordinates.y - aY; Please make this: ```js let hDiff = coordinates.x - aX; let vDiff = coordinates.y - aY; ``` @@ +54,5 @@ > + for (let node of aContainer.children) { > + let coordinates = this._lazyStoreGet(node); > + let hDiff = coordinates.x - aX, > + vDiff = coordinates.y - aY; > + // For wide widgets, we're always going to be further from the center horizontally. not: 80 cols. @@ +63,5 @@ > + // Square to get positive values > + hDiff *= hDiff; > + vDiff *= vDiff; > + // Prefer things which are vertically closer to us: > + if (vDiff < minV || (vDiff <= minV * 1.2 && hDiff < minH)) { How did you get to this algo? Can you write it down in a docblock comment above the function declaration? Where's the magic constant 1.2 coming from? @@ +96,5 @@ > + let isShifted = false; > + let shiftDown = aWide; > + // The basic idea of this code is: go through all the children, and shift > + // them based on the position they would have if we had inserted something > + // before aBefore. We use CSS transforms for this, which are CSS transitioned Please move this comment to a docblock above this function. @@ +97,5 @@ > + let shiftDown = aWide; > + // The basic idea of this code is: go through all the children, and shift > + // them based on the position they would have if we had inserted something > + // before aBefore. We use CSS transforms for this, which are CSS transitioned > + for (let n of aContainer.children) { Not a fan of 1-char variables. Please use a more descriptive name @@ +99,5 @@ > + // them based on the position they would have if we had inserted something > + // before aBefore. We use CSS transforms for this, which are CSS transitioned > + for (let n of aContainer.children) { > + // Don't need to shift hidden nodes: > + if (n.getAttribute("hidden") == "true") { I *think* `if (node.hidden) {` would suffice @@ +119,5 @@ > + if (this.__undoShift) { > + isShifted = false; > + } > + if (isShifted) { > + // Conversely, if we're adding something before a wide node, for simplicity's nit: 80 cols. @@ +128,5 @@ > + // Determine the CSS transform based on the next node: > + n.style.transform = this._getNextPos(n, shiftDown, aSize); > + } else { > + // If we're not shifting this node, reset the transform > + n.style.transform = ''; Please use double-quotes. Please correct that throughout this file. @@ +144,5 @@ > + return this.inPanel && aNode && aNode.firstChild && > + aNode.firstChild.classList.contains(CustomizableUI.WIDE_PANEL_CLASS); > + }, > + > + // Reset all our transforms, optionally without transitioning them Please transform this to a proper docblock comment @@ +146,5 @@ > + }, > + > + // Reset all our transforms, optionally without transitioning them > + clearPlaceholders: function(aContainer, aNoTransition) { > + for (let n of aContainer.children) { Again, not a fan of single character variable names. @@ +150,5 @@ > + for (let n of aContainer.children) { > + if (aNoTransition) { > + n.setAttribute("notransition", true); > + } > + n.style.transform = ''; double-quotes @@ +162,5 @@ > + > + _getNextPos: function(aNode, aShiftDown, aSize) { > + // Shifting down is easy: > + if (this.inPanel && aShiftDown) { > + return 'translate(0, ' + aSize.height + 'px)'; double-quotes @@ +168,5 @@ > + return this._diffWithNext(aNode, aSize); > + }, > + > + _diffWithNext: function(aNode, aSize) { > + let xDiff, yDiff = null; Please make this: ```js let xDiff; let yDiff = null; ``` @@ +171,5 @@ > + _diffWithNext: function(aNode, aSize) { > + let xDiff, yDiff = null; > + let nodeBounds = this._lazyStoreGet(aNode); > + let side = this.dir == "ltr" ? "left" : "right"; > + let next = this._getVisibleSiblingForDirection(aNode, 'next'); double-quotes @@ +177,5 @@ > + // Usually, there will be a next node to base this on: > + if (next) { > + let otherBounds = this._lazyStoreGet(next); > + xDiff = otherBounds[side] - nodeBounds[side]; > + // If the next node is a wide item in the panel, check if we could maybe just move nit: 80 cols throughout this method. @@ +234,5 @@ > + // Otherwise, we haven't > + yDiff = 0; > + } > + } > + return 'translate(' + xDiff + 'px, ' + yDiff + 'px)'; double-quotes @@ +237,5 @@ > + } > + return 'translate(' + xDiff + 'px, ' + yDiff + 'px)'; > + }, > + > + // Helper function to move a node if there isn't a next node to base its position on docblock comment, please @@ +261,5 @@ > + let nodeBounds = this._store.get(aNode); > + if (!nodeBounds) { > + let rect = aNode.getBoundingClientRect(); > + nodeBounds = {x: rect.left + rect.width / 2, y: rect.top + rect.height / 2, > + top: rect.top, bottom: rect.bottom, left: rect.left, right: rect.right}; it doesn't really hurt to format this struct nicely. @@ +282,5 @@ > + > + _getVisibleSiblingForDirection: function(aNode, aDirection) { > + let rv = aNode; > + do { > + rv = rv[aDirection + 'Sibling']; double-quotes @@ +283,5 @@ > + _getVisibleSiblingForDirection: function(aNode, aDirection) { > + let rv = aNode; > + do { > + rv = rv[aDirection + 'Sibling']; > + } while (rv && rv.getAttribute("hidden") == "true") I *think* `while (rv && rv.hidden)` would suffice @@ +288,5 @@ > + return rv; > + }, > + > + _store: null, > + _wideCache: null, Please define these at the top
Attachment #832959 - Flags: review?(mdeboer) → review-
Attached patch Interdiff (obsolete) — Splinter Review
For context, here's an interdiff of my trying to address all the comments. Not sure it's useful, but just in case... :-)
Attachment #809783 - Attachment is obsolete: true
Attachment #832959 - Attachment is obsolete: true
And here's an actual updated patch.
Attachment #8333539 - Flags: review?(mdeboer)
Attachment #810170 - Attachment is obsolete: true
Comment on attachment 8333539 [details] [diff] [review] improve dnd feedback in the panel Review of attachment 8333539 [details] [diff] [review]: ----------------------------------------------------------------- r- because the panel placeholders behave like widgets when dragging a widget over them. Couple of nits left as well! ::: browser/components/customizableui/src/DragPositionManager.jsm @@ +73,5 @@ > + // Square to get positive values > + hDiff *= hDiff; > + vDiff *= vDiff; > + // Prefer things which are vertically closer to us: > + if (vDiff < minV || (vDiff <= minV * 1.2 && hDiff < minH)) { (this is prolly me not grokking this too well): Where's the magic constant 1.2 coming from (I know what it does), could you add that to the comment above? @@ +81,5 @@ > + } > + } > + return closest; > + }, > + correctDragOverNode: function(aContainer, aDragOver, aX, aY, aDraggedItemId) { nit: there's no harm in allowing functions to breathe, please add an empty line here (just like you did everywhere else) @@ +103,5 @@ > + return aDragOver; > + }, > + > + /** > + * "Insert" a "placeholder" by shifting the subsequent children nit: more words fit on a line here. Can you fix up the alignment a bit? @@ +159,5 @@ > + aNode.firstChild.classList.contains(CustomizableUI.WIDE_PANEL_CLASS); > + }, > + > + /** > + * Reset all the transforms in this container, nit: more words fit on a line here. Can you fix up the alignment a bit? @@ +229,5 @@ > + // and messes with alignments > + yDiff = otherBounds.top - nodeBounds.top; > + } > + } else { > + // We don't have a sibling whose position we can use. First, let's see nit: trailing whitespace. @@ +273,5 @@ > + let xDiff = aNodeBounds[side] - otherBounds[side]; > + // If, however, this means we move outside the container's box > + // (i.e. the row in which this item is placed is full) > + // we should move it to align with the first item in the next row instead > + let bound = this._containerSides[this._dir == "ltr" ? "right" : "left"]; You already did this a couple lines before. `this._containerSides[side];` should do it! @@ +281,5 @@ > + } > + return xDiff; > + }, > + > + // Get position details from our cache. If the node is not yet cached, get its position You forgot to morph this into a docblock comment. @@ +287,5 @@ > + _lazyStoreGet: function(aNode) { > + let nodeBounds = this._nodePositionStore.get(aNode); > + if (!nodeBounds) { > + let rect = aNode.getBoundingClientRect(); > + nodeBounds = {x: rect.left + rect.width / 2, y: rect.top + rect.height / 2, You forgot to format this nicely: ```js { x: rect.left + rect.width / 2, y: rect.top + rect.height / 2, top: rect.top, bottom: rect.bottom, left: rect.left, right: rect.right }; ``` perhaps you can just use the rect object and do: ```js rect.x = rect.left + rect.width / 2; rect.y = rect.top + rect.height / 2; ``` @@ +331,5 @@ > + } else { > + gManagers.set(areaNode, new AreaPositionManager(areaNode)); > + } > + } > + }, nit: forgot to add a newline between functions here.
Attachment #8333539 - Flags: review?(mdeboer) → review-
(In reply to Mike de Boer [:mikedeboer] from comment #23) > @@ +273,5 @@ > > + let xDiff = aNodeBounds[side] - otherBounds[side]; > > + // If, however, this means we move outside the container's box > > + // (i.e. the row in which this item is placed is full) > > + // we should move it to align with the first item in the next row instead > > + let bound = this._containerSides[this._dir == "ltr" ? "right" : "left"]; > > You already did this a couple lines before. `this._containerSides[side];` > should do it! No, side = this._dir == "ltr" ? "left" : "right" - it's the inverse.
(In reply to :Gijs Kruitbosch from comment #24) > No, side = this._dir == "ltr" ? "left" : "right" - it's the inverse. Ah, missed that... my bad!
Attached patch Interdiff (obsolete) — Splinter Review
Attachment #8333538 - Attachment is obsolete: true
Comment on attachment 8333838 [details] [diff] [review] Interdiff Egh, oops.
Attachment #8333838 - Attachment is obsolete: true
Alright, updated patch.
Attachment #8333891 - Flags: review?(mdeboer)
Attachment #8333539 - Attachment is obsolete: true
Comment on attachment 8333891 [details] [diff] [review] improve dnd feedback in the panel Review of attachment 8333891 [details] [diff] [review]: ----------------------------------------------------------------- Apart form two minor things, this is ready to go. Gift-wrapped, heading to an m-c near you! ::: browser/components/customizableui/src/DragPositionManager.jsm @@ +332,5 @@ > + do { > + rv = rv[aDirection + "Sibling"]; > + } while (rv && rv.getAttribute("hidden") == "true") > + return rv; > + }, unnecessary trailing comma. @@ +356,5 @@ > + }, > + > + getManagerForArea: function(aArea) { > + return gManagers.get(aArea); > + }, unnecessary trailing comma.
Attachment #8333891 - Flags: review?(mdeboer) → review+
With nits fixed: https://hg.mozilla.org/integration/fx-team/rev/77c3e8e02df4 Just wondering if we need to do something specific with post-merge landing bugs, needinfo'ing jaws about that.
Flags: needinfo?(jaws)
Whiteboard: [Australis:M?][Australis:P2] → [Australis:M9][Australis:P2][fixed-in-fx-team]
Flags: needinfo?(jaws)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P2][fixed-in-fx-team] → [Australis:M9][Australis:P2]
Target Milestone: --- → Firefox 28
Depends on: 940387
Wow, this is like night and day. Thanks. Now back to minding the etiquette...
Depends on: 963773
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: