Closed Bug 625443 Opened 13 years ago Closed 13 years ago

Arranging of expanded stacked groups is broken


(Firefox Graveyard :: Panorama, defect, P1)


(blocking2.0 final+)

Firefox 4.0b10
Tracking Status
blocking2.0 --- final+


(Reporter: ttaubert, Assigned: mitcho)



(Keywords: regression, Whiteboard: [hardblocker])


(1 file, 4 obsolete files)

When expanding a stacked group nothing happens.

This is a regression caused by
blocking2.0: --- → ?
Blocks: 610208
I'm sorry, that's my regression. :( My apologies... but at least this way we'll get tests for the expanded view.
blocking2.0: ? → final+
Whiteboard: [hardblocker]
Two followups from my work on this bug: bug 625654 and bug 625650. I have decided that I will not try to solve these and sneak them into this patch, though.

The test I am writing for this bug will include TODOs for these two new bugs.
Assignee: nobody → mitcho
Blocks: 625654, 625650
Severity: normal → blocker
Priority: -- → P1
Attached patch Patch v1 (obsolete) — Splinter Review
* New expander test which, for the first time, checks expected expander behavior. It's a little involved, but meant to be comprehensive. This includes a couple TODO sections. Followups filed.
* Rearranging of the code which is shared between a regular grid view and an expanded-stack grid view. The actual tab placement code which was accidentally not being hit anymore after the regression is now hit in both code paths, and shared. This is now in _gridArrange for code sanity (a step which was first suggested in some other patch by Sean).
* Two new subscriber hooks: "expanded" and "collapsed", necessary for the obvious reasons.

All tabview mochitests pass locally. Pushing to try now.
Attachment #503747 - Flags: review?(ian)
Passed try!
Comment on attachment 503747 [details] [diff] [review]
Patch v1

Looks good; thanks for doing this!

My only concern is whether stage 3 in the test is even possible for a user to do. At the very least, the comments all say "click" but that's not what you're really doing. In fact, if you clicked out there, it would just make the tray collapse; it wouldn't actually go to any tab. I guess the user could select a tab in group A and then hit the expander in group B and then hit the Panorama key. Though I think actually what should happen when you expand a group is that one of its tabs should become selected. Perhaps this can be addressed in bug 624654?
Attachment #503747 - Flags: review?(ian) → review+
Just for reference, you must have meant bug 625654. :)
Attached patch Patch v1.1 (obsolete) — Splinter Review
(In reply to comment #5)
> Though I think actually what should happen when you expand a group is that
> one of its tabs should become selected.

I see... so, if that's the spec (and that makes sense), what we want to do in Stage 3 instead is to (1) make the original tab the active tab, (2) expand the stacked group, (3) make sure that one of stacked groups' children is the active tab, and (4) if we toggle Panorama via the shortcut at this point, we will go into that active tab (which is part of that expanded stack), and (5) when we return, the stacked tab has already been collapsed. Phew! Will change the test now, with TODOs for 625654.

Also will note this spec in bug 625654.

Pushing to try now.
Attachment #503747 - Attachment is obsolete: true
Passed try!
Attached patch Patch for checkin (obsolete) — Splinter Review
Attachment #504163 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [hardblocker] → [hardblocker][has patch]
Attached patch Patch for checkin (clean) (obsolete) — Splinter Review
Attachment #504206 - Attachment is obsolete: true
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][needs landing]
(In reply to comment #0)
> When expanding a stacked group nothing happens.

For the record, this isn't entirely correct. Stuff happens... it's just not all the right stuff. :)

The dark background that is supposed to contain the tabs from the expanded stack is created as expected, and the titles for each tab are displayed under the tab preview. The problem lies in the fact that the tabs don't move to take their place on the expanded background. Rather, they remain in their little spiral shape as if they were still collapsed (except now they have their titles displayed, too, causing a jumble of text).
Severity: blocker → major
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [hardblocker][has patch][needs landing] → [hardblocker]
TEST-UNEXPECTED-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_expander.js | The expander is below the stack.

backed out
Resolution: FIXED → ---
The last patch included a TODO in the test, as it didn't assume bug 597776 would land before it. Bug 597776 landed together, and thus the unexpected pass.

Changed just that TODO to a regular test statement.
Attachment #504207 - Attachment is obsolete: true
Blocks: 597776
Closed: 13 years ago13 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Blocks: 626465
verified with nightly build of minefield.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.