Tab moving/reordering/drag & drop should use the photon animation curve
Categories
(Firefox :: Theme, enhancement, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jaws, Assigned: jaws, NeedInfo)
References
(Regressed 1 open bug)
Details
(Whiteboard: [photon-animation])
Attachments
(2 files, 1 obsolete file)
Tab moving/reordering should always slide tabs in to the final place and should use the photon animation curve in doing so. Please see the attached UX design video for better description.
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 4•7 years ago
|
||
Comment on attachment 8858227 [details] Bug 1355507 - Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place. Dao, what do you think about the patch? It could probably be cleaned up some more (the duplication in the "drop" method specifically), but I'd like to get your opinion before pushing on it further.
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8858227 [details] Bug 1355507 - Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place. https://reviewboard.mozilla.org/r/130200/#review134376 ::: browser/base/content/browser.css:198 (Diff revision 3) > pointer-events: none; /* avoid blocking dragover events on scroll buttons */ > } > > +.tabbrowser-tab[tabdrop-samewindow], > .tabbrowser-tabs[movingtab] > .tabbrowser-tab[fadein]:not([selected]) { > - transition: transform 200ms ease-out; > + transition: transform 200ms cubic-bezier(.07, .95, 0, 1); Is this reusable with a define or CSS variable? Trying to make sense of the JS code now...
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #6) > > - transition: transform 200ms ease-out; > > + transition: transform 200ms cubic-bezier(.07, .95, 0, 1); > > Is this reusable with a define or CSS variable? Yes, I've been trying to coordinate with sfoster and squib since we will all be using it. I can introduce that variable as part of this patch.
Assignee | ||
Comment 8•7 years ago
|
||
Hey Dao, have you been able to make any more progress on the review?
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8858227 [details] Bug 1355507 - Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place. https://reviewboard.mozilla.org/r/130200/#review136920 ::: browser/base/content/tabbrowser.xml:6939 (Diff revision 3) > + oldTranslateX - translateOffset; > + } else { > + newTranslateX = -translateOffset > tabWidth / 2 ? > + oldTranslateX - tabWidth - translateOffset : > + oldTranslateX - translateOffset; > + } how about: newTranslateX = oldTranslateX - translateOffset; if (oldTranslateX > 0 && translateOffset > tabWidth / 2) { newTranslateX += tabWidth; } else if (oldTranslateX < 0 && -translateOffset > tabWidth / 2) { newTranslateX -= tabWidth; } ::: browser/base/content/tabbrowser.xml:6946 (Diff revision 3) > + draggedTab._dragData.animDropIndex; > + if (dropIndex && dropIndex > draggedTab._tPos) > + dropIndex--; > + if (oldTranslateX && oldTranslateX != newTranslateX) { > + draggedTab.setAttribute("tabdrop-samewindow", "true"); > + draggedTab.style.transform = `translateX(${newTranslateX}px)`; Template literals seem overkill for this, please use ordinary strings and operators. ::: browser/base/content/tabbrowser.xml:6953 (Diff revision 3) > + draggedTab.removeAttribute("tabdrop-samewindow"); > > - // actually move the dragged tab > - if ("animDropIndex" in draggedTab._dragData) { > - let newIndex = draggedTab._dragData.animDropIndex; > - if (newIndex > draggedTab._tPos) > + this._finishAnimateTabMove(); > + if (dropIndex !== false) > + this.tabbrowser.moveTabTo(draggedTab, dropIndex); > + }, {once: true}); So this only moves the tab after the transition has finished, which seems suboptimal. Would it be feasible to move the tab and then apply the transform and animation?
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8858227 [details] Bug 1355507 - Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place. https://reviewboard.mozilla.org/r/130200/#review136920 > So this only moves the tab after the transition has finished, which seems suboptimal. Would it be feasible to move the tab and then apply the transform and animation? I don't see how we could call moveTabTo before the transition has finished without introducing a style flush. moveTabTo calls this.tabContainer.insertBefore, meaning that all of the offsets that were calculated are now incorrect since they are based off of the original position of the element. If we call moveTabTo first, then we will need to change the offset to be relative to the new position, and introduce a style flush to trigger the transition to start from the correct place.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
Hey Dão, can you expedite the review for this? I will be away Wednesday, Thursday, and Friday of this week and it would be great if I could get this fixed before my time off.
Updated•7 years ago
|
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8858227 [details] Bug 1355507 - Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place. https://reviewboard.mozilla.org/r/130200/#review138588 ::: browser/base/content/tabbrowser.xml:6936 (Diff revision 4) > + if (oldTranslateX > 0 && translateOffset > tabWidth / 2) { > + newTranslateX += tabWidth; > + } else if (oldTranslateX < 0 && -translateOffset > tabWidth / 2) { > + newTranslateX -= tabWidth; > + } > + let dropIndex = "animDropIndex" in draggedTab._dragData && nit: please add a blank line above this one ::: browser/base/content/tabbrowser.xml:6940 (Diff revision 4) > + } > + let dropIndex = "animDropIndex" in draggedTab._dragData && > + draggedTab._dragData.animDropIndex; > + if (dropIndex && dropIndex > draggedTab._tPos) > + dropIndex--; > + if (oldTranslateX && oldTranslateX != newTranslateX) { ditto ::: browser/base/content/tabbrowser.xml:6944 (Diff revision 4) > + dropIndex--; > + if (oldTranslateX && oldTranslateX != newTranslateX) { > + draggedTab.setAttribute("tabdrop-samewindow", "true"); > + draggedTab.style.transform = "translateX(" + newTranslateX + "px)"; > + draggedTab.addEventListener("transitionend", () => { > + draggedTab.removeAttribute("tabdrop-samewindow"); The transitionend event bubbles, so you'll need to check event.originalTarget. Should check the transitioning property too.
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8858227 [details] Bug 1355507 - Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place. https://reviewboard.mozilla.org/r/130200/#review138640
Comment 17•7 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/973df2a1ec63 Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place. r=dao
Backed out for eslint failures: https://treeherder.mozilla.org/logviewer.html#?job_id=96064966&repo=autoland https://hg.mozilla.org/integration/autoland/rev/27a562a90829b370c73b221c816f0f58cc715d53
Also appears to have caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=96070631&repo=autoland
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
Comment on attachment 8858227 [details] Bug 1355507 - Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place. Dao, can you review the browser_tabReorder.js change?
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8858227 [details] Bug 1355507 - Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place. https://reviewboard.mozilla.org/r/130200/#review140074
Comment 23•7 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/709528f44f78 Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place. r=dao
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/709528f44f78
Comment 25•7 years ago
|
||
Is this intended that there is now a tab shape on an inactive tab after moving one tab from the right to the left (on macOS)? That's really weird. I guess it has something to do with this ticket…
Comment 26•7 years ago
|
||
(In reply to Sören Hentzschel from comment #25) > Is this intended that there is now a tab shape on an inactive tab after > moving one tab from the right to the left (on macOS)? That's really weird. I > guess it has something to do with this ticket… Could you please file a new bug on this? Thanks.
Comment 27•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #26) > (In reply to Sören Hentzschel from comment #25) > > Is this intended that there is now a tab shape on an inactive tab after > > moving one tab from the right to the left (on macOS)? That's really weird. I > > guess it has something to do with this ticket… > > Could you please file a new bug on this? Thanks. Sorry for the delay. I filed bug 1364669.
Comment 28•6 years ago
|
||
I have verified this change in the current Nightly.
Updated•6 years ago
|
Comment 30•5 years ago
|
||
Jaws, calling moveTabTo after the transition caused a bunch of polish issues that are still unaddressed, see this bug's dependencies. Can you come up with a plan to address those? Otherwise I think we should revert that change.
Assignee | ||
Comment 31•5 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #30)
Jaws, calling moveTabTo after the transition caused a bunch of polish issues that are still unaddressed, see this bug's dependencies. Can you come up with a plan to address those? Otherwise I think we should revert that change.
I looked through the three open issues, one of which was fixed/not-a-regression from this work. One I believe is a platform issue with drop/mouseenter (bug 1364669).
The last issue is indeed a regression from this bug, and I don't see a simple way to fix this aside from trying to store what would be the future tab position and referencing that in various places. This seems error-prone and not something we should do. But I do think that this feature is worth this narrow regression of bug 1529689. Stephen, if we have to choose between animating the placement of the tab drop and bug 1529689, what would you do?
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Description
•