Last Comment Bug 767133 - Add slide-in animation for arrow panels
: Add slide-in animation for arrow panels
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Themes (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: mozilla17
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on: 684534 767813 767861
Blocks: 775089 610545 779302
  Show dependency treegraph
 
Reported: 2012-06-21 14:00 PDT by Dão Gottwald [:dao]
Modified: 2013-11-12 00:56 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch (4.74 KB, patch)
2012-06-21 14:00 PDT, Dão Gottwald [:dao]
shorlander: ui‑review-
Details | Diff | Review
patch v2 (10.54 KB, patch)
2012-06-22 10:43 PDT, Dão Gottwald [:dao]
enndeakin: review+
shorlander: ui‑review+
Details | Diff | Review
patch v3 (2.37 KB, patch)
2012-06-25 00:57 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Review
patch v4 (3.43 KB, patch)
2012-07-12 01:32 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Review
patch v5 (6.73 KB, patch)
2012-07-14 20:22 PDT, Dão Gottwald [:dao]
dtownsend: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Dão Gottwald [:dao] 2012-06-21 14:00:02 PDT
Created attachment 635449 [details] [diff] [review]
patch

Windows and OS X only, since we lack support for translucent windows on Linux.

I guess this can be seen as a subset of bug 610545...
Comment 1 Stephen Horlander [:shorlander] 2012-06-21 19:14:49 PDT
Comment on attachment 635449 [details] [diff] [review]
patch

Review of attachment 635449 [details] [diff] [review]:
-----------------------------------------------------------------

Looks really great!

Just one odd thing I noticed is that on Windows pressing the button that spawned the panel with the panel still open reopens it instead of dismissing it. This happens with the Bookmark Panel and the Downloads Panel but not the Identity Panel.

Seems to work fine on OS X though.
Comment 3 Neil Deakin 2012-06-22 11:49:02 PDT
Comment on attachment 635803 [details] [diff] [review]
patch v2

>+      <handler event="popupshown" phase="target">
>+        this.setAttribute("panelopen", "true");
>+      </handler>

Would it be clearer to set the attribute on panel-arrowcontainer instead and simplify the css rules a bit, though it would require getting that element here? Either way is ok.

 
>+      <handler event="popuphidden" phase="target"><![CDATA[
>+        this.removeAttribute("panelopen");
>+
>+        // Destroying the widget to prevent the current state from bein rendered
>+        // briefly when the panel reopens.

'being'
Comment 4 Dão Gottwald [:dao] 2012-06-22 11:51:20 PDT
(In reply to Neil Deakin from comment #3)
> >+      <handler event="popupshown" phase="target">
> >+        this.setAttribute("panelopen", "true");
> >+      </handler>
> 
> Would it be clearer to set the attribute on panel-arrowcontainer instead and
> simplify the css rules a bit, though it would require getting that element
> here? Either way is ok.

I can use xbl:inherits.
Comment 5 Dão Gottwald [:dao] 2012-06-22 13:16:31 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/70e3a2c8c6b4

assuming ui-r=shorlander based on comment 1
Comment 6 Stephen Horlander [:shorlander] 2012-06-22 13:35:05 PDT
Comment on attachment 635803 [details] [diff] [review]
patch v2

Review of attachment 635803 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Dão Gottwald [:dao] from comment #5)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/70e3a2c8c6b4
> 
> assuming ui-r=shorlander based on comment 1

Yes! Looks great, thank you.
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-06-22 18:56:13 PDT
http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound

Backed out due to mochitest-other orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad52472d98f

https://tbpl.mozilla.org/php/getParsedLog.php?id=12914676&tree=Mozilla-Inbound
13619 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_arrowpanel.xul | Test timed out.
Comment 8 Dão Gottwald [:dao] 2012-06-25 00:57:35 PDT
Created attachment 636245 [details] [diff] [review]
patch v3

This just adds the -moz-transform transition, the rest has been moved to separate bugs.
Comment 9 Dão Gottwald [:dao] 2012-06-25 07:20:57 PDT
Some failures on OS X with this minimal patch:

> browser/base/content/test/browser_bug553455.js | Should have reconstructed
>    the notification UI - Didn't expect [object XULElement], but got it
> browser/base/content/test/browser_popupNotification.js | Test timed out
> dom/indexedDB/test/browser_permissionsPromptDeny.js | Test timed out
> dom/indexedDB/test/browser_privateBrowsing.js | Test timed out
> dom/indexedDB/test/browser_quotaPromptDeny.js | Test timed out

And Windows:

> toolkit/content/tests/chrome/test_arrowpanel.xul | panel label is below
> toolkit/content/tests/chrome/test_arrowpanel.xul | panel label is above
> toolkit/content/tests/chrome/test_arrowpanel.xul | panel label is below
> toolkit/content/tests/chrome/test_arrowpanel.xul | panel label is above
[...]
> browser/base/content/test/browser_bug553455.js | Should have reconstructed
>   the notification UI - Didn't expect [object XULElement], but got it
Comment 10 Dão Gottwald [:dao] 2012-07-04 04:37:02 PDT
For whatever reason, if I reduce the transition distance to 7 pixels, the browser_bug553455.js and test_arrowpanel.xul failures go away.
Enn, any idea what's going on there?

I still get intermittent timeouts in browser_popupNotification.js and browser_quotaPromptDelete.js, specifically in triggerSecondaryCommand. The problem is that the button popup closes when the panel transition finishes. I don't think we need to worry about this in practice, but I'm not sure how to deal with it in the tests.
Comment 11 Neil Deakin 2012-07-04 05:56:08 PDT
Probably the arrow isn't appearing correctly. The test_arrowpanel test mostly tests that the arrows appear on the right side when the panel is flipped due to space constraints, so it is very picky about small changes being made to the sizes of things. Can you identify which parts of the test are failing (by commenting out calls to openPopup/yield?

I'm not familiar with the other test.
Comment 12 Dão Gottwald [:dao] 2012-07-12 01:32:59 PDT
Created attachment 641386 [details] [diff] [review]
patch v4

test_arrowpanel.xul fixed such that it doesn't care about transforms applied to the panel content.

Blair, any idea why these supposedly harmless theme bits cause browser_bug553455.js to fail with "Should have reconstructed the notification UI"?
Comment 13 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-07-12 04:26:36 PDT
Comment on attachment 641386 [details] [diff] [review]
patch v4

Review of attachment 641386 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Dão Gottwald [:dao] from comment #12)
> Blair, any idea why these supposedly harmless theme bits cause
> browser_bug553455.js to fail with "Should have reconstructed the
> notification UI"?

No idea, sorry. But I don't have much experience with arrow panels, or that test - Dave wrote all that.
Comment 14 Dave Townsend [:mossop] 2012-07-13 15:26:20 PDT
Comment on attachment 641386 [details] [diff] [review]
patch v4

This code should be clicking on the panel's anchor twice immediately after the panel opens (in the popupshown event). It looks like with these changes the anchor isn't seeing those clicks any more. I think the arrow panel is over the top of it at that point with this change. Waiting till the transform is done probably won't help much as the panel this test checks is short-lived and goes away when a very short download completes.

Maybe just dispatching the click event directly to the anchor rather than using synthesizeMouse?
Comment 15 Dão Gottwald [:dao] 2012-07-14 07:35:35 PDT
(In reply to Dave Townsend (:Mossop) from comment #14)
> Comment on attachment 641386 [details] [diff] [review]
> patch v4
> 
> This code should be clicking on the panel's anchor twice immediately after
> the panel opens (in the popupshown event). It looks like with these changes
> the anchor isn't seeing those clicks any more. I think the arrow panel is
> over the top of it at that point with this change.

The transform affects the panel content, not the panel itself; the panel remains at the same position throughout the transition and the content is cut off at the top rather than overflowing the panel. So if synthesizeMouse thinks the anchor is blocked, that's a bug :(
Comment 16 Dão Gottwald [:dao] 2012-07-14 20:22:57 PDT
Created attachment 642319 [details] [diff] [review]
patch v5

This was all green on the try server.
Dave, can you review the test fixes / workarounds?
Comment 17 Dave Townsend [:mossop] 2012-07-17 12:56:01 PDT
Comment on attachment 642319 [details] [diff] [review]
patch v5

Review of attachment 642319 [details] [diff] [review]:
-----------------------------------------------------------------

I think that's fine for now. Do you want to file a bug on the bad behaviour though.
Comment 18 Dão Gottwald [:dao] 2012-07-18 07:23:02 PDT
(In reply to Dave Townsend (:Mossop) from comment #17)
> Comment on attachment 642319 [details] [diff] [review]
> patch v5
> 
> Review of attachment 642319 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think that's fine for now. Do you want to file a bug on the bad behaviour
> though.

Filed bug 775089.
Comment 20 Ed Morley [:emorley] 2012-07-19 07:36:12 PDT
https://hg.mozilla.org/mozilla-central/rev/8267d94c2d67
Comment 21 Dão Gottwald [:dao] 2012-07-20 12:47:48 PDT
Comment on attachment 642319 [details] [diff] [review]
patch v5

[Approval Request Comment]
The first part of this already landed for Firefox 16 in bug 767861. This isn't a bad state to be in, but it would be nice to have the full animation in place as originally intended and ui-reviewed in this bug. Medium risk -- some tests didn't like the slide-in transition and had to be updated; however, the problems that these tests had aren't problems that would affect real users.
Comment 22 Alex Keybl [:akeybl] 2012-07-23 11:26:47 PDT
Comment on attachment 642319 [details] [diff] [review]
patch v5

[Triage Comment]
Still very early in the 16 cycle. We can take this on Aurora still at this point. If there's anything in particular QA can do to help us gain confidence here, please add instructions and the qawanted keyword. Thanks!

Note You need to log in before you can comment on or make changes to this bug.