972666 - [Calendar] lines over events longer than 2 hours

Status: VERIFIED FIXED

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

Description

[Miller Medeiros [:millermedeiros]]

Attachment: screenshot

steps to reproduce:
1) create event that lasts longer than 2 hours
2) go to day or week view
3) the lines are rendered above the event bucket (they should go behind the event instead).

It's important to notice that clicking on any hour after 2nd will not display the event details (it creates a new event instead).

This error started to happen after https://github.com/mozilla-b2g/gaia/commit/44174c8032b77d304f7abf933b4d5d527baa98af was merged - removed z-index because of APZ issue (Bug 942854)

Another important aspect is that setting a z-index greater than 0 affects the scroll performance drastically - reduces FPS ~50% on hamachi if APZ is enabled.

Comment 1

[Adam Rogers (:arog)]

Hit targets being off can create a problem in calendar; creates a mismatch between user action and result resulting in bad data being entered.

Should determine the performance impact of backing out the patch in comment 0.

Comment 2

[Jason Smith [:jsmith]]

(In reply to Adam Rogers (:arog) from comment #1)
> Hit targets being off can create a problem in calendar; creates a mismatch
> between user action and result resulting in bad data being entered.
> 
> Should determine the performance impact of backing out the patch in comment
> 0.

Backing out alone I don't think will be a good idea here - it introduces the calendar problem seen in bug 942854 with the graphical artifacts instead. We really need to fix the underlying APZC bug here & then backout the patch from that bug.

==> Over to Panning & Zooming for someone from gfx to look into this

Comment 3

[Jason Smith [:jsmith]]

Oh wait - I see the dependency now. That's tracking the APZC fix. If we end up blocking on this, then we'll need to block on the dependency as well.

Comment 4

[Jason Smith [:jsmith]]

Blocking here to backout bug 942854 after bug 972675 lands.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Isn't there some way to fix this without backing out bug 942854? Bug 972675 is only a problem if we do the backout, but if we find some other way to fix the problem then we shouldn't need to do the backout at all. I can look into this a little bit to see if I can find something.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

Here is an (admittedly very ugly) patch that fixes the problem. It does this by restoring the z-index:10 to the events so they float on top of the lines. However, it also takes the entire content of the day-events section and wraps it inside another div with a stacking context. This prevents the z-index:10 from sandwiching the scrollbar (which is at z-index:0) and therefore prevents the scrolling perf regression that you would otherwise get.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> Created attachment 8378459 [details] [diff] [review]
> WIP
> 
> Here is an (admittedly very ugly) patch that fixes the problem. It does this
> by restoring the z-index:10 to the events so they float on top of the lines.
> However, it also takes the entire content of the day-events section and
> wraps it inside another div with a stacking context. This prevents the
> z-index:10 from sandwiching the scrollbar (which is at z-index:0) and
> therefore prevents the scrolling perf regression that you would otherwise
> get.

I really don't think we should be hacking around this at this point. We've already taken two VD regressions from the original hack - we're taking more risk here by trying to hack around this more. Triage wanted the real fix in the dependency with bug 942854 backed out.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Fair enough.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Based on a discussion we had yesterday in our weekly productivity call, the fix in comment 6 sounds correct.

As I understand it, the underlying problem is that the layout mechanism for calendar here is to intentionally cause overflow but use z-index to get the desired painting effect of the calendar item being painted over the bars/etc and thereby also allowing clicks to be processed.  The correct fix is to maintain this desired painting/interaction model while still having it all composited together without causing regressions.

Creating the additional stacking context accomplishes this goal perfectly.  During our discussion I was under the impression that Miller had already tried a fix like this.  Since this works, this seems great/awesome.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Note that bug 965593 might also be getting backed out, in which case the scrollbar will revert to being at a high z-index and no additional stacking context is needed. Stay tuned.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Note that bug 965593 might also be getting backed out, in which case the
> scrollbar will revert to being at a high z-index and no additional stacking
> context is needed. Stay tuned.

We cannot back that out. Wikipedia has to work on shipping devices - it's not up for negotiation.

Comment 12

[Jason Smith [:jsmith]]

Discussed in bug 965593 - we're going with the alternative solution here, as backing out bug 965593 isn't an option on the table.

Dylan - Can you find an assignee here?

Comment 13

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #12)
> Discussed in bug 965593 - we're going with the alternative solution here, as
> backing out bug 965593 isn't an option on the table.
> 
> Dylan - Can you find an assignee here?

More discussion happened offline on bug 965593 - let's hold off here until Milan comes back with a proposal for bug 965593.

Comment 14

[Miller Medeiros [:millermedeiros]]

Attachment: regression.png

the patch on Comment 6 introduces another visual regression (hours and lines aren't properly aligned with events). I'm not sure how much more work would be required to fix it (did not spend time investigating it).

I was really hoping that we would get a platform fix, but if you guys decide to "hack it", I can probably provide a patch in the next couple days. Please let me know what you guys decide. Thanks.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(In reply to Miller Medeiros [:millermedeiros] from comment #14)
> the patch on Comment 6 introduces another visual regression (hours and lines
> aren't properly aligned with events). I'm not sure how much more work would
> be required to fix it (did not spend time investigating it).

Yeah the patch was just a proof of concept. The week view is also still broken with that.

Comment 16

[Jason Smith [:jsmith]]

Let's hold off until Milan comes back on a conclusion on what to do with bug 965593.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Bug 965593 has been backed out. Feel free to make use of z-indices to fix this bug, provided it doesn't cause other regressions.

Comment 18

[Miller Medeiros [:millermedeiros]]

I used the B2G-flash-tool and updated gaia+gecko to latest (master) and I'm still seeing the performance hit if I do `git revert 78dc0c0` (re-enable z-index). Build Id: 20140224040201

not sure what should I do.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

According to Timothy's comments on bug 972675 there is a different root cause for the performance regression than I thought. We'll have to figure that out. In the meantime it might be worth trying to come up with a workaround in Gaia.

Comment 20

[Miller Medeiros [:millermedeiros]]

since this is blocking 1.3+ and Bug 972675 might only be fixed after 1.4 (they wont have time to fix it before, might even get pushed to 1.5); I'm going to come up with a solution on the calendar app itself.

Comment 21

[Miller Medeiros [:millermedeiros]]

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

I reverted the commit that introduced the bug and added a "hack" (a wrapper div with "position:relative" + "z-index:0") to avoid the performance issues caused by z-index + APZ.

Now performance should be similar on week and day views, with and without events. Tried to change as few lines of code as possible without causing undesired side-effects.

Comment 22

[Miller Medeiros [:millermedeiros]]

merged into master: https://github.com/mozilla-b2g/gaia/commit/b1fdd71bf7f3455dbbf015e08175145e02b36d2a

Comment 23

[Mason Chang [Inactive] [:mchang]]

Hi Miller,

This broke scrolling the 'day' when inside the month view. Please back this out. Thanks!

Comment 24

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

Backed out https://github.com/mozilla-b2g/gaia/commit/3e9005ca10de70f6a55cf52c87a8b96937acd481

Comment 25

[Miller Medeiros [:millermedeiros]]

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

sorry guys, I deleted a block of CSS without noticing it affected the month view. Just reverted it and added a marionette test to avoid regressions in the future.

Comment 26

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

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

LGTM. Just a couple nits.

Comment 27

[Miller Medeiros [:millermedeiros]]

merged into master: https://github.com/mozilla-b2g/gaia/commit/d17943e3d848b161f09589b71f4b981c2e14e8c1

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Please request approval-gaia-v1.3 on this patch for uplift.

Comment 29

[Miller Medeiros [:millermedeiros]]

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

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 942854
[User impact] if declined: User won't be able to "tap" the event after the first hour to see event details (hit area is wrong), and will also see lines going over the events.
[Testing completed]: Marionette tests to validate behavior and manual testing on the hamachi and firefox desktop. 
[Risk to taking this patch] (and alternatives if risky): Changes to the view and CSS structure are small, so it should not change the behavior of the app. The main risk that I see is introducing bugs during the merge (there are conflicts with v1.3 branch).
[String changes made]: none

Comment 30

[Miller Medeiros [:millermedeiros]]

Ryan, just requested approval-gaia-v1.3. Please note that patch won't merge "cleanly". Specially because of changes to the marionette tests.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

A rebased PR would be much appreciated in that case :)

Comment 32

[Kevin Grandon :kgrandon]

This has been backed out of master due to an intermittent failing test.

Backout: https://github.com/mozilla-b2g/gaia/commit/52af3e7be31a4ffd95fe61d4de14ac4bc55d0fa5

Travis log:

 1) month view #months-day-view scroll:

Error: timeout exceeded!

at Object.Client.waitForSync (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:679:16)

at Object.Client.waitFor (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:648:60)

at Context.<anonymous> (/home/travis/build/mozilla-b2g/gaia/apps/calendar/test/marionette/month_view_test.js:42:12)

at Test.Runnable.run (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:211:32)

at Runner.runTest (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:372:10)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:448:12

at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:297:14)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:307:7

at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:245:23)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:269:7

