1154649 - [Homescreen][App-grouping]Collapsing animation is not smooth on edit mode of Homescreen

Status: VERIFIED FIXED

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

Description

[Hermes Cheng (inactive after July 27, 2015)]

When I collapse a group at edit mode of Homescreen on Nexus 5, the collapsing animation is not smooth.

This problem does not exist on Flame.

* Test Env:
Build ID               20150414162502
Gaia Revision          16e948bfaaa15dbc0200135d52f16257b4eab193
Gaia Date              2015-04-14 21:08:25
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0eec28e78eb1
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150414.201606
Firmware Date          Tue Apr 14 20:16:21 EDT 2015
Bootloader             HHZ12f

Comment 1

[Hermes Cheng (inactive after July 27, 2015)]

Chris, could you take a look?

Comment 2

[Hermes Cheng (inactive after July 27, 2015)]

Sometimes, it is worse than this video.

Comment 3

[Gregor Wagner [:gwagner]]

Rob, how bad is it from UX standpoint?

Comment 4

[Rob MacDonald [:robmac]]

Thanks Gregor...

I'll refer this one to Hung in visual design, NI'd. Hung worked closely with Maria and Chris on app grouping and is most familiar with the intent of the transitions. 

Rob

Comment 5

[Hung Nguyen]

It looks pretty bad based on the video. The transition should be smooth and not glitches which makes its feel buggy. 

I definitely recommend addressing this issue.

Comment 6

[Gregor Wagner [:gwagner]]

Evelyn, can the devices team help out here? We don't seem to have a nexus 5 in the london office.

Comment 7

[Evelyn Hung]

Sean, please help here. Thanks.

Comment 8

[Hermes Cheng (inactive after July 27, 2015)]

Just discussed with Sean offline, and we found this is not a Nexus 5 only issue but a Homescreen normal one. We can reproduce on flame, too.

It is easy to reproduce when we tried to collapse a 3 lines group and this collapsing would result in upper groups movement (just like video at comment 2).

Comment 9

[Sean Lee [:seanlee][:weilonge]]

Attachment: Homescreen Glitch on Flame

After offline discussion with Hermes, I can reproduce this issue with the 3-line group at bottom in edit mode.
In this clip, the glitch happens at 0:15 and 0:19 .

Comment 10

[GitHub Autolander Bot]

Attachment: [gaia] weilonge:seanlee/verticalhome/master/Bug1154649 > mozilla-b2g:master

Comment 11

[Sean Lee [:seanlee][:weilonge]]

Hi Chris,

I found transition property in this line:
.edit-mode {
  transition: transform 0.25s;
}
[1] https://github.com/weilonge/gaia/blob/master/shared/elements/gaia_grid/style.css#L392

will override the one in app.css:
gaia-grid {
  display: block;
  /* Minus the bottom margin and the search-box height */
  min-height: calc(100% - 5rem - 5rem);
  width: 100%;
  padding-bottom: 5rem;
  transition: height 0s 0.5s; <<< This will be override.
}
[2] https://github.com/weilonge/gaia/blob/master/apps/verticalhome/style/css/app.css#L22

[2] is used in normal mode (not edit mode) as well.
So attachment 8595095 [details] [review] makes the two mode have the same behavior when height of gaia-grid is changed.

Could you give some suggestions for this patch?

Thanks a lot.

Comment 12

[Chris Lord [:cwiiis]]

