Eitan Isaacson [:eeejay] Assignee
	Description • 11 years ago

This was originally fixed in bug #914882 and, is a regression of bug #920623.

	Eitan Isaacson [:eeejay] Assignee
	Comment 2 • 11 years ago

After some testing, I didn't notice any performance difference.

	Fabien Cazenave [:kaze]
	Comment 3 • 11 years ago

Comment on attachment 8381503 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16625

Vivien, do you remember if we had a particular reason to remove the visibility rules in your patch for bug 810499?

	Fabien Cazenave [:kaze]
	Comment 4 • 11 years ago

Eitan, some unit tests to ensure the non-active panels are either in visibility: hidden or display: none would be helpful. Could you add another commit for that in your PR?

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 5 • 11 years ago

Comment on attachment 8381503 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16625

(In reply to Fabien Cazenave [:kaze] from comment #3)
> Comment on attachment 8381503 [details]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16625
> 
> Vivien, do you remember if we had a particular reason to remove the
> visibility rules in your patch for bug 810499?

I don't remember the exact reasons while I supposed it could have been to avoid a flash.

In all cases this patch is going to be obsoleted by the patch from Mason in bug 976299 who is probably going to do some tricks to ends up with display: none on some panels.

I proposed that you wait a bit to see Mason's progress before going any further.

	Eitan Isaacson [:eeejay] Assignee
	Comment 6 • 11 years ago

Thanks. Standing by..

	Fabien Cazenave [:kaze]
	Comment 7 • 11 years ago

Mason, I see you chose to set the height to zero rather than setting the visibility to hidden in bug 976299. What’s your take on this patch? We still need a visibility: hidden (or display: none) property for accessibility, but we don’t want any flash nor any performance regression.

Do we need both? (= height: 0 and visibility: hidden)
Can we use visibility: hidden without getting any flash?

I think we used to toggle the panel visibility or display with a setTimeout in the JS code to ensure it’s shown/hidden *after* the transition is done. I’d certainly prefer to avoid re-introducing such a trick… but if that’s the only way to preserve accessibility and performances, we’ll cope with that.

I’m on PTO for a week. I think we should wait for the 1.4 branching before landing this patch, but if you want to land this before 2013-03-15 you can ping Arthur for the review. Thanks!

	Mason Chang [Inactive] [:mchang]
	Comment 8 • 11 years ago

The problem with visibility=hidden or visible is that the DOM nodes still stay in memory. They just become hidden / unhidden. The problem with display: none is that it killed all the animations between the panels, while saving the memory. I originally did bug 976299 to save one layer in the layer tree in gecko. Using display: none saves the layer but prevents animations. Using visibility doesn't save the layer. Maybe you can try the visible trick and see if it solves your problem? If so, I can take a look at the layer tree after and make sure we didn't regress there.

What do you mean by any flash?

	Eitan Isaacson [:eeejay] Assignee
	Comment 9 • 10 years ago

Comment on attachment 8381503 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16625

Updated this pull request. I think that toggling visibility *along* with the height hack that mchang introduced may meet both the perf needs, and the accessibility needs. Does this look good?

	Mason Chang [Inactive] [:mchang]
	Comment 11 • 10 years ago

Comment on attachment 8381503 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16625

Layer tree wise, it looks ok. But now we have the visual regression of a flash when animating between panels. Maybe you can eliminate the flash with setting visibility on the previous panel as well?

	Eitan Isaacson [:eeejay] Assignee
	Comment 12 • 10 years ago

(In reply to Mason Chang [:mchang] from comment #11)
> Comment on attachment 8381503 [details]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16625
> 
> Layer tree wise, it looks ok. But now we have the visual regression of a
> flash when animating between panels. Maybe you can eliminate the flash with
> setting visibility on the previous panel as well?

If the panel is not on the screen it would defeat the purpose of the patch.

Perhaps we could have the visibility changed with a 1s delay?

	Eitan Isaacson [:eeejay] Assignee
	Comment 13 • 10 years ago

Comment on attachment 8381503 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16625

Ok. I played with this more. It looks like if the visibility transition is removed altogether when we go from hidden->visible the flashing is avoided.

Not sure why this works, since a hidden->visible transition should always become visible immediately. So removing the transition should not be necessary. Maybe the height changes throw a wrench in the optimized path.

	Mason Chang [Inactive] [:mchang]
	Comment 14 • 10 years ago

Comment on attachment 8381503 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16625

Just tried, looks good. Animations seem smoother now too. Thanks!

	Eitan Isaacson [:eeejay] Assignee
	Comment 15 • 10 years ago

Comment on attachment 8381503 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16625

Thanks!

	Fabien Cazenave [:kaze]
	Comment 16 • 10 years ago

Comment on attachment 8381503 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16625

Thank you all for your feedback.

	Eitan Isaacson [:eeejay] Assignee
	Comment 17 • 10 years ago

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

Updated an integration test too.