988079 - [Calendar] day view sometimes adds duplicated days to the DOM

Status: RESOLVED WONTFIX

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

Description

[Miller Medeiros [:millermedeiros]]

this was driving me crazy for many days.. marionette was complaining that `.event` was not displayed when trying to click the element. I found out that `client.findElements('.event')` was returning 2x as many elements than it should in almost every single case where that happened. Today I finally understood the problem.

Somehow after creating a new event the calendar app duplicates the current day view (`section[data-date]`).

Steps to reproduce:

 1. open calendar app
 2. click "day" tab
 3. click "+" (add event)
 4. create new event using default date (today)

`document.querySelectorAll('#day-view .event')` - will return 2 elements, was supposed to return just 1. You can also inspect the elements and check that there is 1 element for current day that is `.active` and another one that isn't.

This doesn't happen all the time, which makes it way harder to debug.

Comment 1

[Miller Medeiros [:millermedeiros]]

found a solution for the day view test that bypass this problem, so not blocking on it anymore. - we should still fix this ASAP, this already made us lose a LOT of time.

Comment 2

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17670

did a simple fix for the problem, in fact I had to make more changes to make unit tests happy than it was actually necessary to make the code work as expected.

I also considered to remove the extra "frames" (only render a single "frame" each time the day changed) but it broke way more tests and was not as trivial. - I can dedicate more time if you feel that it would be a better approach tho.

I think the multiple "frames" is a leftover from before Bug 791395 (when interaction changed from panning to swipe)..

the multiple "frames" logic would be useful in case we decide to add transitions between the days (I know UX team want to do that in the future).

let me know what you think about it.

Comment 3

[Gareth Aye [:gaye] (back from PTO)]

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

This sounds good to me for the time being and thanks for fixing this!

Comment 4

[Miller Medeiros [:millermedeiros]]

landed into master: https://github.com/mozilla-b2g/gaia/commit/b888cd74fc8b61046b32b7450042671eceb26d79

Comment 5

[Miller Medeiros [:millermedeiros]]

I'll need to back out this since it introduced Bug 989785, will also need to make some changes to the day_view tests before reverting it (otherwise it will break tests).

Comment 6

[Miller Medeiros [:millermedeiros]]

since the "simple fix" did not work that well, maybe we should use this opportunity to clean up the whole "time_parent" logic... - I would love if the calendar app had way less inheritance.. (composition > inheritance)

Comment 7

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18139

second try! now I actually removed the old logic and am I keeping only a single "child view" (add/remove from DOM as needed), and removed all the unused logic. This also means that app is probably using less memory and have better perf while changing between dates (doing less work), and code is a little bit cleaner/simpler.

also added integration tests to avoid regressions (make sure swipe updates dates on week/month/day views).

PS: still need to execute test multiple times on travis before merging!

Comment 8

[Gareth Aye [:gaye] (back from PTO)]

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

I want to know about the performance implications of this change before we press onward. Would you mind investigating miller? See https://github.com/mozilla-b2g/gaia/pull/18139#issuecomment-40229225

Comment 9

[Miller Medeiros [:millermedeiros]]

we have plans to replace the old day view logic with the new MultiDay View (Bug 1052960), so marking this as wont fix. Specially because this doesn't affect the end-user and we abstracted the logic on our marionette tests.