827130 - Feels like can't pan page if gesture starts as blocked overscroll

Status: VERIFIED FIXED

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

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR
 (1) Pan to rightmost homescreen page
 (2) Start quick pan gesture left (which won't move the current page because we go into overscroll)
 (3) Switch finger direction and pan right

In step (3), it feels like the current page should start moving left, but instead
nothing happens.  This is frustrating.  (Finger has to move past where
the pan gesture started originally.)

bb? -> radar

Comment 1

[Cristian Rodriguez (:crdlc)]

I know the problem, taking a look because it is very ugly in my honest opinion

Comment 2

[Cristian Rodriguez (:crdlc)]

Hi all,
  
    I have a solution [1] but sometimes the previous page is not re-painted when we change the display value from 'none' to 'block'. Any suggestion from Chris or Vivien?

Thanks a lot

[1] https://github.com/crdlc/gaia/commit/f1b81ba2c722678f60c7e7ccadb733594245374b

Comment 3

[Cristian Rodriguez (:crdlc)]

For example, if we change the backgroundColor of the previous page it is always re-painted

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Except forcing a repaint manually I'm not sure what else to do :/

Comment 5

[Cristian Rodriguez (:crdlc)]

By means of getBoundingClientRect() sometimes doesn't work but for example i checked changing the background property and it works fine

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hi, see comment 2: it seems like we're not flushing properly when changing to block display.  Any suggestions on how to debug?

Comment 7

[Cristian Rodriguez (:crdlc)]

Yes, please take a look to this branch:

https://github.com/crdlc/gaia/tree/bug-827130-v2

or to this commit:

https://github.com/crdlc/gaia/commit/72b95811e3fb976397216e7ae98fe958a1e45359

STR:

* Both use cases in the last page always:

1) Swipe from left to right and it works fine, perfectly, the previous page is always painted.

2) Swipe from right to left and immediately left to right and sometimes the page is not painted. Works better if the gesture is slow but when the gesture is fast is more reproducible the bad behavior.

Thanks

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would have thought it unlikely we would have a bug related to flushing/invalidation that's not already reported on Web sites.

Is it possible we're just starving the homescreen's process so it's late in reflowing/painting the previously hidden page?

Comment 9

[Cristian Rodriguez (:crdlc)]

what is your opinion?

Comment 10

[Cristian Rodriguez (:crdlc)]