at Hook.Runnable.run (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:213:5)

at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:257:10)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:269:7

at done (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:185:5)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:197:9

at Object.executeHook (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:370:18)

at process._tickCallback (node.js:415:13)

Comment 33

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

Perhaps marionette-js-stable-check was doing some good? ;)

Comment 34

[Miller Medeiros [:millermedeiros]]

travis + marionettejs are REALLY unstable.. on the past couple months I haven't seen it catch a single regression and had to disable/report many intermittent failing tests and had to spend multiple days trying to fix marionette bugs. This is very unproductive. I honestly can't see any value on the integration tests if they are not reliable, false alarms makes us spend way more time than it saves.. We should make travis + marionette more reliable or simply stop caring/writing integration tests, this is really annoying.

Any ideas on how to make the tests not fail? The logic seems correct, it looks like the "swipe" is not triggering the scroll (maybe because it is not displayed yet), but that doesn't really makes sense since we call `app.waitForMonthView()` before doing the scroll. The only solution that I can think of is to add a delay, but that is "ugly" and might not work in all cases.

Comment 35

[Kevin Grandon :kgrandon]

I feel your pain Miller, and this might make a good dev-gaia topic. I think we are missing a lot of infrastructure to make it easy to debug these tests - the biggest one being screenshots/logs in travis of what's happening. If we had a screenshot, and maybe an inspectable DOM when the test failed, it would be trivial to debug tests.

