950225 - [B2G][Calendar]When adding a Yahoo or Caldav account hidden buttons in the background can be pressed

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Josh Schmitt [Joshs]]

Description:
Hidden buttons 'Weekly' 'Day' or the alarm selection of an event are being pressed if the user selects part of the page while adding a Yahoo or Caldav account.

Repro Steps:
1) Updated Buri to BuildID: 20131213004002
2) Open the Calendar App
3) Select the drawer icon
4) Select the gear icon
5) Select the '+' icon
6) Select Caldav
7) While on the Caldav page, tap near the bottom right corner

Buri 1.3
Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20131213004002
Gaia: 888f9df5515a47d2f5806efee77485e05e1e5416
Gecko: dfae9c83bfbc
Version: 28.0a2
RIL Version: 01.02.00.019.102
Firmware Version: v1.2_20131115

Comment 1

[Jason Smith [:jsmith]]

Can you get a video?

Comment 2

[Josh Schmitt [Joshs]]

Does not occur on Buri 1.2 MOZ.

Environmental Variables:
Device: Buri 1.2 MOZ
BuildID: 20131213004002
Gaia: 1aca7c4860e39b1a9969807d335dcf9f070ea9b3
Gecko: a2b69b561d9b
Version: 26.0
RIL Version: 01.02.00.019.102
Firmware Version: V1.2_20131115

Video - http://youtu.be/iKSzRr-Mb-8

Comment 3

[gbennett]

Here is a repro video using the latest 1.3 mozRIL: http://www.youtube.com/watch?v=eJun6lhK08M

Environmental Variables:
Device: Buri 1.3 mozRIL
BuildID: 20131213004002
Gaia: 888f9df5515a47d2f5806efee77485e05e1e5416
Gecko: dfae9c83bfbc
Version: 28.0a2
RIL Version: 01.02.00.019.102
Firmware Version: 20131115

Comment 4

[Jason Smith [:jsmith]]

How many places on that caldav screen can there be hidden buttons triggered?

Comment 5

[gbennett]

Here is a video showing all the hidden buttons within caldav and yahoo that can be pressed: http://www.youtube.com/watch?v=MzSRprh2tl4

Comment 6

[Jason Smith [:jsmith]]

There's something really broken here.

Comment 7

[Dylan Oliver [:doliver]]

triage: 1.3+ for regression

Comment 8

[Miller Medeiros [:millermedeiros]]

I could not replicate the bug inside b2g-bin or inside firefox nightly (I still don't have a device to test).

Does anyone have any idea of what could be causing the problem? For me this bug doesn't make sense (looking at code from v1.3 branch), the `section#modify-account-view` has `z-index:1200` and is supposed to block ALL the touch/click interactions of the layers below it *unless* it has `pointer-events:none` (which isn't the case).

Can anyone confirm that this bug still happens with code on v1.3 branch? (any commit from this week) If that is the case this bug might be related with the way Gecko is interpreting the layers, which I wouldn't be able to fix by myself and don't even know who should I ask for help.

PS: one "hack" that would probably fix the problem is to set `pointer-events:none` to the `section#time-views` every time a `.fullscreen-view` is opened - which I wouldn't recommend since it would just beget more hacks in the future and probably mask the real bug.

Comment 9

[Jason Smith [:jsmith]]

QA Wanted - Can we confirm this is still happening on the latest 1.3 build?

Comment 10

[gbennett]

This issue repros on the latest 1.3

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140109004002
Gaia: 22bc6be5b76cdc6d4e9667ff070979041a20ce2f
Gecko: 2c8f8683bd0d
Version: 28.0a2
Firmware Version: v1.2_20131115

Comment 11

[Punam Dahiya [:pdahiya]]

Here are my findings 

This issue is not replicable on latest gaia master with gecko as 1.2

Environment
OS Version 1.2.0.0. -prerelease
Platform: 26.0
BuildId: 20140109004002
Gecko : - http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ec909be2a849

Updated with latest Gaia Master: 61797fd7cf908f13f140b98f94ace038e2321c05
using make reset-gaia

It appears to be a gecko issue which is replicable in gecko 1.3 and 1.4 irrespective
of gaia version

Kevin, I am not familiar with calendar app. Can you pl. validate the above findings and help
assign to the right team.
Thanks

Comment 12

[Dylan Oliver [:doliver]]

I can reproduce this easily on device (Geeksphone Peak) with today's 1.4 build, pressing in the lower right hand corner on the "Add an account" screen.

Comment 13

[Kevin Grandon :kgrandon]

I am also able to reproduce this, but unfortunately I will unlikely have cycles to do serious debugging for the next 1-2 weeks.

Comment 14

[Kevin Grandon :kgrandon]

I am going to clear the ni? flag for now, and can take another look in the future if needed.

Comment 15

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

Evan,

There were only a handful of commits to calendar in 1.3, so it might be useful to bisect through those and also check to see if this reproduces on gecko 26 because it could also be a platform bug.

Comment 16