Current state (sometimes doesn't paint):

http://www.youtube.com/watch?v=59YhQONzkKw

Adding .apps > div { background: yellow } (works)

http://www.youtube.com/watch?v=b4LqygtbGHo

Any clue? As I said, I am not sure if this behavior is due to gecko or not, but could be, same code with or without background different  behaviors

Comment 11

[Cristian Rodriguez (:crdlc)]

Chris, did you see these videos? thanks for your help!

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Are you 100% for sure the homescreen is updating the visibility correctly?

Comment 13

[Cristian Rodriguez (:crdlc)]

Vivien implemented it and I was reviewing with new logs and I am sure that we change the display property correctly. Here [1] we change the display prop for the current and previous pages. Although I am going to take a loog again. Thanks

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/grid.js#L76

Comment 14

[Cristian Rodriguez (:crdlc)]

I tested again and the logs say to me that we are setting the correct values for display prop

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Do you know if this reproduces on a B2G desktop build?

Comment 16

[Cristian Rodriguez (:crdlc)]

I have tested on B2G desktop build but here it works fine. The previous page is always painted. Thanks a lot

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not sure what the right way to proceed is. We could dump display lists for the homescreen process but it would be hard to match those up with the frames with the missing content. But I can't think of anything else. How easy is it to set MOZ_DUMP_PAINT_LIST=1 on the device?

Comment 18

[Cristian Rodriguez (:crdlc)]

is MOZ_DUMP_PAINT_LIST=1 for me? thanks

Comment 19

[Cristian Rodriguez (:crdlc)]

any update here?

Comment 20

[Timothy Nikkel (:tnikkel)]

I'll try to take a look at this.

Comment 21

[Cristian Rodriguez (:crdlc)]

The patch to implement this bug is [1]. When do you say: 'I'll try to take a look at this', What do you mean? on gecko? Thanks a lot Timothy!

[1] https://github.com/crdlc/gaia/commit/72b95811e3fb976397216e7ae98fe958a1e45359

Comment 22

[Timothy Nikkel (:tnikkel)]

The file seems to have changed a fair bit since then. Is there an updated diff?

Comment 23

[Timothy Nikkel (:tnikkel)]

Nevermind, I got it applied.

Comment 24

[Cristian Rodriguez (:crdlc)]

If you want I can rebase my branch, do you want? thanks

Comment 25

[Timothy Nikkel (:tnikkel)]

I've got it now, thanks.

Comment 26

[Cristian Rodriguez (:crdlc)]

If somebody need the changes

* the branch:

https://github.com/crdlc/gaia/tree/bug-827130-v3

* commit:

https://github.com/crdlc/gaia/commit/71e95a1ce3bfc4fd4cc79b700a3891ba6d80fac4

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Bug 842218 seems like the symptom described here.  It would be very interesting to know why the change here exacerbates the problem.

Comment 28

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v1

Comment 29

[Cristian Rodriguez (:crdlc)]

This new style [1] does that all pages are always re-painted in panning

[1] https://github.com/mozilla-b2g/gaia/pull/8230/files#L1R16

Comment 30

[Cristian Rodriguez (:crdlc)]

This patch [1] solves this bug but we need to wait for landing this bug 842218 where the re-painting issue will be fixed.

Thanks

[1] https://github.com/crdlc/gaia/commit/897d55b0d65a46736ff4348e1403dcd3914d3ffb

Comment 31

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v1

Comment 32

[Cristian Rodriguez (:crdlc)]

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

Comment 33

[Cristian Rodriguez (:crdlc)]

Comment on attachment 717898 [details]
Patch v1

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: bad user experience when users try to go to previous page from last or go to next one from the first (evme)
Testing completed: yes
Risk to taking this patch (and alternatives if risky): close to null
String or UUID changes made by this patch:

Comment 34

[John Ford [:jhford] CET/CEST Berlin Time]

Commit c62a8f8 does not apply to v1-train.  This means that there are merge
conflicts which need to be resolved.  If there are dependencies that are not
approved for branch landing, or have yet to land on master, please let me know

If a manual merge is required, a good place to start might be:
  cd gaia
  git checkout v1-train
  git cherry-pick -x <function determine_cherry_pick_master_number at 0x103a115f0> c62a8f8
  <RESOLVE MERGE CONFLICTS>

Comment 35

[Cristian Rodriguez (:crdlc)]

done

v1-train

https://github.com/mozilla-b2g/gaia/commit/8848a0c8c1e19ac3cb414d9745b5f751886a433d

Comment 36

[Michael Vines [:m1] [:evilmachines]]

Requesting uplift to v1.0.1.  This is one of the patches in the MWC build so plenty of eyes on it.

Comment 37

[John Ford [:jhford] CET/CEST Berlin Time]

Unfortunately, this has merge conflicts on v1.0.1 that I'm not 100% sure how to resolve.

cd gaia
git checkout v1.0.1
git cherry-pick -x 8848a0c8c1e19ac3cb414d9745b5f751886a433d

would be a good starting point to merging this.

Comment 38

[Cristian Rodriguez (:crdlc)]

It depends on bug 842218, so if it isn't on v1.0.1, to uplift this one doesn't make sense

Comment 39

[Marcelino Veiga Tuimil [:sonmarce]]

(In reply to John Ford [:jhford] from comment #37)
> Unfortunately, this has merge conflicts on v1.0.1 that I'm not 100% sure how
> to resolve.
> 
> cd gaia
> git checkout v1.0.1
> git cherry-pick -x 8848a0c8c1e19ac3cb414d9745b5f751886a433d
> 
> would be a good starting point to merging this.

I see bug 842218 is nominated as tef?, and as Cristian says it is a dependency, It should be tef+ in order this bug to be merged in v1.0.1

Comment 40

[mdaniloff]

Verified Fixed on Unagi:
Unagi Build ID: 20130322070203
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/4931ec89ebbe
Gaia: 85fd164691bb852f1cfaf82405df4380629ced6e

Comment 41

[Daniel Coloma:dcoloma]

bug 842218 is landed in v1.0.1 so it seems we could land this now in v1.0.1 too? Thanks!

Comment 42

[Cristian Rodriguez (:crdlc)]

Yes, it could be land in v1.0.1 although there will be conflicts, one month and a half ago it was implemented :)

Comment 43

[Daniel Coloma:dcoloma]

Cristian, can you please try to do so?

Comment 44

[Cristian Rodriguez (:crdlc)]

on v.1.0.1

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