Last Comment Bug 836867 - The Find Toolbar should transition when opening and closing
: The Find Toolbar should transition when opening and closing
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: All All
-- normal with 2 votes (vote)
: mozilla21
Assigned To: Jared Wein [:jaws] (please needinfo? me)
: Mike de Boer [:mikedeboer]
Depends on: 1327839
  Show dependency treegraph
Reported: 2013-01-31 11:55 PST by Jared Wein [:jaws] (please needinfo? me)
Modified: 2017-01-01 02:58 PST (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (4.02 KB, patch)
2013-01-31 11:55 PST, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review-
Details | Diff | Splinter Review
Patch v1.1 (4.79 KB, patch)
2013-01-31 12:46 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch v2 (3.05 KB, patch)
2013-02-06 09:01 PST, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review+
Details | Diff | Splinter Review

Description User image Jared Wein [:jaws] (please needinfo? me) 2013-01-31 11:55:23 PST
Created attachment 708713 [details] [diff] [review]
Comment 1 User image Dão Gottwald [:dao] 2013-01-31 12:02:59 PST
Comment on attachment 708713 [details] [diff] [review]

>+      <property name="hidden">
>+        <getter>
>+          return !this.classList.contains("slideIn") ||
>+                 this.hasAttribute("hidden");
>+        </getter>
>+        <setter><![CDATA[
>+          if (val) {
>+            this.addEventListener("transitionend", function onTransitionEnd() {
>+              this.removeEventListener("transitionend", onTransitionEnd);
>+              this.setAttribute("hidden", "true");
>+            })
>+            this.classList.remove("slideIn");

We'd infinitely wait for the transitionend event here when a theme doesn't add a transition. We could set a stub transition in xul.css to handle that.
Comment 2 User image Jared Wein [:jaws] (please needinfo? me) 2013-01-31 12:46:33 PST
Created attachment 708747 [details] [diff] [review]
Patch v1.1
Comment 3 User image Guillaume C. [:ge3k0s] 2013-02-01 09:37:33 PST
This transition will make the bar appear from the bottom on opening and the other way on closing ?

If so the transition will have to be changed when the find bar will be resdesigned (bug 776708).
Comment 4 User image Alfred Kayser 2013-02-06 05:33:11 PST
Can the .slideIn be changed to .slideOut?

The current patch doesn't allow backwards compatibility for themes, as the
margin-bottom:-1em and opacity:0 cannot be set in FF's before this patch.

By using a slideOut to specify the moving direction one can leave the normal
style rule for findbar as-is:
Comment 5 User image Jared Wein [:jaws] (please needinfo? me) 2013-02-06 09:01:45 PST
Created attachment 710764 [details] [diff] [review]
Patch v2

This approach is simpler and should work without causing issues with 3rd-party themes.
Comment 6 User image Dão Gottwald [:dao] 2013-02-12 07:50:02 PST
Comment on attachment 710764 [details] [diff] [review]
Patch v2

> findbar {
>   border-top: 2px solid;
>   -moz-border-top-colors: ThreeDShadow ThreeDHighlight;
>   padding-bottom: 1px;
>   min-width: 1px;
>+  transition-property: margin-bottom, opacity, visibility;
>+  transition-duration: 150ms, 150ms, 0s;
>+  transition-timing-function: ease-in-out, ease-in-out, linear;
>+  margin-bottom: 0;
>+  opacity: 1;

margin-bottom:0 and opacity:1 are the default states and redundant. r=me with them removed
Comment 7 User image Jared Wein [:jaws] (please needinfo? me) 2013-02-12 09:36:34 PST
Thanks for the review. I removed the default states and pushed to inbound:
Comment 8 User image Ryan VanderMeulen [:RyanVM] 2013-02-12 18:34:56 PST

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