Tab moving/reordering should use the photon animation curve

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
2 months ago
5 days 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])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

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

Updated

2 months ago
Whiteboard: [photon]
(Assignee)

Updated

2 months ago
Whiteboard: [photon] → [photon-animation]
(Assignee)

Updated

2 months ago
Points: --- → 5

Updated

2 months ago
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
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)

Updated

2 months ago
Iteration: --- → 55.4 - May 1
Comment hidden (mozreview-request)

Comment 6

2 months 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)

Comment 9

2 months ago
I hope to finish this review tomorrow.
Flags: needinfo?(dao+bmo)

Comment 10

2 months 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)
(Assignee)

Comment 11

2 months 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)
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)

Updated

2 months ago
Iteration: 55.4 - May 1 → 55.5 - May 15

Comment 14

2 months 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 months 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 months 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
Flags: needinfo?(jaws)
Also appears to have caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=96070631&repo=autoland
(Assignee)

Updated

2 months ago
Flags: needinfo?(jaws)
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 months 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 months 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 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/709528f44f78
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 25

2 months 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…
(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

a month ago
Depends on: 1364003
No longer depends on: 1364003

Updated

a month ago
Depends on: 1364003

Comment 27

a month 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.
Flags: needinfo?(cadeyrn)

Updated

a month ago
Depends on: 1364669
Depends on: 1366060

Updated

a month ago
Blocks: 1367136
I have verified this change in the current Nightly.
Status: RESOLVED → VERIFIED

Updated

15 days ago
Flags: qe-verify+

Updated

5 days ago
Depends on: 1374052
You need to log in before you can comment on or make changes to this bug.