Tab moving/reordering should use the photon animation curve

ASSIGNED
Assigned to

Status

()

Firefox
Theme
P1
normal
ASSIGNED
16 days ago
9 hours ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks: 1 bug)

55 Branch
Points:
5
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox55 affected)

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.
Whiteboard: [photon]
Whiteboard: [photon] → [photon-animation]
Points: --- → 5

Updated

15 days ago
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 hidden (mozreview-request)

Comment 6

8 days 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

a day 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

9 hours 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)
You need to log in before you can comment on or make changes to this bug.