[Evan Tseng [:evanxd]]

Hi Gareth,

The bug is also happened in Gaia master branch on gecko 29.
So, yes, it is a platform bug.

And the gecko 26 is used in v1.2.
Do we need to fix the bug for v1.2?
(We could see the mapping list in https://wiki.mozilla.org/Release_Management/B2G_Landing#Versions_and_Scheduling)

Thanks.

Comment 17

[Evan Tseng [:evanxd]]

Attachment: Pull request

Hi Gareth,

We do workaround here.
Because the click event just pass through the "create-account-view" panel to the under panel
with unknown reasons, then the bug is happened.
It might be a Gecko issue.

And the bug is just happened in real device not b2g-desktop or nightly.
So we do need to have marionette tests for it on b2g-desktop.

If we want to fix the issue without any workaround, we might fix it in Gecko not in Gaia.
But I think we could fix it in workaround way, and file a follow up bug for the possible gecko issue.

So please help me review the patch.
Thanks.

Comment 18

[Evan Tseng [:evanxd]]

Comment on attachment 8360324 [details] [review]
Pull request

Hi Kevin,

Could you have time to give me feedback for the patch?
Thanks.

Evan

Comment 19

[Kevin Grandon :kgrandon]

Comment on attachment 8360324 [details] [review]
Pull request

I generally hate workarounds, so I'm hesitant to provide an R+ or F+ unless we understand what platform issue is causing this. My recommendation would be to needsinfo a few gecko developers to see if they would know. 

Thank you for submitting a patch with a workaround though! It is really nice to have. My preference would be to just wait until we understand *what* is causing this, then land. But maybe Gareth will want to land anyway :)

Comment 20

[Evan Tseng [:evanxd]]

Hi Kevin,

Thanks for the response.

Yes, we could cc Kanru a gecko guy to figure out the issue.
And thank Kanru.

Comment 21

[Evan Tseng [:evanxd]]

Hi Kanru,

The event target is the element under "create-account-view" panel.
(We click on right side on "create-account-view" panel)

The click event is just pass through "create-account-view" panel to the under panel.
Do you have any idea for this?
Thanks.

Comment 22

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

I don't know enough about the DOM stuff going on in calendar, but for e-mail in bug 960290 it appears that the user is able to click buttons on cards that are off-screen to the right/left, presumably because of click area inflation.  Are the things being clicked actually under the visible card, or are they off to the side?

Comment 23

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

Hm, and it appears to have been an 0.5px offset in e-mail though.  So maybe it's something different.  Well, something to consider!

Comment 24

[Kevin Grandon :kgrandon]

(In reply to Andrew Sutherland (:asuth) from comment #23)
> Hm, and it appears to have been an 0.5px offset in e-mail though.  So maybe
> it's something different.  Well, something to consider!

I wonder if these are being caused by some gaia change to support edge gestures or something? I also noticed that it's possible to dismiss the keyboard by tapping at the bottom of the screen. I wonder if we can narrow down the root cause and avoid any workarounds. I may be able to look in the next few days..

Comment 25

[Kevin Grandon :kgrandon]

Vivien - any ideas?

Comment 26

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

I'm not sure the workaround is what we want here (for calendar that is). If that was the solution, then we would be able to click the frame underneath anywhere on the account create screen. I think edge gestures is good intuition. Let's see if we can figure out where this was introduced and go from there.

Comment 27

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

Comment on attachment 8360324 [details] [review]
Pull request

^^

Comment 28

[Evan Tseng [:evanxd]]

Hi all,

I disabled the edge gestures before, but the bug was still happened.
So I think the bug is not related with edge gestures.

I also tried to let create-account-view have 120% width, then the bug is not happened.

Comment 29

[Evan Tseng [:evanxd]]

(In reply to Andrew Sutherland (:asuth) from comment #23)
> Hm, and it appears to have been an 0.5px offset in e-mail though.  So maybe
> it's something different.  Well, something to consider!

Hi Andrew,

Thanks for the tips.

I used the app manager to figure out the width of create-account-view and the under panel in device. The width of them are the same as 320px.

And the bug just could be reproduced in real device not in nightly or b2g-desktop.

Comment 30

[Evan Tseng [:evanxd]]

And as I know, there is no 0.5px things in Calendar app.
But it is really a way to consider.
Really thanks.

Comment 31

[Adam Rogers (:arog)]

PM Triaged, considered a blocker for 1.3

Comment 32

[Kan-Ru Chen [:kanru] (UTC+9)]

Investigating.

Comment 33

[Kan-Ru Chen [:kanru] (UTC+9)]

Reproduced on master

Gaia      c75a943bb4c2ca329c01406211c4fe77186d49cb
Gecko     7d855479045ad673d20cb398391542734b3ff5f7
BuildID   20140116120359
Version   29.0a1
ro.build.version.incremental=eng.cltbld.20131108.071732
ro.build.date=Fri Nov  8 07:39:33 EST 2013

Comment 34

[Kan-Ru Chen [:kanru] (UTC+9)]

Also reproduced on b2g-desktop of the same gecko/gaia revision.

Comment 35

[Kan-Ru Chen [:kanru] (UTC+9)]

The issue is caused by hit-target fluffing. Try set ui.touch.radius.enabled in b2g.js to false and the issue disappear. The hit test selected the calender view because it was not hidden and have event listener bound to it. You could workaround this by hide the calender view if it is covered by something else.

To fix it properly I think we should ignore the region outside of the viewport when doing hit-target fluffing.

Comment 36

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Restrict event fluffing in visible area

Comment 37

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Restrict event fluffing in visible area with test case

Add test case.

I don't know a better way to get the bound rect of the document :/

Comment 38

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

Comment on attachment 8364846 [details] [diff] [review]
Restrict event fluffing in visible area with test case

Review of attachment 8364846 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/PositionedEventTargeting.cpp
@@ +264,4 @@
>               AppUnitsFromMM(aRootFrame, aPrefs->mSideRadii[3], false));
>    nsRect r(aPointRelativeToRootFrame, nsSize(0,0));
>    r.Inflate(m);
> +  return MoveInsideVisibleArea(aRootFrame, aRestrictToDescendants, r);

Let's call this ClipToFrame, and just intersect r with the bounds of aRestrictToDescendants (converted to the correct coordinate system using nsLayoutUtils::TransformFrameRectToAncestor, not GetOffsetToCrossDoc which won't take transforms into account properly). We know that aRootFrame is an ancestor of restrictToDescendants.

