927226 - Implement animated build-in for Firefox start grids

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect)

Description

[Sam Foster [:sfoster] (he/him)]

The windows start screen has a fancy flow-in kind of animation of the tiles. We can draw attention to the tile grids laid out across the start screen by animating them in.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Prototype for the transition here: http://fiddle.jshell.net/nxB2c/show/

Comment 2

[Sam Foster [:sfoster] (he/him)]

Attachment: build-in richgrid animation

Needs patches from bug 927226. This patch adds a build-in animation for the Start grids and removes a flicker we were seeing when the tile backgrounds were set twice.

Comment 3

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 818034 [details] [diff] [review]
build-in richgrid animation

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

Much nicer with the animation! :D

::: browser/metro/base/content/startui/TopSitesView.js
@@ +167,5 @@
>        let filepath = PageThumbsStorage.getFilePathForURL(aSite.url);
>        if (yield OS.File.exists(filepath)) {
>          aSite.backgroundImage = 'url("'+PageThumbs.getThumbnailURL(aSite.url)+'")';
> +        if ('backgroundImage' in aTileNode &&
> +            aTileNode.backgroundImage != aSite.backgroundImage) {

When aTileNode.backgroundImage == aSite.backgroundImage you probably don't want to execute the else block either.

::: browser/metro/modules/View.jsm
@@ +87,5 @@
>        // get the rgb value that represents this color at given opacity over a white matte
>        let tintColor = ColorUtils.addRgbColors(matteColor, ColorUtils.createDecimalColorWord(r,g,b,alpha));
>        aItem.setAttribute("tintColor", ColorUtils.convertDecimalToRgbColor(tintColor));
> +      if ('color' in aItem) {
> +        aItem.color = background;

Is this code related to this bug?

::: browser/metro/theme/platform.css
@@ +525,5 @@
>    display: inherit;
>    overflow: inherit;
>  }
>  
>  @media (max-width: 330px) {

hmmm, we're using 330px for snapped in our CSS but we've set 600px to get snapped layout in the browser.ui.snapped.maxWidth pref. This can cause some funkiness in 8.1 where you can set the width between 330px and 600px. For this case it's probably safer to select on viewstate.

@@ +697,5 @@
> +  opacity: 1;
> +  transform: translateX(0) scale(1);
> +  transition-duration: 367ms;
> +  transition-delay: 500ms;
> +  transition-timing-function: cubic-bezier(0.1, 0.9, 0.2, 1);

Use @metro_animation_easing@.

@@ +699,5 @@
> +  transition-duration: 367ms;
> +  transition-delay: 500ms;
> +  transition-timing-function: cubic-bezier(0.1, 0.9, 0.2, 1);
> +}
> +.meta-section+.meta-section > richgrid {

I think .meta-section:nth-child(x) > richgrid is more readable.

Comment 4

[Sam Foster [:sfoster] (he/him)]

On fx-team:  https://hg.mozilla.org/integration/fx-team/rev/dc42a14663fe

I split out those parts of the patch that addressed a separate issue and filed bug 927938 for that. I'm carrying the r+ here for the rest here. I got the other nits addressed. The backgroundImage logic is actually correct as-is, if that property doesn't exist it means the node isn't bound by the richgrid-item binding, so we just leave the new value on the customImage attribute where it will get picked up when the binding happens.

Comment 5

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/84544a4d0e4d - it remains to be seen whether you landed on metro-chrome bustage and then caused other metro-chrome bustage, or you landed on metro-chrome bustage and then other metro-chrome bustage landed above you, but so far I'm betting on the former.

Comment 6

[Phil Ringnalda (:philor)]

Backout was green, so caused: https://tbpl.mozilla.org/php/getParsedLog.php?id=29282731&tree=Fx-Team

Comment 7

[Sam Foster [:sfoster] (he/him)]

I don't get this test failure locally, which probably means its another issue related to the screen resolution these tests are run at on try. I'm trying a patch that caps clientHeight in portrait mode. The test that failed is looking for scrollMaxY to be > 0, which can be false if the display is tall enough to accomodate the start screen contents without overflow.

Comment 8

[Sam Foster [:sfoster] (he/him)]

Attachment: build-in richgrid animation + portrait test fix

Same as before but with a provision in the setPortraitViewstate helper to cap content height at 1400px. 

Results will be displayed on TBPL as they come in:
https://tbpl.mozilla.org/?tree=Try&rev=4faa90c59d5a

Once completed, builds and logs will be available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4faa90c59d5a

Comment 9

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 819125 [details] [diff] [review]
build-in richgrid animation + portrait test fix

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

::: browser/metro/theme/platform.css
@@ +699,5 @@
> +
> +.meta-section:nth-child(2) > richgrid {
> +  transition-delay: 600ms;
> +}
> +.meta-section:nth-child(3) > richgrid > richgrid {

Drive by comment: you have an extra "> richgrid" here and below.

Comment 10

[Sam Foster [:sfoster] (he/him)]

Attachment: build-in richgrid animation + portrait test fix

One more try. Fixed CSS selector typo (thanks rsilveira). 
I measured the scrollHeight in that portrait scoll test and sure enough the CSS grid scaling looks like its probably the culprit. 1400 was still too big, I updated the patch to cap vertical height at 900px. The tests pass locally at the height, so as we're faking portrait mode just for the purposes of testing UI and Start page response this /should/ work just fine.

Comment 11

[Sam Foster [:sfoster] (he/him)]

Try results at: https://tbpl.mozilla.org/?tree=Try&rev=bd608ad371d2

Comment 12

[Sam Foster [:sfoster] (he/him)]

Green on try, landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/df6f7d68a3d2

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/df6f7d68a3d2