Closed Bug 1210201 Opened 9 years ago Closed 9 years ago

Difficulty creating a Daily and Weekly event in Calendar directly after a previous event.

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.5+, b2g-v2.2 unaffected, b2g-v2.5 verified, b2g-master verified)

VERIFIED FIXED
blocking-b2g 2.5+
Tracking Status
b2g-v2.2 --- unaffected
b2g-v2.5 --- verified
b2g-master --- verified

People

(Reporter: jthomas, Assigned: julienw)

References

()

Details

(Keywords: regression, Whiteboard: [2.5-Daily-Testing][Spark])

Attachments

(5 files, 2 obsolete files)

Description: If the user creates a Daily event in Calendar and then attempts to create another event the hour afterwards it will be very difficult in doing so since the hit detection from the 1st event is very large and interfering with the blank box below it. This also occurs for the weekly calendar but isn't as difficult to create the follow up event. Repro Steps: 1) Update a Aries to 20150930115400 2) Select Calendar 3) Select Day (Or Week) 4) Create an event 5) Attempt to create an event in the box below the previous event. Actual: The first event create will be viewed. Expected: It is expected that selecting the blank box will displayed the "+" symbol that allows the user to create a new a event. Environmental Variables: Device: Aries 2.5 BuildID: 20150930115400 Gaia: 14a64f1ebd353bccc3f1c0399e1a01a03327749e Gecko: 97e537f85183ef31481602ab9e5587a6e7d16b4d Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd Version: 44.0a1 (2.5) Firmware Version: D5803_23.1.A.1.28_NCB.ftf User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0 Repro frequency: 4/6 See attached: Logcat & Video Video: https://youtu.be/v7mbSLIHCNk
This issue DOES occur on Flame 2.5. Result: Difficulty creating a Daily or Weekly event the hour after a previous event. Environmental Variables: Device: Flame 2.5 Kk Fullflash (512mb) BuildID: 20150930030225 Gaia: 1bc0b19527777ffee494962b48db4be857b07d64 Gecko: 891ee0d0ba3ec42b6484cf0205b3c95e21c58f74 Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd Version: 44.0a1 (2.5) Firmware Version: v18D User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0 ------------------- This issue does NOT occur on Flame 2.2. Result: It is much easier to create an event the hour after a previous event in Daily and Weekly calendar events. Environmental Variables: Device: Flame 2.2 Kk Fullflash (319mb) BuildID: 20150930032502 Gaia: 5dd95cfb9f1d6501ce0e34414596ef3dd9c2f583 Gecko: 65ddad73ad6b Gonk: bd9cb3af2a0354577a6903917bc826489050b40d Version: 37.0 (2.2) Firmware Version: v18D User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
Keywords: regression
Whiteboard: [2.5-Daily-Testing][Spark]
[Blocking Requested - why for this release]: Regression that causes calendar to become harder to use. Let's get a window here.
blocking-b2g: --- → 2.5?
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
QA Contact: sleedavid
Mozilla inbound window: Last Working: Environmental Variables: Device: Flame 2.5 BuildID: 20150717092757 Gaia: 8c009877aff6b8b2f4a60756e2d09c0182393721 Gecko: a1e66a69688f Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd Version: 42.0a1 (2.5) Firmware Version: v18D User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0 First Broken: Environmental Variables: Device: Flame 2.5 BuildID: 20150717093501 Gaia: 8c009877aff6b8b2f4a60756e2d09c0182393721 Gecko: 285d33e0631d Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd Version: 42.0a1 (2.5) Firmware Version: v18D User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42. PushLog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a1e66a69688f&tochange=285d33e0631d This issue appears to be caused by changes made in Bug 1181763.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
Kartikaya this issue seems to have been caused by your changes for bug 1181763. Can you please take a look?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado) → needinfo?(bugmail.mozilla)
Unfortunately I'm going on PTO for a couple of weeks starting Tuesday and I won't have time to look into this before then. Probably the soonest I will realistically get to this is next month sometime. You have three options: 1) If this issue is severe, you can have bug 1181763 backed out 2) If this issue doesn't warrant backing that out, but still needs immediate attention, you can try to find somebody who can look into this soon. :julienw might be a good person since he's familiar with the Gaia side of things and has looked at the relevant Gecko code before. 3) If this issue can wait, feel free to assign it to me and I'll look at it when I get a chance (probably not until November).
Flags: needinfo?(bugmail.mozilla)
Teri, do you know who could look into this since Kartikaya can't?
Flags: needinfo?(twen)
NI Dylan to help find someone to look into this. Thanks.
Flags: needinfo?(twen) → needinfo?(doliver)
Strictly from a Calendar perspective, I'd vote for a backout as this is a regression and doesn't fulfill the purpose statement in bug 1181763 comment 10. That said, it looks like it's been in there for a couple of months so (a) I don't know how feasible a backout is, and (b) I hadn't noticed it until this bug. (Though I rarely create events on the phone -- if you do, this bug is easy to hit and it's annoying.) ni? to gaye and julienw to get your thoughts on backout vs. a followup fix. I'm sure this is helping in other areas of Gaia, particularly it is a good improvement for SMS. In any case, I don't think we can leave it this way for 2.5 so blocking on it.
blocking-b2g: 2.5? → 2.5+
Flags: needinfo?(doliver) → needinfo?(gaye)
Flags: needinfo?(felash)
I agree this case is a valid case where we would want to get the touch at the exact place. I really think bug 1181763 is really useful in a lot of situations so I'd really want to keep it. I'll have a look at how the calendar work and try to come up with a possible solution.
I don't have good ideas. Here are my ideas: Gaia fixes: * have an element for each empty cell (I don't know if it needs to be recognized as clickable or not, but for sure we can still keep the event delegation) * make the event element not a child of the day element -- but I think it would be bad for accessibility Gecko fixes: * backout bug 1181763; I think it's possible to do it cleanly. * reduce the fluffing radius to make the issue less noticeable. For example I don't see it's painful when I try to create an event _before_ the existing one, only when I want to create an event _after_ the existing one. The reason is that currently, we use "5mm" for the "top" radius, "2mm" for the "bottom" radius, and "3mm" for the left and right radii (but left/right are not triggering the issue here, because the other days are not ancestors of the existing event element). I believe we use a bigger value for "top" than for "bottom" because the user is more likely to miss the event by tapping below it. Yet more than twice a difference is a lot. I tried with "3mm" and this issue is much less noticeable, and should still fairly correct bad touches. I tried also "4mm" but the issue is still quite noticeable. So that would be my advice here: set "ui.mouse.radius.topmm" to "3mm". Now, how to do it ? Either in gecko's default prefs, or in gaia's settings json file. Also I'm not sure how we change existing user preference values -- likely these ones were never changed by the user so changing b2g.js (and maybe b2gdroid.js, and maybe fennec's mobile.js too) should be enough.
Flags: needinfo?(felash)
Oh, I realize I kept the first line "I don't have good ideas" but I actually think the last one is good ;)
Sounds like a fine approach to me -- I'd be happy to evaluate a build with the 3mm setting.
Dylan, you can already try and change it directly using WebIDE :) (don't forget to restart the app)
Priority: -- → P2
Assignee: nobody → gaye
Flags: needinfo?(gaye)
So I added user_pref("ui.mouse.radius.topmm", "3mm"); to my profile's prefs.js and that seemed to not do very much to help?
Flags: needinfo?(felash)
Maybe because the value should be "3" ?
Flags: needinfo?(felash)
Actually the value should be "3" without the quotes, it's an integer.
That seems to make things slightly better but still mostly terrible. Can we just back out the gecko patch?
Flags: needinfo?(felash)
The gecko patch brings a better behavior in some other situations. I'd prefer to make the calendar work with the gecko patch. I just retested on both flame and aries and with both "ui.touch.radius.topmm" and "ui.mouse.radius.topmm" to "3" (without quotes) I think it works quite fine. Did you try setting "ui.touch.radius.topmm" as well ? I admit I didn't mention it earlier, sorry :/ I think the "touch" prefs are used for touch events while the "mouse" prefs are used for mouse events (although I'm not sure, maybe the "touch" prefs are always used when the initial events is a touch event, even if we're handling a "click" event).
Flags: needinfo?(felash)
I think that makes the day view work okay but the week view is still super off.
Flags: needinfo?(felash)
Hummm I was especially looking at the week view myself. From my own test it's still possible to trigger the previous event but I need to nearly do it on purpose. However I know different people use a phone differently so I'm open to a second opinion. Maybe we could even decrease this to 2 instead of 3, like the other prefs. But only Kats will be able to tell if this has serious implications. Fortunately he's coming back tomorrow :)
I'm fine with changing the radius values for B2G if that helps here, we can change them in the b2g prefs file (b2g/app/b2g.js in gecko).
Kats, don't you think the issue could happen on Fennec as well? Do you know why the value was initially 5mm ? Gaye, I'm testing on Flame and for me the "Week" view is ok with the value of "3", but the "Day" view is a little off (so quite the contrary than you ;) ). I tried with "2" and it feels OK in both view for me. Can you try as well ? As a reminder, this is for both prefs "ui.touch.radius.topmm" and "ui.mouse.radius.topmm".
Flags: needinfo?(felash) → needinfo?(bugmail.mozilla)
It would be worth checking if it happens on Fennec by loading the same HTML there. It might happen, although Fennec has slightly different prefs set for fluffing that might affect this. The value was initially set to 5mm mostly by trial and error, so there's no argument against tweaking it.
Flags: needinfo?(bugmail.mozilla)
Assignee: gaye → nobody
Hey Julien do you have bandwidth to drive this for the week? I am on pto until next week. I grant you temporary calendar overlordship.
Flags: needinfo?(felash)
Sure !
Assignee: nobody → felash
Flags: needinfo?(felash)
I tried with 2mm and gave it to test to nical. As a result I now see it's still easy to miss depending how the user holds the device and where the target is. For example if the target is rather low in the screen, but the user holds the device quite centrally (the thumb will be somewhat in the middle of the height), then it gets more difficult to tap the target. Myself I hold the device with my hand quite low and that's why it's easier for me to not trigger the bug. I'll do a simpler testcase tomorrow and try to come up with some solutions, otherwise I think we'll need to try to backout or disable the patch from bug 1181763.
Attached file testcase-delegation.html (obsolete) —
I haven't checked if this exhibits the issue yet, but I think it's very close to the case in Calendar.
Yes it exhibits the issue :)
Hey Kats, I wonder what you think of the testcase above ? I think there are ways to make it work with the current code, but I also think this is a valid use case and maybe even we broke the web with the change in bug 1181763. So for now I'm very sad to suggest that we should backout the change in bug 1181763. What do you think ?
Flags: needinfo?(bugmail.mozilla)
(backout or disable) BTW do you know if we had any bug in Fennec that could be related ? NI Margaret for the same question as well.
Flags: needinfo?(margaret.leibovic)
Unfortunately I don't have a lot of cycles right now to dig into this (and it's not obvious to me what the STR/expected behaviour is with the testcase you attached) but if you think we should back out bug 1181763 I'm ok with that.
Flags: needinfo?(bugmail.mozilla)
(In reply to Julien Wajsberg [:julienw] from comment #30) > (backout or disable) > > BTW do you know if we had any bug in Fennec that could be related ? NI > Margaret for the same question as well. I haven't seen similar bugs reported in Fennec.
Flags: needinfo?(margaret.leibovic)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31) > Unfortunately I don't have a lot of cycles right now to dig into this (and > it's not obvious to me what the STR/expected behaviour is with the testcase > you attached) but if you think we should back out bug 1181763 I'm ok with > that. The STR is trying to touch the square just below the initial "+". (and then below any lighted square). I'll need to try on Fennec to see how this feels.
Flags: needinfo?(felash)
Better testcase. Try to create cases 1 and 3, and then try to create case 2; this is really difficult with our current fluffing prefs. I have the same issue on Fennec as well. I'm trying to determine whether such issue is common on the Web.
Attachment #8679641 - Attachment is obsolete: true
Flags: needinfo?(felash)
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master Hey Gareth, James, I'd like to know what you'd think about this solution. I still think the new algorithm from bug 1181763 is useful in a lot of situations and that's why I'd like as much as possible to not back it out. That said I'm still in favor of changing the current values of radius as said in previous comments (and both for Fennec and B2G, BTW). We can do that in a separate bug. Yet I tried to workaround this new algorithm with this hacky patch. Should I move forward and write the tests for this ? Maybe you could suggest a better place to do part of it (for example put some part of the code in view_event.js / "primary" method).
Attachment #8681403 - Flags: feedback?(jrburke)
Attachment #8681403 - Flags: feedback?(gaye)
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master I gave some feedback in the pull request. Main issue is the use of .hourHeight, so but I think you found the area of the solution: detect if the tap is really outside the other element and if so, treat it like an md__day tap. I put a comment in the pull request with a possible different way to do it, but also still fresh, appreciate your feedback on it.
Attachment #8681403 - Flags: feedback?(jrburke)
Hey Fabrice, I think you're the best to tell me if this is a good idea. Especially I saw that the fluffing for the "touch" events are not enabled on Android, would you know the reason by chance ?
Attachment #8681454 - Flags: feedback?(fabrice)
It's more a question of why fluffing for touch events is enabled on B2G. See bug 1091889.
Right, thanks Kats ! Do you know who I should ask feedback to when changing these pref values for Fennec ?
You'll probably want to get UX feedback from :antlam for Fennec.
Attachment #8681454 - Flags: feedback?(alam)
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master I got an idea for a much cleaner patch. To come today !
Attachment #8681403 - Flags: feedback?(gaye)
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master So my idea of the cleaner patch was not applicable: I thought that I could access the event from view_event's onactive method, and I wanted to redispatch the event on md__day if I found we didn't tap right on the button. So I reused my previous implementation, I used quite a bit of your suggestion, and here it is. Please tell me what you think :) I tried to add a "touchstart" handler to be able to prevent the style change because of :active. This works... and then we don't get the click anymore :) So maybe the solution could involve using the "touchstart" event instead of the "click" event... Tell me if you'd prefer this. Anyway Bug 1091889 should disable the event fluffing for "touchstart" and then we would not have this issue anymore. Also I didn't write any test simply because I don't have a clue of where to put them. I tried to write a marionette test in test/marionette/week_view_test.js but it's really painful to write as I need to find exactly where to tap... I'd very much prefer to write a unit test but I don't know where to insert this. So pointers are very much welcome ! Thanks :)
Attachment #8681403 - Flags: feedback?(jrburke)
See Also: → 1220770
Comment on attachment 8681454 [details] [diff] [review] 0001-Bug-1210201-Reduce-the-top-constants-for-the-Positio.patch Moving to bug 1220770
Attachment #8681454 - Attachment is obsolete: true
Attachment #8681454 - Flags: feedback?(fabrice)
Attachment #8681454 - Flags: feedback?(alam)
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master Looks good to me! Some very small nits that are up to your discretion in the pull request.
Attachment #8681403 - Flags: feedback?(jrburke) → feedback+
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master hey James, I applied your comments but also completely removed the class 'md__event-like-button' because I realized that we actually want the event retargeting to work fot the `+` button as it's useful. So I changed the logic to check only for the existing event buttons. I spent 2 days to write integration tests but it's really difficult to deal with scrolling. You can see my try in https://github.com/mozilla-b2g/gaia/compare/master...julienw:1210201-create-events-in-calendar-with-tests?expand=1 but it's really pitiful. I also didn't find where to create unit tests for this...
Attachment #8681403 - Flags: review?(jrburke)
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master r+ with the if conditional fix, mentioned in the pull request.
Attachment #8681403 - Flags: review?(jrburke) → review+
Hey James, Actually I made the "mistake" you found completely on purpose as I explained in comment 46: I want the "+" button to enjoy the event retargeting mechanism. The bug here is that we can't easily create events after another event, so the "+" button has nothing to do with this. In my previous patches I incorrectly wanted to prevent the retargeting mechanism to work. Tell me if you'd like that I add parenthesises to make this more clear that it was on purpose. Or I could also add a comment.
Flags: needinfo?(jrburke)
Oh, for me, I thought the general issue was that event fuzzing was not producing the desired result. In one case, the user tries to tap on an hour after an event, but cannot actually do so easily because the event fuzzing on the <a> tag for the event claims the tap. A similar situation happens once the + button shows up. After tapping to show the + button, if I want to then move the + button to the hour below where it exists now, the same event fuzzing problem occurs. It may not be as severe, in that the user ends up in the Add Event screen where they could manually change the time, but I believe it is still disorienting: instead of seeing the expected + button show up where I tapped the screen is changed to an Add Event screen. It is your call on the fix. I prefer to constrain the taps for both buttons for the reason above, but if you think otherwise, a comment should be placed so that others who inspect that code later can know the expression is explicitly designed like that and not an accidental bug.
Flags: needinfo?(jrburke)
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master Hey James, thanks to Johan, I managed to write integration tests \o/ So asking for a new review for this part (in a separate commit).
Attachment #8681403 - Flags: review?(jrburke)
Attachment #8681403 - Flags: review+
Attachment #8681403 - Flags: feedback?(jlorenzo)
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master I am not familiar with the test infrastructure for calendar or the scrolling code used in the test, so that part did not get a thorough review.
Attachment #8681403 - Flags: review?(jrburke) → review+
(In reply to James Burke [:jrburke] from comment #51) > I am not familiar with the test infrastructure for calendar or the scrolling > code used in the test, so that part did not get a thorough review. nobody is ;) Thanks !
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master Nice start! There are some ways to improve the tests, in GitHub. Please tell me what you think.
Attachment #8681403 - Flags: feedback?(jlorenzo)
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master I think I applied your comments, tell me what you think ! Also I ran using all hours as target in a loop and it worked for each of them (except 23 but I expected this).
Attachment #8681403 - Flags: review?(jlorenzo)
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master The test looks good to me, modulo 2 small nits. I won't block on them, though.
Attachment #8681403 - Flags: review?(jlorenzo) → review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master [Approval Request Comment] [Bug caused by] (feature/regressing bug #): 1181763 [User impact] if declined: difficult to create an event just after an existing one [Testing completed]: yes: manual + integration tests [Risk to taking this patch] (and alternatives if risky): low [String changes made]: none
Attachment #8681403 - Flags: approval-gaia-v2.5?
This bug has been verified as pass on latest build of Flame kk v2.6 and Aries kk v2.6 by the STR in comment 0. Actual result: Select the blank box will displayed the "+" symbol that allows the user to create a new a event. See attachment: Verified_Aries_v2.6.3gp Reproduce rate: 0/10 Device: Flame kk v2.6 512mb (master) (Pass) Build ID 20151110150205 Gaia Revision c0482775b1526add626b170dd53a72d10bcaf07c Gaia Date 2015-11-10 02:25:52 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/cc473fe5dc512c450634506f68cbacfb40a06a23 Gecko Version 45.0a1 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20151110.182433 Firmware Date Tue Nov 10 18:24:47 EST 2015 Firmware Version V18D V4 Bootloader L1TC000118D0 Device: Aries kk v2.6(master) (Pass) Build ID 20151110120047 Gaia Revision c0482775b1526add626b170dd53a72d10bcaf07c Gaia Date 2015-11-10 02:25:52 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/cc473fe5dc512c450634506f68cbacfb40a06a23 Gecko Version 45.0a1 Device Name aries Firmware(Release) 4.4.2 Firmware(Incremental) eng.worker.20151110.111927 Firmware Date Tue Nov 10 11:19:35 UTC 2015 Bootloader s1
QA Whiteboard: [QAnalyst-Triage+] → [MGSEI-Triage+]
Comment on attachment 8681403 [details] [review] [gaia] julienw:1210201-create-events-in-calendar > mozilla-b2g:master Tested and verified. Approved for 2.5 uplift. Thanks
Attachment #8681403 - Flags: approval-gaia-v2.5? → approval-gaia-v2.5+
This bug has been verified as "pass" on the latest user build of Flame KK 2.5 and Aires KK 2.5 by the STR in comment 0. Actual results: Tapping on an hour after a previous event in Daily and Weekly calendar events, it shows "+" icon and is much easier to create an event. See attachment: verified_Flame_v2.5.3gp Reproduce rate: 0/10 Device: Flame KK 2.5 512mb user build (Pass) Build ID 20151117132020 Gaia Revision 28d63cf3bdc4417f7ad8cab2230f096bf9f6d3b5 Gaia Date 2015-11-17 07:35:12 Gecko Revision http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/8f193d35090cdceac7a7de513244ef4cd976133f Gecko Version 44.0a2 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.worker.20151117.123256 Firmware Date Tue Nov 17 12:33:05 UTC 2015 Firmware Version v18D v4 Bootloader L1TC000118D0 Device: Aries KK 2.5 user build (Pass) Build ID 20151117132925 Gaia Revision 28d63cf3bdc4417f7ad8cab2230f096bf9f6d3b5 Gaia Date 2015-11-17 07:35:12 Gecko Revision http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/8f193d35090cdceac7a7de513244ef4cd976133f Gecko Version 44.0a2 Device Name aries Firmware(Release) 4.4.2 Firmware(Incremental) eng.worker.20151117.123839 Firmware Date Tue Nov 17 12:38:47 UTC 2015 Bootloader s1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: