Closed Bug 689215 Opened 8 years ago Closed 2 years ago

Tab object array is not adjusted to account for tab movement

Categories

(Add-on SDK Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: dbuchner, Unassigned)

References

Details

I created a pretty simple add-on that sets a DOMMouseScroll event on content tabs. When the user holds down the control key and scrolls the mouse wheel, the active tab will switch to the left or right of the current tab depending on scroll direction.

The issue is, as highlighted in this test case https://builder.addons.mozilla.org/addon/1017761/latest/, that when the tabs are moved, the index and events are reporting/firing incorrectly for the tab that was moved.

Please advise.
Another even more reduced test case: https://builder.addons.mozilla.org/addon/1018113/latest/

As you will see, when a tab is moved, the key events fire and activate tabs as if the tabs possessed the same tab index values as before the tabs were reordered.

This is a crushing bug for sooo many potential tab-centric add-ons, please advise.
Upon further investigation, the problem is that the tab array is not being reordered to match the on-screen order of the tabs after a tab move event takes place. Please add both the tab move event and reordering of the Tabs array to the current API.
Assignee: nobody → rFobic
Assignee: rFobic → nobody
Summary: Tab events are not properly reporting the tab index when moved → Tab object array is not adjusted to account for tab movement
Let's focus this bug on the issue of the Tabs array not being updated when tabs are moved and leave the enhancement request for a "move event" (which I think is actually asking for a `move` method) to a separate bug report.

cc:ing a couple folks who might be able to confirm this and suggest a fix.
Whiteboard: [triage:followup]
Daniel: I think I understand the problem being reported, but the test cases don't seem to demonstrate that problem, since neither of them move tabs.  They do change which tab is active, but they don't move any tabs first.

Perhaps you can point me to the original addon for which you experienced this problem?  Or maybe you can create a reduced case that simply logs the tab-index ordered list of tab locations to the console before and after moving a tab?
No, you as the user need to move the tabs to see the issue. I couldn't move them with script given the present API regardless, that event is not currently supported in the tabs module.

After you move them, the behaviour is wonky, as you will see. This is because the objects in that array are not reordered to match the on-screen UI order.

Is this clearer now?
Much further reduced test case: https://builder.addons.mozilla.org/addon/1018386/latest/

As you can see, when you reorder the tabs and then activate another, the logged array of indexes is wrong. It should always read 0 strait through to Tabs.length - 1 without any transposition of indices. Instead you observe jumbled indices that do not follow an ordered progression because it fails to reorder the tab objects accordingly.
Ok, now I understand.  Here's a another reduced testcase that demonstrates the problem:

https://builder.addons.mozilla.org/addon/1003384/latest/

Steps to Reproduce:

1. load https://builder.addons.mozilla.org/addon/1003384/latest/
2. press the Test button
3. open the Error Console

Expected Results: the Error Console contains the message "info: false".

Actual Results: the Error Console contains the message "info: true".

See comments in the testcase for the details of the test, but the summary of the bug is as Daniel says: the list of tabs represented by require("tabs") does not get reordered when tabs are moved, such that tabs appear in the wrong place in the list.
I know why this happens, I don't think we even listen or dispatch tab move events, though I have this taken care of in E10S branch. maybe I can port back a fix.
Assignee: nobody → rFobic
(In reply to Daniel Buchner [:dbuc] from comment #2)
> Upon further investigation, the problem is that the tab array is not being
> reordered to match the on-screen order of the tabs after a tab move event
> takes place. Please add both the tab move event and reordering of the Tabs
> array to the current API.

Yeah I noticed the lack of a tab move handler in the module (per comment 2 above) when reviewing the code. Being that support of tab move seems integral to fixing this issue, can you surface the move event in the API for folks to use? Not having it prevents many tab-centric use-cases. If we do support tab move, I can see that being a new ticket, but I thought I'd raise the issue again to see what people thought about the idea.
Oh, and don't forget to reshuffle the tab object array on tab *close* as well. Tab close will change the order of the tab object array anytime a tab other than the last one is closed ;)
Whiteboard: [triage:followup]
Priority: -- → P1
Duplicate of this bug: 995933
Hey Irakli, are you working on this one? if not could you please unassign yourself.
Flags: needinfo?(rFobic)
Assignee: rFobic → nobody
Flags: needinfo?(rFobic)
Priority: P1 → --
https://bugzilla.mozilla.org/show_bug.cgi?id=1399562
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.