Closed Bug 1355507 Opened 6 years ago Closed 6 years ago

Tab moving/reordering/drag & drop should use the photon animation curve

Categories

(Firefox :: Theme, enhancement, P1)

Product:
Firefox
Component:
Theme
Version:
55 Branch
Type:
enhancement
Points:
5

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Iteration:
55.5 - May 15
Tracking Flags:
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)

Reorder_Tabs.mp4
482.21 KB, video/mp4
Details
Bug 1355507 - Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place.
59 bytes, text/x-review-board-request
dao
: review+
Details
Attached video Reorder_Tabs.mp4 — 
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.
Whiteboard: [photon]
Whiteboard: [photon] → [photon-animation]
Points: --- → 5
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
Attachment #8858226 - Attachment is obsolete: true
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.
Attachment #8858227 - Flags: feedback?(dao+bmo)
Iteration: --- → 55.4 - May 1
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...
(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.
Hey Dao, have you been able to make any more progress on the review?
Flags: needinfo?(dao+bmo)
I hope to finish this review tomorrow.
Flags: needinfo?(dao+bmo)
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?
Attachment #8858227 - Flags: review?(dao+bmo)
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.
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.
Flags: needinfo?(dao+bmo)
Iteration: 55.4 - May 1 → 55.5 - May 15
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.
Attachment #8858227 - Flags: review?(dao+bmo) → 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
Attachment #8858227 - Flags: review?(dao+bmo) → review+
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
Flags: needinfo?(jaws)
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?
Flags: needinfo?(dao+bmo)
Attachment #8858227 - Flags: review+ → review?(dao+bmo)
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
Attachment #8858227 - Flags: review?(dao+bmo) → review+
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
https://hg.mozilla.org/mozilla-central/rev/709528f44f78
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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…
(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.
Flags: needinfo?(cadeyrn)
Depends on: 1364003
No longer depends on: 1364003
Depends on: 1364003
(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.
Flags: needinfo?(cadeyrn)
Depends on: 1364669
Depends on: 1366060
Blocks: 1367136
I have verified this change in the current Nightly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1374052
Depends on: 1387165
Depends on: 1396833
Depends on: 1529689

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.

Flags: needinfo?(jaws)
No longer depends on: 1387165

(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?

Flags: needinfo?(jaws) → needinfo?(shorlander)
Type: defect → enhancement
Regressions: 1561850
No longer blocks: 1367136
Regressions: 1536480
Summary: Tab moving/reordering should use the photon animation curve → Tab moving/reordering/drag & drop should use the photon animation curve
You need to log in before you can comment on or make changes to this bug.