295996 - Support tabbox's first-tab/last-tab attributes in tabbrowser

Status: NEW

(Toolkit :: UI Widgets, defect, P3)

Description

[Asaf Romano (gone)]

The first-tab and last-tab attributes should be dynamically updated in
tabbrowser.  We're now setting the first-tab and last-tab on the very first-tab
and just ignoring tabs addition & removal.

Comment 1

[Asaf Romano (gone)]

Attachment: v1

Comment 2

[Mike Connor [:mconnor]]

Comment on attachment 184875 [details] [diff] [review]
v1

This is bitrotted now, you're welcome!	You can figure out how this works with
moveTabTo! :)  (Should be fairly straightforward, ping me if you get lost.)

Comment 3

[Adam Guthrie]

*** Bug 314894 has been marked as a duplicate of this bug. ***

Comment 4

[Julien "_FrnchFrgg_" RIVAUD]

Note that current GTK native rendering of tabs heavily resort on the first-tab attribute, so whenever the first tab is drag&dropped elsewhere, things break badly.

Suppose you have the tab order [A][B][C]. When a tab is selected, its left two pixels are drawn by its left neighbour, unless it is the leftmost. A bit twisted, but supposed to work around the inability to set the z-index of the selected tab to paint it on top of others. Everything works fine, until...

You drag'n drop, and get [B][A][C] for instance. Then [B] doesn't get any left border if selected, since it doesn't know it is leftmost, and [A] gets two (the one drawn by B, and its self painted one, since it still thinks it is leftmost).

Comment 5

[Adam Guthrie]

*** Bug 307780 has been marked as a duplicate of this bug. ***

Comment 6

[logan]

Attachment: moveTabTo patch v1

How's this look? I'm just tinkering, but it seems to do the job...

Comment 7

[Julien "_FrnchFrgg_" RIVAUD]

Also note that when the first tab is closed, the second must gain the "first-tab" attribute. Idem for "last-tab".

On a side note, it seems that in latest trunk with brand new tab behaviour, the "first-tab"/"last-tab" are at least correctly (re)set for a tab when it becomes the active tab or loose this state. But until such an event occurs, the attributes are still wrong.
For example, while having two tabs, the first one active, if you move it to the right, it correctly looses "first" and gains "last" (as if an activation was triggered), but the other one (which is now first) still thinks it is last.
In a similar way, if you have three tabs, the first active, and you close the last (without selecting it), then the second doesn't get the right attributes (last-tab, that is) until it is selected.

So even without this patch, there is some support added (but it is still broken).

Comment 8

[logan]

Attachment: patch v2 (branch)

I see the target milestone is 1.9a, but this seems like a pretty minor change. Could it get into 1.8.0 and 1.8.1 branches?

Comment 9

[logan]

Attachment: patch v2 (trunk)

I'm having alot of problems with trunk, so I'm not 100% on this. Aside from the minor appendChild/insertBefore change in moveTabTo, the rest is the same.

Comment 10

[tabmix.onemen]

i think it is better to check if you need to update "first-tab" or "last-tab" befor you set or remove attribute

this is for "moveTabTo":
// Remove "first-tab" and "last-tab" attributes
if (aTab == this.mTabContainer.firstChild || aIndex == 0)
  this.mTabContainer.firstChild.removeAttribute("first-tab");
if (aTab == this.mTabContainer.lastChild || this.mTabContainer.childNodes.length == aIndex)
  this.mTabContainer.lastChild.removeAttribute("last-tab");

this.mCurrentTab.selected = false;
if (this.mTabContainer.childNodes.length == aIndex)
  this.mTabContainer.appendChild(aTab);
else
  this.mTabContainer.insertBefore(aTab, this.mTabContainer.childNodes[aIndex]);

// Update "first-tab" and "last-tab" attributes
if (!this.mTabContainer.firstChild.hasAttribute("first-tab")) 
  this.mTabContainer.firstChild.setAttribute("first-tab", "true");
if (!this.mTabContainer.lastChild.hasAttribute("last-tab")) 
  this.mTabContainer.lastChild.setAttribute("last-tab", "true");

Comment 11

[logan]

Attachment: patch v2.1 (branch180)

Comment 12

[logan]

Attachment: patch v2.1 (branch18)

Comment 13

[logan]

Attachment: patch v2.1 (trunk)

these look any better?

Comment 14

[Dave Townsend [:mossop]]

I can't see which version of the file you're patching, but the patch for bug 308396 appears to play with first-tab and last-tab a little.

Comment 15

[logan]

Attachment: trunk cvs diff

sorry, last one for now

Comment 16

[logan]

(In reply to comment #14)
> I can't see which version of the file you're patching, but the patch for bug
> 308396 appears to play with first-tab and last-tab a little.

Ya, seems to be fixed on trunk. Is there any interest for this on 1.8 branches?

Comment 17

[Dave Townsend [:mossop]]

If it was bug 308396 that fixed this then that will be landing on the branch for 1.8.1. I can't imagine this is something that would make a 1.8.0.x release.

Comment 18

[Wayne Woods]

(In reply to comment #16)
> Ya, seems to be fixed on trunk. Is there any interest for this on 1.8 branches?

It only seems to be fixed on the trunk for situations where the tab is selected. For example (on Mac at least.. haven't checked Windows yet), the first tab has a (bad) css margin property that causes the icon to be in a slightly different place. Open two or more tabs. Select the second and drag and drop it left so it's now the first. Note where the icon is positioned on the backgrounded second tab. Now click on the second tab and watch the icon move to a "normal" tab position. So it's only being updated to the non-first-tab css after selection, which presumably means the attribute hasn't been reset till then.

Maybe once this patch is in, the checks in the |selected| property can be removed?

It'd be *really* nice to get this on the 1.8 branch, as I need it to try to correct another tab weirdness, but I suspect that would take the some convincing of the drivers at this point.

Comment 19

[Mike Connor [:mconnor]]

Comment on attachment 209393 [details] [diff] [review]
trunk cvs diff

Seems WFM (patch is long bitrotted, I suck)