Closed
Bug 767133
Opened 12 years ago
Closed 12 years ago
Add slide-in animation for arrow panels
Categories
(Toolkit :: Themes, enhancement)
Toolkit
Themes
Tracking
()
RESOLVED
FIXED
mozilla17
Tracking | Status | |
---|---|---|
firefox16 | --- | fixed |
People
(Reporter: dao, Assigned: dao)
References
Details
Attachments
(1 file, 4 obsolete files)
6.73 KB,
patch
|
mossop
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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...
Attachment #635449 -
Flags: ui-review?(shorlander)
Comment 1•12 years ago
|
||
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.
Attachment #635449 -
Flags: ui-review?(shorlander) → ui-review-
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #635449 -
Attachment is obsolete: true
Attachment #635803 -
Flags: ui-review?(shorlander)
Assignee | ||
Updated•12 years ago
|
Attachment #635803 -
Flags: review?(enndeakin)
Comment 3•12 years ago
|
||
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'
Attachment #635803 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 4•12 years ago
|
||
(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.
Assignee | ||
Comment 5•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/70e3a2c8c6b4
assuming ui-r=shorlander based on comment 1
Target Milestone: --- → mozilla16
Comment 6•12 years ago
|
||
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.
Attachment #635803 -
Flags: ui-review?(shorlander) → ui-review+
Comment 7•12 years ago
|
||
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.
Target Milestone: mozilla16 → ---
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 8•12 years ago
|
||
This just adds the -moz-transform transition, the rest has been moved to separate bugs.
Attachment #635803 -
Attachment is obsolete: true
Assignee | ||
Comment 9•12 years ago
|
||
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
Assignee | ||
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 12•12 years ago
|
||
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"?
Attachment #636245 -
Attachment is obsolete: true
Attachment #641386 -
Flags: review?(enndeakin)
Attachment #641386 -
Flags: feedback?(bmcbride)
Comment 13•12 years ago
|
||
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.
Attachment #641386 -
Flags: feedback?(bmcbride) → feedback?(dtownsend+bugmail)
Comment 14•12 years ago
|
||
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?
Attachment #641386 -
Flags: feedback?(dtownsend+bugmail)
Assignee | ||
Comment 15•12 years ago
|
||
(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 :(
Assignee | ||
Comment 16•12 years ago
|
||
This was all green on the try server.
Dave, can you review the test fixes / workarounds?
Attachment #641386 -
Attachment is obsolete: true
Attachment #641386 -
Flags: review?(enndeakin)
Attachment #642319 -
Flags: review?(dtownsend+bugmail)
Comment 17•12 years ago
|
||
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.
Attachment #642319 -
Flags: review?(dtownsend+bugmail) → review+
Assignee | ||
Comment 18•12 years ago
|
||
(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.
Assignee | ||
Comment 19•12 years ago
|
||
Target Milestone: --- → mozilla17
Comment 20•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•12 years ago
|
||
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.
Attachment #642319 -
Flags: approval-mozilla-aurora?
Comment 22•12 years ago
|
||
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!
Attachment #642319 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 23•12 years ago
|
||
status-firefox16:
--- → fixed
Assignee | ||
Updated•12 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•