614154 - Don't change selected attribute when moving tab

Status: UNCONFIRMED

(Firefox :: Tabbed Browser, defect)

Description

[ithinc]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; zh-CN; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; zh-CN; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12 (.NET CLR 3.5.30729)

In gBrowser.moveTabTo, it set _selected to false for each tab. It sets/unsets first-tab/last-tab/beforeselected/afterselected attributes. This is no problem. But there is no need to set/unset "selected" attribute. Moving a tab won't change the selected state.
Please make _selected setter changes first-tab/last-tab/beforeselected/afterselected attributes only, and add another selected setter if necessary which will change "selected" attribute and call _selected setter.

Reproducible: Always

Comment 1

[Kevin Brosnan]

Sorry, but this behavior is intentional.

Comment 2

[ithinc]

And set "selected" attribute to "true" or "false" in selected setter, but don't remove it. Thus a tab could be easily identified whether it has been visited.

Comment 3

[ithinc]

(In reply to comment #1)
> Sorry, but this behavior is intentional.

Please make it intentional to differentiate a tab selected, visited or unvisited.

Comment 4

[ithinc]

I have just read bug 296970. It tells an entirely different story.

Comment 5

[ithinc]

As you can read, in gBrowser.mTabContainer.selectedIndex setter, it checks whether a tab is already selected before setting selected to false. gBrowser.moveTabTo's behavior , which sets each tab's selected to false without checking, is unreasonable.

Comment 6

[Dão Gottwald [:dao]]

Why exactly is the current behavior a problem for you?

Comment 7

[ithinc]

moveTabTo does an unnecessary unreasonable action. That's the problem. 
You may say "selected" is not for unread state. That's OK. But moveTabTo should not set each tab without checking. It should behave like gBrowser.mTabContainer.selectedIndex setter.

Comment 8

[Dão Gottwald [:dao]]

So we set the 'selected' attribute when it's already there and try to remove it when it shouldn't be there -- while unnecessary, why is that a problem?

The selectedIndex setter seems to set _selected just like moveTabTo:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tabbox.xml

384             Array.forEach(this.childNodes, function (aTab) {
385               if (aTab.selected && aTab != tab)
386                 aTab._selected = false;
387             });
388             tab._selected = true;

Comment 9

[ithinc]

The selectedIndex setter checks aTab.selected, as you see, while moveTabTo doesn't. Why need moveTabTo set each tab's selected to false?

Comment 10

[ithinc]

(In reply to comment #8)
> So we set the 'selected' attribute when it's already there and try to remove it
> when it shouldn't be there -- while unnecessary, why is that a problem?

Its being a problem is that it breaks some existing user styles, and there may be some other potential impacts. It can be done better, can't it?

Comment 11

[Dão Gottwald [:dao]]

So rather than setting _selected = false in a loop, moveTabTo should set _selected = false on the selected tab, move aTab, and set _selected = true on the selected tab. Is that what you're asking for?

Comment 12

[ithinc]

Yes, and that has been there. I guess the looped _selected = false is to set "beforeselected"/"afterselected"/etc., but I'm not sure.

Comment 13

[ithinc]

I correct myself. The looped _selected = false is probably to set "first-tab"/"last-tab" attributes, as "beforeselected"/"afterselected" have been correctly set with setting _selected on the selected tab.

Comment 14

[ithinc]

By the way, how can I upload a proposed patch? The "instructions on how to submit a patch"(http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree) page seems broken.

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> By the way, how can I upload a proposed patch? The "instructions on how to
> submit a
> patch"(http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree)
> page seems broken.

That page appears to have moved here: https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch

Comment 16

[ithinc]

(In reply to comment #15)
> That page appears to have moved here:
> https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch

Thanks. It seems I need some software to checkout the code, which I'm not familiar with yet. So I give up.

My proposal is to separate the current "_selected" setter into several parts. 
One is still named "_selected" setter, which handles "beforeselected"/"afterselected" attributes only. 
Another is "selected" setter which handles "selected" attribute only and calls "_selected" setter. The selectedIndex setter will call "selected" setter instead of "_selected" setter.
The 3rd is "_first" setter and the 4th "_last", which will be called by addTab/removeTab/moveTabTo. Maybe these setters are not necessary. We can set/remove "first-tab"/"last-tab" attributes directly. These two attributes are not related to selected state.

Besides, after the landing of Panorama, some logic might need improving?

Comment 17

[ithinc]

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tabbox.xml#740

The first-tab/last-tab/beforeselected/afterselected attributes are now entirely broken by Panorama.

Comment 18

[ithinc]

The _selected = false looping should be removed once "first-tab"/"last-tab" attributes are obsolete.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2219