Closed Bug 625443 Opened 12 years ago Closed 12 years ago
Arranging of expanded stacked groups is broken
When expanding a stacked group nothing happens. This is a regression caused by http://hg.mozilla.org/mozilla-central/rev/9217634f7b9e
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+
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.
* 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)
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. :)
(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
Status: NEW → ASSIGNED
Whiteboard: [hardblocker] → [hardblocker][has patch]
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).
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
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
Status: RESOLVED → REOPENED
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
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.