I think these tests are really valuable, but we need to spend some time on infrastructure to really make them shine. Actions such as flicking and checking for scroll position are extremely difficult to debug.

Regarding this test specifically, I am not entirely sure, but my guess is that it might be possible to switch to the month view and do the flick before the event actually shows up? I don't see any specific wait for the actual event and iirc these can come in after loading the month view.

Comment 36

[Miller Medeiros [:millermedeiros]]

Attachment: screenshot: firefox desktop left, b2g right

I think the error is related to the fact that the month changed Saturday (March 1st) and the layout isn't "correct" inside b2g-bin (March uses 6 rows to show all days, February used only 5). I'm not sure why we have such huge difference in the layout, even with same screen size.

As you can see the calendar days are covering almost the whole screen area, so the swipe should not work as expected. I really think the styles/visual inside b2g-bin should look the same as on the device and browser in all cases (same fonts, same styles). Any ideas on what could be causing this?

PS: both screenshots were taken while running the app on a mac, but I'm assuming Travis produces a similar result. In any case the days should NEVER cover the whole screen area, so there is definitely something broken (not related to the patch).

Comment 37

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

My guess is that it has something to do with the resizing of the month view squares based on resolution https://github.com/mozilla-b2g/gaia/commit/fed688c0474975d276a5bdc2f8bd880da40e3952#diff-76897c436457793d3bf35535eed52fa0R314

