1152591 - Implement a CSS variable to control gaia transitions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Shared, defect)

Description

[Kevin Grandon :kgrandon]

We have a lot of transition times which could be replaced with a CSS variable. I think we should do so in order to be able to make updates to a single place.

This might also allow us to have an easy way to have fast integration tests, we can make tests run transitions at something like 0.1s, or even instantly.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1152591_transition_variable > mozilla-b2g:master

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 8589963 [details] [review]
[gaia] KevinGrandon:bug_1152591_transition_variable > mozilla-b2g:master

Hey guys, have some time to put a review stamp on this file? I would like to use this file to contain a list of gaia-wide CSS variables. This doesn't need to block the parent bug from landing, and can land independently of that patch.

If we don't want to port the parent bug to use this, we can do so in follow-up. Thanks!

Comment 3

[Kevin Grandon :kgrandon]

I suppose I will take this bug and we can file follow-ups if needed to actually utilize the variable.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 8589963 [details] [review]
[gaia] KevinGrandon:bug_1152591_transition_variable > mozilla-b2g:master

The approach here then would be to include this file in all the apps using system-wide shared CSS variables? I do wonder about the performance trade-off in yet another bit of IO system-wide for just this one variable - but a problem not inherent to this patch, as we're doing hundreds of includes per app already. I wonder if CSS files in content are put in Gecko's startup cache...

Comment 5

[Kevin Grandon :kgrandon]

(In reply to Dietrich Ayala (:dietrich) from comment #4)
> The approach here then would be to include this file in all the apps using
> system-wide shared CSS variables?

Correct, or investigate some way of inlining these at buildtime. 

> I do wonder about the performance
> trade-off in yet another bit of IO system-wide for just this one variable -
> but a problem not inherent to this patch, as we're doing hundreds of
> includes per app already. I wonder if CSS files in content are put in
> Gecko's startup cache...

If there is a perf hit, then we should be able to concatenate and build CSS. I'm not too worried about a small piece of CSS like this though.

Thanks for the review. I'll file a new bug to *actually* implement this after your initial patch lands.

Comment 6

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#NSPPUqy0T-m3mZo2HIpyqQ

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 7

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/0e1475941690fa757509dd566c6b4644385037fa

Comment 8

[Kevin Grandon :kgrandon]

Thinking about this more, I wonder if we should use gaia-theme here instead as it's included everywhere by default pretty much.

Comment 9

[Kevin Grandon :kgrandon]

Attachment: Pull request - Implement default transition time in gaia-theme

Wilson - can you review this if you have a chance?

We landed a CSS file for definitions in shared/ but only because I forgot about gaia-theme. I think this makes more sense in gaia-theme, what do you think?

Comment 10

[Wilson Page [:wilsonpage]]

Comment on attachment 8590368 [details] [review]
Pull request - Implement default transition time in gaia-theme

I have one NIT, but nice!

Comment 11

[Kevin Grandon :kgrandon]

In gaia-theme: https://github.com/gaia-components/gaia-theme/commit/c4bc39c49239d1856504f747ecfe745e64b18ded

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1152591_update_gaia_theme_for_transitions > mozilla-b2g:master

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8590386 [details] [review]
[gaia] KevinGrandon:bug_1152591_update_gaia_theme_for_transitions > mozilla-b2g:master

Assuming everything passes, r=me on the simple bower manifest bump.

Comment 14

[Dietrich Ayala (:dietrich)]

Would any changes to the default now need to be updated in both places?

Comment 15

[Kevin Grandon :kgrandon]

(In reply to Dietrich Ayala (:dietrich) from comment #14)
> Would any changes to the default now need to be updated in both places?

I think I'm going to kill the old json file, this is much better suited in gaia-theme.

Comment 16

[Kevin Grandon :kgrandon]

Adding checkin-needed for the gaia-theme manifest bump.

Comment 17

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#aqywiNuIRDWV-H6rzVUeuQ

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 18

[Kevin Grandon :kgrandon]

Failures are due to bug 1148154, those seem to be resolved now so let's try this again.

Comment 19

[Kevin Grandon :kgrandon]

Hmm, appears there is an autolander bug here as we don't update from master before attempting the merge. This could possibly fail once more before landing...

Comment 20

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#Brp-F3E1T6ig3jOJ-8cj5A

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 21

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/e60b0e9c982eb38b6d6b88edc345be314234304a

Comment 22

[Kevin Grandon :kgrandon]

Original approach backed out, so now we only have to update the theme, and it should already be included for all apps.

https://github.com/mozilla-b2g/gaia/commit/e7266036e8e5be2f0054771d0e5391ff4ff2e315