(In reply to Sean Lee [:seanlee] from comment #11)
> Hi Chris,
> 
> I found transition property in this line:
> .edit-mode {
>   transition: transform 0.25s;
> }
> [1]
> https://github.com/weilonge/gaia/blob/master/shared/elements/gaia_grid/style.
> css#L392
> 
> will override the one in app.css:
> gaia-grid {
>   display: block;
>   /* Minus the bottom margin and the search-box height */
>   min-height: calc(100% - 5rem - 5rem);
>   width: 100%;
>   padding-bottom: 5rem;
>   transition: height 0s 0.5s; <<< This will be override.
> }
> [2]
> https://github.com/weilonge/gaia/blob/master/apps/verticalhome/style/css/app.
> css#L22
> 
> [2] is used in normal mode (not edit mode) as well.
> So attachment 8595095 [details] [review] makes the two mode have the same behavior
> when height of gaia-grid is changed.
> 
> Could you give some suggestions for this patch?
> 
> Thanks a lot.

Nice find! You can just concatenate the rule on the .edit-mode rule so that they both apply, so you'd get "transition: transform 0.25s, height 0s 0.5s;"

Comment 13

[Sean Lee [:seanlee][:weilonge]]

Hey Chris, thanks for your comments. About concatenating the rule, do you mean the way like this? 
https://github.com/weilonge/gaia/commit/e54d1988cd9b72b14d89e55bf0b5aeae43b2dd4d

Correct me if I misunderstand your point. :p Thank you.

Comment 14

[Chris Lord [:cwiiis]]

(In reply to Sean Lee [:seanlee] from comment #13)
> Hey Chris, thanks for your comments. About concatenating the rule, do you
> mean the way like this? 
> https://github.com/weilonge/gaia/commit/
> e54d1988cd9b72b14d89e55bf0b5aeae43b2dd4d
> 
> Correct me if I misunderstand your point. :p Thank you.

Exactly like that, yes - although the rule should be in the verticalhome stylesheet.

Comment 15

[Sean Lee [:seanlee][:weilonge]]

Hi Chris, if we move the rule (transition: height 0s ease 0.25s;) from style.css (gaia_grid) to app.css (verticalhome) [1], gaia_grid node still uses style.css without height transition.

unless we remove the rule from style.css (gaid_grid):
- .edit-mode {
-  transition: transform 0.25s;
- }
But this way might have some regression for any transform property in style.css (gaia_grid).

Could you give some suggestions? Thank you.




[1] sample design:
diff --git a/apps/verticalhome/style/css/app.css b/apps/verticalhome/style/css/app.css
index 6adb873..7ed5eac 100644
--- a/apps/verticalhome/style/css/app.css
+++ b/apps/verticalhome/style/css/app.css
@@ -35,6 +35,7 @@ gaia-grid {

 .edit-mode {
   -moz-user-select: none;
+  transition: height 0s ease 0.25s;
 }

 .edit-mode #curtain {
diff --git a/shared/elements/gaia_grid/style.css b/shared/elements/gaia_grid/style.css
index 8474c7a..51dffd3 100644
--- a/shared/elements/gaia_grid/style.css
+++ b/shared/elements/gaia_grid/style.css
@@ -389,7 +389,7 @@ instead of the context menu.
  * creation hint.
  */
 .edit-mode {
-  transition: transform 0.25s, height 0s ease 0.25s;
+  transition: transform 0.25s;
 }

Comment 16

[Sean Lee [:seanlee][:weilonge]]

Hi Kevin,

I agree with Chris that we should add the rule (transition: height 0s ease 0.25s;) to verticalhome's stylesheet, but there is a rule override issue described at comment 15.

As far as I know that you are the owner of Vertical Home, could you give some suggestions about comment 15?

Thanks a lot.

Comment 17

[Kevin Grandon :kgrandon]

That sounds fine to me, do you want to write a patch? It seems that Chris has already done some investigation here, so he could probably review it for you?

Comment 18

[Sean Lee [:seanlee][:weilonge]]

Sorry that I didn't notice that Chris is the peer of vertical home. I will write a patch and it needs Chris' help to review it. Thanks.

Comment 19

[Chris Lord [:cwiiis]]

Sorry, I meant to get back to you earlier on this and got caught up doing other things.

With regards to your rule being overridden, I assume it's being overridden because the gaia-grid styling has higher precedence - you can check why your rule isn't being applied using the built-in Firefox inspector.

It wouldn't make sense for the rule to be included in gaia-grid, as it's counteracting something set only in verticalhome (and we have other users of gaia-grid). If my assumption is correct, you should be able to make it work by increasing the specificity of the rule: https://developer.mozilla.org/en-US/docs/Web/CSS/Specificity

Comment 20

[Sean Lee [:seanlee][:weilonge]]

Hi Chris, agree with you that the rule should not be included in gaia-grid.
I ever used ID #icons to apply the rule but it doesn't work. This is probably caused by scoped style sheet.

-.edit-mode {
+#icons {
   -moz-user-select: none;
+  transition: height 0s ease 0.25s;
 }

The second way I tried is inline style. It did work but the rule (transition: transform 0.25s;) in gaia-grid is never been applied.

diff --git a/apps/verticalhome/index.html b/apps/verticalhome/index.html
index 858e0f5..1374277 100644
--- a/apps/verticalhome/index.html
+++ b/apps/verticalhome/index.html
@@ -93,7 +93,7 @@
       </form>
     </div>

-    <gaia-grid id="icons" dragdrop></gaia-grid>
+    <gaia-grid id="icons" dragdrop style="transition: height 0s 0.5s;"></gaia-grid>

Comment 21

[Chris Lord [:cwiiis]]

(In reply to Sean Lee [:seanlee] from comment #20)
> Hi Chris, agree with you that the rule should not be included in gaia-grid.
> I ever used ID #icons to apply the rule but it doesn't work. This is
> probably caused by scoped style sheet.

Oh, interesting... Have you tried sticking an !important on the rule? Not ideal, but if it works... And does the rule show up (but crossed out) in the inspector?

Comment 22

[Sean Lee [:seanlee][:weilonge]]

It works if I use !important like this:

diff --git a/apps/verticalhome/style/css/app.css b/apps/verticalhome/style/css/app.css
index 6adb873..9e41950 100644
--- a/apps/verticalhome/style/css/app.css
+++ b/apps/verticalhome/style/css/app.css
@@ -35,6 +35,7 @@ gaia-grid {

 .edit-mode {
   -moz-user-select: none;
+  transition: transform 0.25s, height 0s ease 0.25s !important;
 }

Comment 23

[Chris Lord [:cwiiis]]

If the rule can't be made more specific, I'm ok with that - though it should be

transition: transform 0.25s, height 0s 0.5s !important;

right?

Comment 24

[Sean Lee [:seanlee][:weilonge]]

Yes, it should be "transition: transform 0.25s, height 0s 0.5s !important;" which is based on the rule:
gaia-grid {
  ...
  transition: height 0s 0.5s;
}

I will prepare a patch for you to review. Thanks a lot.

Comment 25

[Chris Lord [:cwiiis]]

Comment on attachment 8595095 [details] [review]
[gaia] weilonge:seanlee/verticalhome/master/Bug1154649 > mozilla-b2g:master

Nice catch and nice work, thanks for dealing with this :)

Comment 26

[Chris Lord [:cwiiis]]

Looks like CSSLint doesn't like the use of !important... Kevin, is it possible to add an exception here? Or can you advise on a fix?

Comment 27

[Kevin Grandon :kgrandon]

(In reply to Chris Lord [:cwiiis] from comment #26)
> Looks like CSSLint doesn't like the use of !important... Kevin, is it
> possible to add an exception here? Or can you advise on a fix?

Which rule are we trying to override? Can we add more CSS specificity? It seems our option might either be a longer selector, or an exception in csslint/xfail.list. I would prefer a longer selector.

Comment 28

[Sean Lee [:seanlee][:weilonge]]

Hi Kevin,

At comment 20, I've tried ID #icons for the css rule in verticalhome's style sheet, but it is still overrided by gaia-grid's one.

Comment 29

[Chris Lord [:cwiiis]]

(In reply to Sean Lee [:seanlee] from comment #28)
> Hi Kevin,
> 
> At comment 20, I've tried ID #icons for the css rule in verticalhome's style
> sheet, but it is still overrided by gaia-grid's one.

Did you try making the rule longer? Something like 'body > gaia-grid#icons.edit-mode'? (I think that's the most specific rule you can make given the current structure, assuming it works)

Comment 30

[Sean Lee [:seanlee][:weilonge]]

Attachment: WebIDE screenshot

This way is still not working:
-.edit-mode {
+body > gaia-grid#icons.edit-mode {
   -moz-user-select: none;
-  transition: transform 0.25s, height 0s 0.5s !important;
+  transition: transform 0.25s, height 0s 0.5s;
 }

gaia_grid's stylesheet is in a scoped style:
    @import url(/shared/elements/gaia_grid/style.css);

would this be the root cause?

Comment 31

[Chris Lord [:cwiiis]]

(In reply to Sean Lee [:seanlee] from comment #30)
> Created attachment 8597885 [details]
> WebIDE screenshot
> 
> This way is still not working:
> -.edit-mode {
> +body > gaia-grid#icons.edit-mode {
>    -moz-user-select: none;
> -  transition: transform 0.25s, height 0s 0.5s !important;
> +  transition: transform 0.25s, height 0s 0.5s;
>  }

Could you try one more with just 'body > #icons.edit-mode'? I can't remember if our web-components implementation actually lets you select on the element name like that or not...

> gaia_grid's stylesheet is in a scoped style:
>     @import url(/shared/elements/gaia_grid/style.css);
> 
> would this be the root cause?

Could very well be, if the above doesn't work I think we should just lump it and add an exception as Kevin suggestions. I think we should also check the spec about scoped style rules and if this isn't explicitly specified behaviour, we should file a platform bug too.

Comment 32

[Sean Lee [:seanlee][:weilonge]]

This selector 'body > #icons.edit-mode' doesn't work as well.
I will try to find whether there is any documentation about scoped css.

I found one way to get higher priority than scoped css is inline style.
So here is an alternative solution for this issue. I think this way can avoid to configure csslint/xfail.list.
diff --git a/apps/verticalhome/js/app.js b/apps/verticalhome/js/app.js
index 1b829d4..1a706f0 100644
--- a/apps/verticalhome/js/app.js
+++ b/apps/verticalhome/js/app.js
@@ -20,6 +20,8 @@
     this.grid.addEventListener('gaiagrid-resize', this);
     this.grid.addEventListener('cached-icons-rendered', this);
     this.grid.addEventListener('edititem', this);
+    this.grid.addEventListener('editmode-start', this);
+    this.grid.addEventListener('editmode-end', this);
     window.addEventListener('hashchange', this);
     window.addEventListener('gaiagrid-saveitems', this);
     window.addEventListener('online', this.retryFailedIcons.bind(this));
@@ -323,6 +325,14 @@
           }
           break;

+        case 'editmode-start':
+          this.grid.style.transition = 'transform 0.25s, height 0s 0.5s';
+          break;
+
+        case 'editmode-end':
+          this.grid.style.transition = '';
+          break;
+
         // A hashchange event means that the home button was pressed.
         // The system app changes the hash of the homescreen iframe when it
         // receives a home button press.

Comment 33

[Sean Lee [:seanlee][:weilonge]]

In addition, the above patch doesn't need any css modification.

Comment 34

[Chris Lord [:cwiiis]]

(In reply to Sean Lee [:seanlee] from comment #32)
> This selector 'body > #icons.edit-mode' doesn't work as well.
> I will try to find whether there is any documentation about scoped css.
> 
> I found one way to get higher priority than scoped css is inline style.
> So here is an alternative solution for this issue. I think this way can
> avoid to configure csslint/xfail.list.
> diff --git a/apps/verticalhome/js/app.js b/apps/verticalhome/js/app.js
> index 1b829d4..1a706f0 100644
> --- a/apps/verticalhome/js/app.js
> +++ b/apps/verticalhome/js/app.js
> @@ -20,6 +20,8 @@
>      this.grid.addEventListener('gaiagrid-resize', this);
>      this.grid.addEventListener('cached-icons-rendered', this);
>      this.grid.addEventListener('edititem', this);
> +    this.grid.addEventListener('editmode-start', this);
> +    this.grid.addEventListener('editmode-end', this);
>      window.addEventListener('hashchange', this);
>      window.addEventListener('gaiagrid-saveitems', this);
>      window.addEventListener('online', this.retryFailedIcons.bind(this));
> @@ -323,6 +325,14 @@
>            }
>            break;
> 
> +        case 'editmode-start':
> +          this.grid.style.transition = 'transform 0.25s, height 0s 0.5s';
> +          break;
> +
> +        case 'editmode-end':
> +          this.grid.style.transition = '';
> +          break;
> +
>          // A hashchange event means that the home button was pressed.
>          // The system app changes the hash of the homescreen iframe when it
>          // receives a home button press.

I'd be happy with this solution, if you added a comment as to why it isn't done in CSS (that it's impossible to get a higher specificity than a scoped style, assuming that's the case).

Comment 35

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8595095 [details] [review]
[gaia] weilonge:seanlee/verticalhome/master/Bug1154649 > mozilla-b2g:master

Hi Chris,

I've updated the PR with some comments and unit-test.
Could you help to review the PR again?
Thank you very much!

Comment 36

[Chris Lord [:cwiiis]]

Comment on attachment 8595095 [details] [review]
[gaia] weilonge:seanlee/verticalhome/master/Bug1154649 > mozilla-b2g:master

r+, with the two comments addressed. Thanks again :)

Comment 37

[Sean Lee [:seanlee][:weilonge]]

Thanks a lot, Chris :)

landed on master: https://github.com/mozilla-b2g/gaia/commit/4d2f58208ed749c3ebf60ef1c1fd852fa63abaab
gaia-try: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=d18096254ea22d9fd0fddac7380f52cd40e929f6

Comment 38

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8595095 [details] [review]
[gaia] weilonge:seanlee/verticalhome/master/Bug1154649 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):

[User impact] if declined:
Collapsing animation has some glitch in edit-mode.

[Testing completed]:
Verified by unit-test.

[Risk to taking this patch] (and alternatives if risky):
Only apply an inline css property. very minor risk. 

[String changes made]:
None.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/9e2105af1cd82ecd4820a80108e99946e96dd539

Comment 40

[Hermes Cheng (inactive after July 27, 2015)]

Verified
**2.2
Build ID               20150503162504
Gaia Revision          8d14361337e608c8cdf165ea5034db5eda23b618
Gaia Date              2015-05-01 18:23:46
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cb7cb6597c91
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150503.195932
Firmware Date          Sun May  3 19:59:50 EDT 2015
Bootloader             HHZ12f


**master
Build ID               20150503160200
Gaia Revision          e18cce173840d6ff07fb6f1f0e0ffb58b99aab3e
Gaia Date              2015-05-02 04:27:01
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/dc5f85980a82
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150503.193743
Firmware Date          Sun May  3 19:38:00 EDT 2015
Bootloader             HHZ12f