Comment 38

[Miller Medeiros [:millermedeiros]]

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

It was indeed related to the resizing of month view squares based on resolution. B2G doesn't report the correct `device-height` for CSS media queries (it uses the host resolution instead of window height), so the media query was being triggered when it shouldn't. I created a new bug for it: https://bugzilla.mozilla.org/show_bug.cgi?id=979924

I also found out that the way we check if view is active or not is error-prone, so I created a new issue for it and submitted a new PR: https://bugzilla.mozilla.org/show_bug.cgi?id=979969 (otherwise one of tests was also going to fail intermittently)

Comment 39

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

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

r=me and thanks for filing 979969

Comment 40

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

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

Comment 41

[Ryan VanderMeulen [:RyanVM]]

I'll admit, I'm not sure what the uplift story is for v1.3 at this point.

Comment 42

[Miller Medeiros [:millermedeiros]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #41)
> I'll admit, I'm not sure what the uplift story is for v1.3 at this point.

Sorry Ryan! The change I did to fix the problem was minimal and only affected the integration tests. I'm assuming I still have the green light to land this into v1.3 (since changes are basically the same) and I'm currently solving the merge conflicts locally before I push it to mozilla-b2g.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Sounds great, thanks!

Comment 44

[Dylan Oliver [:doliver]]

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

Carrying forward fabrice's a+ from the previous patch (only tests were changed in this one)

Comment 45

[Miller Medeiros [:millermedeiros]]

rebased the changes, fixed merge conflicts and landed into v1.3 branch: https://github.com/mozilla-b2g/gaia/commit/8aed4fafbaeb86d6884d31ce7d3cbeb87bcbf837

Comment 46

[Miller Medeiros [:millermedeiros]]

backed out the changes and landed it again since travis integration tests failed - only difference from previous patch is a single line on package.json to include "chai" as a devDependency: https://github.com/mozilla-b2g/gaia/commit/f4668919aca8efead396e462453ec20840b2219e

Comment 47

[Edward Chen[:edchen]]

Hi Team,

Please check this bug again, I still can reproduce original situation. I configure 6 hours but the line just longer then 2 hours, and the orange bold line longer then 1 hour. Please see the attachment.


[Environment]

Gaia      71f78f7f68fcf5e23cc5965fee0dad45289c438b
Gecko     https://hg.mozilla.org/mozilla-central/rev/5dc091b7e86f
BuildID   20140304160205
Version   30.0a1

[Steps]
1. Launch calendar app
2. Tap + to add new event (5am - 10am)) and then save

[Actual result]
The blank area is not align you selected interval.

[Expect result]
The bland area should be align you selected interval.

[Result]
Failed. I reopen it.

Comment 48

[Edward Chen[:edchen]]

Attachment: Bland area does not align you selected interval

Comment 49

[Miller Medeiros [:millermedeiros]]

Edward, you are testing an OLD version of Gaia (before the patch was applied).

version tested (2014-03-03 22:59:29): https://github.com/mozilla-b2g/gaia/commits/71f78f7f68fcf5e23cc5965fee0dad45289c438b

patch was merged (2014-03-05 17:23:05): https://github.com/mozilla-b2g/gaia/commits/ef64aa12b65723a29b3d5dbe115820a557763fab

Latest on master should fix this problem.

Comment 50

[Natalya Kot [:nkot]]

Verified Fixed.
1. Events span more than 2 hours are displayed properly - no lines over 
2. Tapping the event opens Edit (event details) page.

BuildID: 20140307040203
Gaia: 04eb7996543f114133d1367f834a4d88b68c60ac
Gecko: b0e7f63c2138
Version: 30.0a1