Zoom animation frame is rotated when we zoom out of a non-top stacked tab

VERIFIED FIXED

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
8 years ago
2 years ago

People

(Reporter: mitcho, Assigned: mitcho)

Tracking

Dependency tree / graph
Bug Flags:
in-litmus +

Details

(Whiteboard: [visual][polish])

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 514032 [details]
Key frame from a screen capture

STR:
1. Stacked tabs A, B, C, ...
2. Zoom into tab A.
3. Switch to tab B or C...
4. Zoom out into Panorama.

Expected: normal zoom animation
Actual: the zoom frame is rotated, and does a little dance to become the top child after the animation.
(Assignee)

Comment 1

8 years ago
Created attachment 514036 [details] [diff] [review]
A version of the bug 633074 patch which also fixes this bug

I had originally hoped that the patch to bug 633074 would fix this, but it does not... however, a more general version of the bug 633074 patch *does*. The issue is, the patch there only applies if the tab we're zooming out of is "hidden", i.e. one of the lower tabs that are hidden in the stack. What we want to do is to make all the other tabs also get the same treatment: make them topChild before we do any animation.
Assignee: nobody → mitcho
Status: NEW → ASSIGNED
Attachment #514036 - Flags: feedback?(tim.taubert)
Comment on attachment 514036 [details] [diff] [review]
A version of the bug 633074 patch which also fixes this bug

Looks good! Let's combine those two patches.
Attachment #514036 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Comment 3

8 years ago
Comment on attachment 514036 [details] [diff] [review]
A version of the bug 633074 patch which also fixes this bug

Thanks Tim.
Attachment #514036 - Flags: review?(ian)
Comment on attachment 514036 [details] [diff] [review]
A version of the bug 633074 patch which also fixes this bug

Right on. 

Is this testable? Should we bother?
Attachment #514036 - Flags: review?(ian) → review+
(Assignee)

Comment 5

8 years ago
The STR here wouldn't really be testable deterministically with mochi... I would suggest adding a litmust test.

Tim, how about the 633074 STR? You think we could test that?
(In reply to comment #5)
> Tim, how about the 633074 STR? You think we could test that?

I think we could test that but this would just roughly check whether the tabItem is hidden or not. And I tried checking zoom states which is really no fun :/
(Assignee)

Comment 7

8 years ago
Comment on attachment 514036 [details] [diff] [review]
A version of the bug 633074 patch which also fixes this bug

Thanks Tim. Let's ask for approval and see what the approvers say, then.

But before that, pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=b73cefda8ed7
(Assignee)

Comment 8

8 years ago
Comment on attachment 514036 [details] [diff] [review]
A version of the bug 633074 patch which also fixes this bug

Passed try, modulo three known intermittents.

Note to approvers: This (trivial, lightweight) patch fixes this bug as well as bug 633074. Both are visual flaws in how the zoom animation works for a tab in a stack. As such, writing deterministic tests for these behaviors could be quite tricky, as we discussed above, so this is submitted without mochitests. Litmust tests may be appropriate. r+ Ian, passed try (link above).
Attachment #514036 - Flags: approval2.0?
Comment on attachment 514036 [details] [diff] [review]
A version of the bug 633074 patch which also fixes this bug

a=beltzner
Attachment #514036 - Flags: approval2.0? → approval2.0+
Created attachment 514261 [details] [diff] [review]
Patch for checkin

Patch for checkin. No test included; requesting litmus.
Attachment #514036 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Flags: in-litmus?
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4e6809c053cd
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Version: unspecified → Trunk
(Assignee)

Updated

8 years ago
Blocks: 634574

Comment 12

8 years ago
Mozilla/5.0 (X11; Linux i686; rv:2.0b13pre) Gecko/20110224 Firefox/4.0b13pre

Verified issue and it's no longer present with the latest build.
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus?(vlad.maniac)
Flags: in-litmus?(vlad.maniac) → in-litmus+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.