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

NEW
Unassigned

Status

()

Toolkit
XUL Widgets
P3
normal
13 years ago
9 years ago

People

(Reporter: mano, Unassigned)

Tracking

unspecified
mozilla1.9alpha1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

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.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.8beta3
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.)
Attachment #184875 - Flags: first-review?(mconnor) → first-review-
Target Milestone: mozilla1.8beta3 → mozilla1.9alpha1

Comment 3

13 years ago
*** Bug 314894 has been marked as a duplicate of this bug. ***
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

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

Comment 6

12 years ago
Created attachment 209130 [details] [diff] [review]
moveTabTo patch v1

How's this look? I'm just tinkering, but it seems to do the job...
Attachment #209130 - Flags: first-review?

Updated

12 years ago
Attachment #209130 - Flags: first-review? → first-review?(mconnor)
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

12 years ago
Created attachment 209320 [details] [diff] [review]
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?
Attachment #209130 - Attachment is obsolete: true
Attachment #209130 - Flags: first-review?(mconnor)

Comment 9

12 years ago
Created attachment 209321 [details] [diff] [review]
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

12 years ago
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

12 years ago
Created attachment 209386 [details] [diff] [review]
patch v2.1 (branch180)
Attachment #209320 - Attachment is obsolete: true

Comment 12

12 years ago
Created attachment 209387 [details] [diff] [review]
patch v2.1 (branch18)

Comment 13

12 years ago
Created attachment 209388 [details] [diff] [review]
patch v2.1 (trunk)

these look any better?
Attachment #209321 - Attachment is obsolete: true
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.
Assignee: bugs.mano → nobody
Status: ASSIGNED → NEW
QA Contact: bugs.mano → xul.widgets

Comment 15

12 years ago
Created attachment 209393 [details] [diff] [review]
trunk cvs diff

sorry, last one for now
Attachment #209388 - Attachment is obsolete: true
Attachment #209393 - Flags: first-review?(mconnor)

Comment 16

12 years ago
(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?

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

12 years ago
(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.

Updated

11 years ago
Attachment #209393 - Flags: first-review?(mconnor) → review?(mconnor)
Comment on attachment 209393 [details] [diff] [review]
trunk cvs diff

Seems WFM (patch is long bitrotted, I suck)
Attachment #209393 - Flags: review?(mconnor)
You need to log in before you can comment on or make changes to this bug.