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

VERIFIED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::Homescreen
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Hermes Cheng (inactive after July 27, 2015), Assigned: seanlee)

Tracking

unspecified
2.2 S11 (1may)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [systemsfe], URL)

Attachments

(3 attachments)

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
Chris, could you take a look?
blocking-b2g: --- → 2.2?
Flags: needinfo?(chrislord.net)
Whiteboard: [2.2-nexus-5-l][systemsfe]
Sometimes, it is worse than this video.
Rob, how bad is it from UX standpoint?
Flags: needinfo?(rmacdonald)
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
Flags: needinfo?(rmacdonald) → needinfo?(hnguyen)

Comment 5

3 years ago
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.
Flags: needinfo?(hnguyen)
Evelyn, can the devices team help out here? We don't seem to have a nexus 5 in the london office.
Flags: needinfo?(ehung)
Flags: needinfo?(nhirata.bugzilla)

Updated

3 years ago
blocking-b2g: 2.2? → 2.2+

Comment 7

3 years ago
Sean, please help here. Thanks.
Flags: needinfo?(ehung) → needinfo?(selee)
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).
Whiteboard: [2.2-nexus-5-l][systemsfe] → [systemsfe]
(Assignee)

Comment 9

3 years ago
Created attachment 8593879 [details]
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 .
Flags: needinfo?(selee)
Created attachment 8595095 [details] [review]
[gaia] weilonge:seanlee/verticalhome/master/Bug1154649 > mozilla-b2g:master
(Assignee)

Comment 11

3 years ago
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.

Updated

3 years ago
Assignee: nobody → selee
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
(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;"
Flags: needinfo?(chrislord.net)
(Assignee)

Comment 13

3 years ago
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.
Flags: needinfo?(chrislord.net)
(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.
Flags: needinfo?(chrislord.net)
status-b2g-v2.2: --- → affected
status-b2g-master: --- → unaffected
Flags: needinfo?(nhirata.bugzilla)
status-b2g-master: unaffected → affected
Summary: [Homescreen][App-grouping][Nexus 5] Collapsing animation is not smooth on edit mode of Homescreen → [Homescreen][App-grouping]Collapsing animation is not smooth on edit mode of Homescreen
(Assignee)

Comment 15

3 years ago
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;
 }
Flags: needinfo?(chrislord.net)
(Assignee)

Comment 16

3 years ago
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.
Flags: needinfo?(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?
Flags: needinfo?(kgrandon)
(Assignee)

Comment 18

3 years ago
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.
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
Flags: needinfo?(chrislord.net)
(Assignee)

Comment 20

3 years ago
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>
(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?
(Assignee)

Comment 22

3 years ago
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;
 }
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?
(Assignee)

Comment 24

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8595095 - Flags: review?(chrislord.net)
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 :)
Attachment #8595095 - Flags: review?(chrislord.net) → review+
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?
Flags: needinfo?(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.
Flags: needinfo?(kgrandon)
(Assignee)

Comment 28

3 years ago
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.
(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)
(Assignee)

Comment 30

3 years ago
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;
 }

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

would this be the root cause?
(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.
Status: NEW → ASSIGNED
(Assignee)

Comment 32

3 years ago
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.
(Assignee)

Comment 33

3 years ago
In addition, the above patch doesn't need any css modification.
(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).
(Assignee)

Comment 35

3 years ago
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!
Attachment #8595095 - Flags: review+ → review?(chrislord.net)
Comment on attachment 8595095 [details] [review]
[gaia] weilonge:seanlee/verticalhome/master/Bug1154649 > mozilla-b2g:master

r+, with the two comments addressed. Thanks again :)
Attachment #8595095 - Flags: review?(chrislord.net) → review+
(Assignee)

Comment 37

3 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: affected → fixed
Resolution: --- → FIXED
(Assignee)

Comment 38

3 years ago
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.
Attachment #8595095 - Flags: approval-gaia-v2.2?

Updated

3 years ago
Attachment #8595095 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
v2.2: https://github.com/mozilla-b2g/gaia/commit/9e2105af1cd82ecd4820a80108e99946e96dd539
status-b2g-v2.2: affected → fixed
Target Milestone: --- → 2.2 S11 (1may)
Flags: needinfo?(hcheng)
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
Status: RESOLVED → VERIFIED
status-b2g-v2.2: fixed → verified
status-b2g-master: fixed → verified
Flags: needinfo?(hcheng)
You need to log in before you can comment on or make changes to this bug.