Comment 39

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Restrict event fluffing in visible area with test case v2.

https://tbpl.mozilla.org/?tree=Try&rev=4c846c815675

Comment 40

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

Comment on attachment 8365766 [details] [diff] [review]
Restrict event fluffing in visible area with test case v2.

Review of attachment 8365766 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/PositionedEventTargeting.cpp
@@ +231,5 @@
> + * Clip aRect with the bounds of aFrame in the coordinate system of
> + * aRootFrame. aRootFrame is an ancestor of aFrame.
> + */
> +static nsRect
> +ClipToFrame(nsIFrame* aRootFrame, nsIFrame* aFrame, nsRect aRect)

const nsRect& aRect

@@ +234,5 @@
> +static nsRect
> +ClipToFrame(nsIFrame* aRootFrame, nsIFrame* aFrame, nsRect aRect)
> +{
> +  nsRegion bound = nsLayoutUtils::TransformFrameRectToAncestor(
> +    aFrame, aFrame->GetRect(), aRootFrame);

aFrame->GetRect()'s top-left is not 0,0. So here you need nsRect(nsPoint(0, 0), aFrame->GetSize()).

Comment 41

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bfec63e417b

Comment 42

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/071ced6c6e9d for test failures:

desktop: https://tbpl.mozilla.org/php/getParsedLog.php?id=33674357&tree=Mozilla-Inbound
opt emulator mochitest-9: https://tbpl.mozilla.org/php/getParsedLog.php?id=33674941&tree=Mozilla-Inbound
debug emulator mochitest-14: https://tbpl.mozilla.org/php/getParsedLog.php?id=33674523&tree=Mozilla-Inbound

Comment 43

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8365766 [details] [diff] [review]
Restrict event fluffing in visible area with test case v2.

>+ClipToFrame(nsIFrame* aRootFrame, nsIFrame* aFrame, nsRect aRect)
>+{
>+  nsRegion bound = nsLayoutUtils::TransformFrameRectToAncestor(
>+    aFrame, aFrame->GetRect(), aRootFrame);
>+  nsRegion result = bound.Intersect(aRect);
>+  return result.GetBounds();
>+}

When this relands you should change nsRegion to nsRect here. TransformFrameRectToAncestor returns a rect, not a region.

Comment 44

[Kan-Ru Chen [:kanru] (UTC+9)]

The mochitest test runner created a iframe with width 500 which is wider than the browser window so the test couldn't simulate a mouse click in that region.

Comment 45

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=6c1d3aa80615

Disable the two problemtic test when the window is too narrow.

Comment 46

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=e98fdff8a6d2

https://hg.mozilla.org/integration/mozilla-inbound/rev/192c0bc86b01

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/192c0bc86b01

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ce98c76af3f5

Comment 49

[Ryan VanderMeulen [:RyanVM]]

This was backed out from Aurora because test_reftests_with_caret.html went perma-fail on Win7/Win8 after it landed. Funny enough, I'd filed bug 965526 earlier today for the exact same failure on trunkand Ehsan had already landed a fix on inbound for it. And indeed, it fixed the perma-fail here as well, so re-landed :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/60d7769f9d4c

If you're wondering, my *guess* is that the changes in test_event_target_radius.html (which runs in the same mochitest chunk as test_reftests_with_caret.html) somehow perturbed the other test into failing. In other words, I don't think this directly caused the failure but made an already-flaky test more failure-prone that it was before.