Closed Bug 972666 Opened 6 years ago Closed 6 years ago

[Calendar] lines over events longer than 2 hours

Categories

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

x86
macOS
defect
Not set

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

VERIFIED FIXED
1.4 S2 (28feb)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: mmedeiros, Assigned: mmedeiros)

References

Details

(Keywords: regression, Whiteboard: [p=5])

Attachments

(5 files, 3 obsolete files)

Attached image 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.
Blocks: 968478
See Also: → 942854, 971904
Keywords: regression
Depends on: 972675
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.
blocking-b2g: --- → 1.3?
(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
Component: Gaia::Calendar → Panning and Zooming
Product: Firefox OS → Core
Version: unspecified → 28 Branch
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.
Component: Panning and Zooming → Gaia::Calendar
Product: Core → Firefox OS
Version: 28 Branch → unspecified
Blocking here to backout bug 942854 after bug 972675 lands.
blocking-b2g: 1.3? → 1.3+
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.
Attached patch WIP (obsolete) — Splinter Review
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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?
No longer depends on: 972675
Flags: needinfo?(doliver)
(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.
Flags: needinfo?(doliver)
Attached image 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.
Flags: needinfo?(jsmith)
(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.
Let's hold off until Milan comes back on a conclusion on what to do with bug 965593.
Flags: needinfo?(jsmith)
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.
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.
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
Assignee: nobody → mmedeiros
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.
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.
Attachment #8378459 - Attachment is obsolete: true
Attachment #8381493 - Flags: review?(gaye)
No longer depends on: 972675
Attachment #8381493 - Flags: review?(gaye) → review+
merged into master: https://github.com/mozilla-b2g/gaia/commit/b1fdd71bf7f3455dbbf015e08175145e02b36d2a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [p=5]
Target Milestone: --- → 1.4 S2 (28feb)
Hi Miller,

This broke scrolling the 'day' when inside the month view. Please back this out. Thanks!
Status: RESOLVED → REOPENED
Flags: needinfo?(mmedeiros)
Resolution: FIXED → ---
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.
Attachment #8381493 - Attachment is obsolete: true
Attachment #8383613 - Flags: review?(gaye)
Flags: needinfo?(mmedeiros)
Comment on attachment 8383613 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16737

LGTM. Just a couple nits.
Attachment #8383613 - Flags: review?(gaye) → review+
merged into master: https://github.com/mozilla-b2g/gaia/commit/d17943e3d848b161f09589b71f4b981c2e14e8c1
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Please request approval-gaia-v1.3 on this patch for uplift.
Flags: needinfo?(mmedeiros)
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
Attachment #8383613 - Flags: approval-gaia-v1.3?(fabrice)
Ryan, just requested approval-gaia-v1.3. Please note that patch won't merge "cleanly". Specially because of changes to the marionette tests.
Flags: needinfo?(mmedeiros) → needinfo?(ryanvm)
A rebased PR would be much appreciated in that case :)
Flags: needinfo?(ryanvm)
Attachment #8383613 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
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)
Status: RESOLVED → REOPENED
Flags: needinfo?(mmedeiros)
Resolution: FIXED → ---
Perhaps marionette-js-stable-check was doing some good? ;)
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.
Flags: needinfo?(mmedeiros)
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.
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).
Flags: needinfo?(gaye)
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
Flags: needinfo?(gaye)
Depends on: 979969
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)
Attachment #8383613 - Attachment is obsolete: true
Attachment #8386239 - Flags: review?(gaye)
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
Attachment #8386239 - Flags: review?(gaye) → review+
https://github.com/mozilla-b2g/gaia/commit/ef64aa12b65723a29b3d5dbe115820a557763fab
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
I'll admit, I'm not sure what the uplift story is for v1.3 at this point.
Flags: needinfo?(mmedeiros)
(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.
Flags: needinfo?(mmedeiros)
Sounds great, thanks!
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)
Attachment #8386239 - Flags: approval-gaia-v1.3+
rebased the changes, fixed merge conflicts and landed into v1.3 branch: https://github.com/mozilla-b2g/gaia/commit/8aed4fafbaeb86d6884d31ce7d3cbeb87bcbf837
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
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Depends on: 988516
You need to log in before you can comment on or make changes to this bug.