The default bug view has changed. See this FAQ.

Group's columns value is not reset correctly after tabs are removed.

RESOLVED FIXED in Firefox 10

Status

Firefox Graveyard
Panorama
P4
normal
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: mitcho, Assigned: luyunxie)

Tracking

({polish})

Trunk
Firefox 10
polish

Details

(Whiteboard: [visual][good first bug])

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 504027 [details]
Video

When the second to last tab is removed from a GroupItem, the Group Item continues to be rendered as if there are two tabs and therefore must have two columns.

While mostly visual in nature, it seems like this may be one part of bug 622872 (blocker).
I believe that the original design was that above a certain width, a lone tab would take only half the available width of the group. Otherwise you end up with a huge tab that fills the group entirely. By providing that extra space, we invite more tabs to be dropped in, and we avoid having the group just be an extra frame around the tab. Of course below a certain width, it made sense for the tab to fill the group, other wise it would be too small.

It appears that the code that did that has eroded such that it kind of does it sometimes. I would propose we use this bug to reinstate the original design.

Also note that when you first go into Panorama, if there's a group with a single tab, that tab fills the group at first, but then stops doing so when you start messing with it. That should be fixed as well (so it starts out half-width if need be).
Blocks: 627096
(Reporter)

Updated

6 years ago
Priority: -- → P4
(Reporter)

Updated

6 years ago
No longer blocks: 627096
Target Milestone: --- → Future
Duplicate of this bug: 630171
Version: unspecified → Trunk
Duplicate of this bug: 666213
Duplicate of this bug: 672794
(Assignee)

Comment 5

6 years ago
(In reply to Ian Gilman [:iangilman] from comment #1)
> I believe that the original design was that above a certain width, a lone
> tab would take only half the available width of the group. Otherwise you end
> up with a huge tab that fills the group entirely. By providing that extra
> space, we invite more tabs to be dropped in, and we avoid having the group
> just be an extra frame around the tab. Of course below a certain width, it
> made sense for the tab to fill the group, other wise it would be too small.
>
Not exactly. The original design is to make tabs flexible and fills the group space as much as possible. Whenever you drag a tab into a group, before you drop that tab, tabs inside the targeting group will automatically rearrange to make space for the new comer. I think this "half width tab" is just a kind mistake.
(Assignee)

Comment 6

6 years ago
Created attachment 557397 [details] [diff] [review]
update column number even if there is only one tab in the group

This root cause is that the group is reluctant to update column information when there is only one tab in the group. Thus, when a tab is removed from a two-columns-two-tabs group, the column number remains 2.
Comment on attachment 557397 [details] [diff] [review]
update column number even if there is only one tab in the group

Request a review
Attachment #557397 - Flags: review?(tim.taubert)
Duplicate of this bug: 628158
Comment on attachment 557397 [details] [diff] [review]
update column number even if there is only one tab in the group

Review of attachment 557397 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that looks good to me! Let's add a test for this. r- only for the missing test.
Attachment #557397 - Flags: review?(tim.taubert) → review-
Depends on: 685456
Duplicate of this bug: 689279
(Assignee)

Comment 11

6 years ago
Created attachment 562951 [details] [diff] [review]
patch with testcase

Thanks for advice from Tim.
Attachment #557397 - Attachment is obsolete: true
Attachment #562951 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #562951 - Flags: review? → review?(ttaubert)
Comment on attachment 562951 [details] [diff] [review]
patch with testcase

Review of attachment 562951 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks! Can you please prepare the patch for checkin?

Here's some advice on how to do that: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Attachment #562951 - Flags: review?(ttaubert) → review+
Assignee: nobody → marffin
Status: NEW → ASSIGNED
(Assignee)

Comment 13

6 years ago
Created attachment 563284 [details] [diff] [review]
fresh patch

r=ttaubert
Attachment #562951 - Attachment is obsolete: true
Attachment #563284 - Flags: checkin?(ttaubert)
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Whiteboard: [visual][polish][good first bug] → [visual][good first bug]
Target Milestone: Future → Firefox 10
Thanks for the patch!

https://hg.mozilla.org/integration/fx-team/rev/268ba6fecae0
Keywords: checkin-needed
Whiteboard: [visual][good first bug] → [visual][good first bug][fixed-in-fx-team]
Attachment #563284 - Flags: checkin?(ttaubert) → checkin+
https://hg.mozilla.org/mozilla-central/rev/268ba6fecae0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [visual][good first bug][fixed-in-fx-team] → [visual][good first bug]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.