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

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1355507 - Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place.

Review commit: https://reviewboard.mozilla.org/r/130198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130198/

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1355507 - Releasing a tab while dragging through the tabstrip on the same window should show a transition to its final resting place.

Review commit: https://reviewboard.mozilla.org/r/130200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130200/

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130200/diff/1-2/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130200/diff/2-3/

Comment 6

[Dão Gottwald [:dao]]

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...

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Hey Dao, have you been able to make any more progress on the review?

Comment 9

[Dão Gottwald [:dao]]

I hope to finish this review tomorrow.

Comment 10

[Dão Gottwald [:dao]]

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?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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 12

[Jared Wein [:jaws] (please needinfo? me)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130200/diff/3-4/

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 14

[Dão Gottwald [:dao]]

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 15

[Jared Wein [:jaws] (please needinfo? me)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130200/diff/4-5/

Comment 16

[Dão Gottwald [:dao]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for eslint failures: https://treeherder.mozilla.org/logviewer.html#?job_id=96064966&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/27a562a90829b370c73b221c816f0f58cc715d53

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Also appears to have caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=96070631&repo=autoland

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130200/diff/5-6/

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

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

[Dão Gottwald [:dao]]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/709528f44f78

Comment 25

[Sören Hentzschel]

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

[Dão Gottwald [:dao]]

(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

[Sören Hentzschel]

(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

[Justin [:JW_SoftvisionQA]]

I have verified this change in the current Nightly.

Comment 30

[Dão Gottwald [:dao]]

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.

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

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