Closed
Bug 625443
Opened 14 years ago
Closed 14 years ago
Arranging of expanded stacked groups is broken
Categories
(Firefox Graveyard :: Panorama, defect, P1)
Firefox Graveyard
Panorama
Tracking
(blocking2.0 final+)
VERIFIED
FIXED
Firefox 4.0b10
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: ttaubert, Assigned: mitcho)
References
Details
(Keywords: regression, Whiteboard: [hardblocker])
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
When expanding a stacked group nothing happens.
This is a regression caused by http://hg.mozilla.org/mozilla-central/rev/9217634f7b9e
Reporter | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Assignee | ||
Comment 1•14 years ago
|
||
I'm sorry, that's my regression. :( My apologies... but at least this way we'll get tests for the expanded view.
Updated•14 years ago
|
blocking2.0: ? → final+
Whiteboard: [hardblocker]
Assignee | ||
Comment 2•14 years ago
|
||
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 | ||
Updated•14 years ago
|
Assignee | ||
Comment 3•14 years ago
|
||
* 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)
Assignee | ||
Comment 4•14 years ago
|
||
Passed try!
Comment 5•14 years ago
|
||
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+
Assignee | ||
Comment 6•14 years ago
|
||
Just for reference, you must have meant bug 625654. :)
Assignee | ||
Comment 7•14 years ago
|
||
(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
Assignee | ||
Comment 8•14 years ago
|
||
Passed try!
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #504163 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #504206 -
Attachment is obsolete: true
Updated•14 years ago
|
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][needs landing]
Comment 11•14 years ago
|
||
(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).
Updated•14 years ago
|
Severity: blocker → major
Comment 12•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [hardblocker][has patch][needs landing] → [hardblocker]
Comment 13•14 years ago
|
||
TEST-UNEXPECTED-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_expander.js | The expander is below the stack.
backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 14•14 years ago
|
||
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
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 15•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•