Arranging of expanded stacked groups is broken

VERIFIED FIXED in Firefox 4.0b10

Status

P1
major
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: mitcho)

Tracking

({regression})

Trunk
Firefox 4.0b10
regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Details

(Whiteboard: [hardblocker])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
When expanding a stacked group nothing happens.

This is a regression caused by http://hg.mozilla.org/mozilla-central/rev/9217634f7b9e
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?
(Reporter)

Updated

8 years ago
Blocks: 610208
(Assignee)

Comment 1

8 years ago
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]
(Assignee)

Comment 2

8 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

8 years ago
Assignee: nobody → mitcho
Blocks: 625654, 625650
Severity: normal → blocker
Priority: -- → P1
(Assignee)

Comment 3

8 years ago
Created attachment 503747 [details] [diff] [review]
Patch v1

* 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

8 years ago
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+
(Assignee)

Comment 6

8 years ago
Just for reference, you must have meant bug 625654. :)
(Assignee)

Comment 7

8 years ago
Created attachment 504163 [details] [diff] [review]
Patch v1.1

(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

8 years ago
Passed try!
(Assignee)

Comment 9

8 years ago
Created attachment 504206 [details] [diff] [review]
Patch for checkin
Attachment #504163 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [hardblocker] → [hardblocker][has patch]
Created attachment 504207 [details] [diff] [review]
Patch for checkin (clean)
Attachment #504206 - Attachment is obsolete: true

Updated

8 years ago
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).

Updated

8 years ago
Severity: blocker → major
http://hg.mozilla.org/mozilla-central/rev/005134c9c078
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 504251 [details] [diff] [review]
Patch for checkin, now assumes bug 597776

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

8 years ago
Keywords: checkin-needed
(Assignee)

Updated

8 years ago
Blocks: 597776

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/7c9e4303c3c7
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

8 years ago
Blocks: 626465
(Assignee)

Updated

8 years ago
Duplicate of this bug: 626465
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.