Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add slide-in animation for arrow panels

RESOLVED FIXED in Firefox 16

Status

()

Toolkit
Themes
--
enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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...
Attachment #635449 - Flags: ui-review?(shorlander)
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

5 years ago
Created attachment 635803 [details] [diff] [review]
patch v2

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgottwald@mozilla.com-e0612c307082/try-win32/firefox-16.0a1.en-US.win32.zip
Attachment #635449 - Attachment is obsolete: true
Attachment #635803 - Flags: ui-review?(shorlander)
(Assignee)

Updated

5 years ago
Depends on: 767462
(Assignee)

Updated

5 years ago
Blocks: 684534
(Assignee)

Updated

5 years ago
Attachment #635803 - Flags: review?(enndeakin)
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

5 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

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/70e3a2c8c6b4

assuming ui-r=shorlander based on comment 1
Target Milestone: --- → mozilla16
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+
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

5 years ago
Depends on: 767813
No longer depends on: 767462
(Assignee)

Updated

5 years ago
Depends on: 767861
(Assignee)

Updated

5 years ago
No longer blocks: 684534
Depends on: 684534
(Assignee)

Comment 8

5 years ago
Created attachment 636245 [details] [diff] [review]
patch v3

This just adds the -moz-transform transition, the rest has been moved to separate bugs.
Attachment #635803 - Attachment is obsolete: true
(Assignee)

Comment 9

5 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

5 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.
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

5 years ago
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"?
Attachment #636245 - Attachment is obsolete: true
Attachment #641386 - Flags: review?(enndeakin)
Attachment #641386 - Flags: feedback?(bmcbride)
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 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

5 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

5 years ago
Created attachment 642319 [details] [diff] [review]
patch v5

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 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

5 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)

Updated

5 years ago
Blocks: 775089
(Assignee)

Comment 19

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/8267d94c2d67
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/8267d94c2d67
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

5 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 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

5 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/3253a84c85c8
status-firefox16: --- → fixed

Updated

5 years ago
Depends on: 779302
(Assignee)

Updated

5 years ago
Blocks: 779302
No longer depends on: 779302
You need to log in before you can comment on or make changes to this bug.