[New Tab Page] don't start transition for dynamically created elements off a timeout

RESOLVED FIXED in Firefox 18

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
The new tab page updater uses setTimeout(0) to start transitions for dynamically created elements:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/updater.js#174

After doing some testing and looking at bug 701626 it seems that this method doesn't work in 95% of all cases - which I somehow didn't notice when implementing it like this.
(Assignee)

Comment 1

5 years ago
Created attachment 661777 [details] [diff] [review]
use getComputedStyle() before setting the transitioned property to compute the element's current style
Attachment #661777 - Flags: review?(jaws)
Comment on attachment 661777 [details] [diff] [review]
use getComputedStyle() before setting the transitioned property to compute the element's current style

>-      // Without the setTimeout() the node would just appear instead of fade in.
>-      setTimeout(function () {
>-        gTransformation.showSite(site, function () batch.pop());
>-      }, 0);
>+      // Compute the element's current style before showing it again.
>+      window.getComputedStyle(site.node).getPropertyValue("opacity");
>+      gTransformation.showSite(site, function () batch.pop());

This comment doesn't really tell what's going on... It should say that style changes are being flushed in order to make the fade-in transition work.
(Assignee)

Comment 3

5 years ago
Created attachment 661799 [details] [diff] [review]
use getComputedStyle() to flush all style changes to make the transition work

(In reply to Dão Gottwald [:dao] from comment #2)
> This comment doesn't really tell what's going on... It should say that style
> changes are being flushed in order to make the fade-in transition work.

Yes, that comment was poorly chosen, sorry.
Attachment #661777 - Attachment is obsolete: true
Attachment #661777 - Flags: review?(jaws)
Attachment #661799 - Flags: review?(jaws)
Comment on attachment 661799 [details] [diff] [review]
use getComputedStyle() to flush all style changes to make the transition work

>+      // Use getComputedStyle() to flush all style changes for the
>+      // dynamically inserted site to make the fade-in transition work.

"Use getComputedStyle() to" is redundant, just looking at the code you're commenting on makes this already clear.

>+      window.getComputedStyle(site.node).getPropertyValue("opacity");

window.getComputedStyle(site.node).opacity;
Attachment #661799 - Flags: review?(jaws) → review+
(Assignee)

Comment 5

5 years ago
Thank you!

https://hg.mozilla.org/integration/fx-team/rev/fff9c3a1028b
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/fff9c3a1028b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
You need to log in before you can comment on or make changes to this bug.