1932425 - When dragging tabs, move other tabs over when hitting 70% instead of 50% of their width

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, task, P1)

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1932425 - When dragging tabs, move other tabs over when hitting 70% instead of 50% of their width. r?sthompson,dwalker

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44fc1ca324c1
When dragging tabs, move other tabs over when hitting 70% instead of 50% of their width. r=sthompson,tabbrowser-reviewers

Comment 3

[agoloman]

Backed out for causing bc failures @browser_multiselect_tabs_reorder.js.

• Backout link
• Push with failures
• Failure Log

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63c01df97d4c
When dragging tabs, move other tabs over when hitting 70% instead of 50% of their width. r=sthompson,tabbrowser-reviewers

Comment 5

[amarc]

https://hg.mozilla.org/mozilla-central/rev/63c01df97d4c

Comment 6

[tabmix.onemen]

After this patch landed tab can move before 70% when drag direction change.

Steps to reproduce:

1. Set browser.tabs.groups.enabled to true.
2. browser.tabs.groups.dragOverThresholdPercent is 20 (the default).
3. Start to drag a tab forward.
4. When you reach 20% of the tab width both tab marked for grouping, background tab did NOT move.
5. Now, instead of keep moving forward, move backward few pixels, the background tab moves left although the drag position is about 20%.

Expected behavior:

When dragging back and exiting the "threshold zone" for grouping no tab supposed to move and you probably need to unmarked the tabs for grouping.

Comment 7

[tabmix.onemen]

Some more edge cases (tested on clean profiles):

• Firefox 133.0b9 - tab-grouping index is wrong in RTL language, the index for the tab to group with the dragged tab is off by one.
• Firefox 134.01 - when dragging forward selecting tabs for grouping swap the order of the tabs, the dragged tab moves after the grouped tab.
• Firefox 134.01 - drag over the point that make background tab(s) shift, don't end the drag, now if you drag back, no other tabs are selected for grouping only the dragged tab. if you accept the group only the dragged tab will be in the new group.

Comment 8

[Dão Gottwald [:dao]]

(In reply to tabmix.onemen from comment #6)

After this patch landed tab can move before 70% when drag direction change.
[...]

Thanks, I've reopened bug 1932695.

(In reply to tabmix.onemen from comment #7)

Some more edge cases (tested on clean profiles):

• Firefox 133.0b9 - tab-grouping index is wrong in RTL language, the index for the tab to group with the dragged tab is off by one.

I take it you can't reproduce this in Nightly?

• Firefox 134.01 - when dragging forward selecting tabs for grouping swap the order of the tabs, the dragged tab moves after the grouped tab.
• Firefox 134.01 - drag over the point that make background tab(s) shift, don't end the drag, now if you drag back, no other tabs are selected for grouping only the dragged tab. if you accept the group only the dragged tab will be in the new group.

Hmm, these seem like new bugs. They might be regressions from bug 1927540. Either way, would you mind filing new bugs on these issues?

Comment 9

[tabmix.onemen]

Opened bug 1933210 and bug 1933213

Firefox 133.0b9 - tab-grouping index is wrong in RTL language, the index for the tab to group with the dragged tab is off by one.

I can not reproduce this in Nightly.
I am guessing it was "fixed" by this patch when you moved

      if (newIndex >= oldIndex) {
        newIndex++;
      }

out of getDragOverIndex

Comment 10

[Dão Gottwald [:dao]]

(In reply to tabmix.onemen from comment #9)

Opened bug 1933210 and bug 1933213

Thanks!

Firefox 133.0b9 - tab-grouping index is wrong in RTL language, the index for the tab to group with the dragged tab is off by one.

I can not reproduce this in Nightly.
I am guessing it was "fixed" by this patch when you moved

      if (newIndex >= oldIndex) {
        newIndex++;
      }

out of getDragOverIndex

Yeah, okay, that's what I thought. Thanks for confirming.