1820912 - Tab drop indicator can appear at the end of the tabbar when dragging into just before 1st tab

Status: ASSIGNED

(Firefox :: Tabbed Browser, defect)

Description

[Oriol Brufau [:Oriol]]

Attachment: Screen recording

1. Open a window with few tabs, so that they don't overflow
2. Make sure there is some button before the tabbar, e.g. Firefox View is there by default.
3. Load http://example.com/
4. Select some text and drag it into just before 1st tab

Expected the tab drop indicator appears before the 1st tab
Actual the tab drop indicator appears after the last tab

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1808787

Comment 2

[Oriol Brufau [:Oriol]]

The proposal in bug 1791393 comment 4 was

      if (!tab) {
        let firstTab = this.allTabs[0];
        let isBeforeFirstTab = RTL_UI
          ? event.screenX > firstTab.screenX + firstTab.getBoundingClientRect().width
          : event.screenX < firstTab.screenX;
        if (isBeforeFirstTab) {
          return 0;
        }
        return this.allTabs.length;
      }

However, that would still be wrong if the 1st tab is hidden. This works better:

      if (!tab) {
        let firstTab = this._getVisibleTabs()[0];
        let isBeforeFirstTab = RTL_UI
          ? event.screenX >
            firstTab.screenX + firstTab.getBoundingClientRect().width
          : event.screenX < firstTab.screenX;
        if (isBeforeFirstTab) {
          return firstTab._tPos;
        }
        return this.allTabs.length;
      }

but may still be fragile e.g. if there is some separation between two tabs or something. Like a margin set in userChrome.css, or possibly during a tab close animation?

Might be safer to do a binary search as initially planned in bug 1808787.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1808787

Comment 4

[Dianna Smith [:diannaS]]

:oriol is there by any chance any update on this? Or is there any way to set a severity for this so we can decide on what to do with 112?

Comment 5

[Oriol Brufau [:Oriol]]

I guess the options are:

• Recover the old non-performant code that was removed in bug 1808787, but just in the !tab case. It can be optimized in other bugs.
• Use the 2nd code block from comment 2, even if it may be somewhat fragile. And rather than _getVisibleTabs()[0] it should probably be something like this.allTabs.find(tab => !tab.hidden), though I guess the difference may not really matter during drag and drop.
• Use a binary search like in https://phabricator.services.mozilla.com/D166125?vs=on&id=665354

Gijs, what do you prefer?

Comment 6

[:Gijs (he/him)]

I think severity-wise this isn't the end of the world - nothing actually breaks, and you can still reorder tabs how you like.

(In reply to Oriol Brufau [:Oriol] from comment #5)

I guess the options are:

• Recover the old non-performant code that was removed in bug 1808787, but just in the !tab case. It can be optimized in other bugs.
• Use the 2nd code block from comment 2, even if it may be somewhat fragile. And rather than _getVisibleTabs()[0] it should probably be something like this.allTabs.find(tab => !tab.hidden), though I guess the difference may not really matter during drag and drop.
• Use a binary search like in https://phabricator.services.mozilla.com/D166125?vs=on&id=665354

Gijs, what do you prefer?

I think for now let's go with option 2, ie the second code block from comment 2. I don't think we need to cater to the userChrome margin case here.

Comment 7

[Pascal Chevrel:pascalc]

Oriol, are you working on this bug? Thanks