Closed Bug 927226 Opened 6 years ago Closed 6 years ago

Implement animated build-in for Firefox start grids

Categories

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

x86
Windows 8
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: sfoster, Assigned: sfoster)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Prototype for the transition here: http://fiddle.jshell.net/nxB2c/show/
Attached patch build-in richgrid animation (obsolete) — Splinter Review
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.
Attachment #818034 - Flags: review?(rsilveira)
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.
Attachment #818034 - Flags: review?(rsilveira) → review+
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.
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.
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.
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/sfoster@mozilla.com-4faa90c59d5a
Attachment #818034 - Attachment is obsolete: true
Attachment #819125 - Flags: review?(mbrubeck)
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.
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.
Attachment #819125 - Attachment is obsolete: true
Attachment #819125 - Flags: review?(mbrubeck)
Attachment #819237 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/df6f7d68a3d2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.