Tab moving/reordering should use the photon animation curve

VERIFIED FIXED in Firefox 55

Status

()

P1
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

55 Branch
Firefox 55
Points:
5
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-animation])

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 8857005 [details]
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] → [photon-animation]
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 hidden (mozreview-request)

Comment 6

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

2 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?
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.
Comment hidden (mozreview-request)
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 14

2 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.
Attachment #8858227 - Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request)

Comment 16

2 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
Attachment #8858227 - Flags: review?(dao+bmo) → review+

Comment 17

2 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
Comment hidden (mozreview-request)
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 22

2 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
Attachment #8858227 - Flags: review?(dao+bmo) → review+

Comment 23

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/709528f44f78
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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)

Updated

2 years ago
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

Updated

2 years ago
Blocks: 1367136
I have verified this change in the current Nightly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

2 years ago
Depends on: 1374052
Depends on: 1387165
Depends on: 1396833
You need to log in before you can comment on or make changes to this bug.