I was testing the patches from bug 920036 which are pretty much ready to land and found a bug in the APZ hit-testing code. The problem is that if a HitTestingTreeNode returns a hit (i.e. ApzcHitRegion or ApzcContentRegion), there might still be no containing APZC for that node. In other words, when we reach the code at [1] for the notification tray layer, hitResult is ApzcHitRegion but result is still null. Because we check for !result as the termination condition in GetAPZCAtPoint, we continue the search and end up finding the content under notification tray which does have an APZC, and return that. [1] https://hg.mozilla.org/integration/mozilla-inbound/file/9cb763be208e/gfx/layers/apz/src/APZCTreeManager.cpp#l1382

This fixes it but I want to write a test and make sure there's no other fallout.

I'm considering renaming the HitTestResult values to not refer to Apzc specifically since it's kind of misleading now that we have nodes without APZCs. Any thoughts on that?

Comment on attachment 8546196 [details] [diff] [review] Patch with test Review of attachment 8546196 [details] [diff] [review]: ----------------------------------------------------------------- This patch confused me for the longest time because if you trace through the code, you'll find that we still recurse over the subtrees of older siblings after we find a non-APZC hit, we just never get into the block that actually calls HitTest() for them. Unless I'm missing something, this is unnecessary, and we can avoid it by changing the 'if (result)' just below the changes in this patch, to 'if (*aOutHitTestResult != NoApzcHit)'. r+ with that change. > I'm considering renaming the HitTestResult values to not refer to Apzc > specifically since it's kind of misleading now that we have nodes without > APZCs. Any thoughts on that? Yes please. r+.

(In reply to Botond Ballo [:botond] from comment #3) > > Unless I'm missing something, this is unnecessary, and we can avoid it by > changing the 'if (result)' just below the changes in this patch, to 'if > (*aOutHitTestResult != NoApzcHit)'. r+ with that change. Ah good point, I will make that change and land it. > > I'm considering renaming the HitTestResult values to not refer to Apzc > > specifically since it's kind of misleading now that we have nodes without > > APZCs. Any thoughts on that? > > Yes please. r+. Will do.

Updated as per review comment. I also changed the nested if condition to check for result, because that code (to skip past scrollinfo layers when event-regions are disabled) assumes result is non-null.

Renamed enum values; rs=botond from comment 3 (but let me know if you don't like the new names).

remote: https://hg.mozilla.org/integration/b2g-inbound/rev/57e3b87fa66f remote: https://hg.mozilla.org/integration/b2g-inbound/rev/27d6d4a76992

https://hg.mozilla.org/mozilla-central/rev/57e3b87fa66f https://hg.mozilla.org/mozilla-central/rev/27d6d4a76992

Backed out this bug for causing the smoketest regression in bug 1121033. https://hg.mozilla.org/integration/b2g-inbound/rev/86bcc35413b4

Rebased the "Rename enum values" patch across bug 1117712, which landed after this was backed out. Carrying r+.

(In reply to Botond Ballo [:botond] from comment #10) > Created attachment 8549123 [details] [diff] [review] > Rename enum values > > Rebased the "Rename enum values" patch across bug 1117712, which landed > after this was backed out. Carrying r+. This rebase assumes the Bug1117712 gtest comes first, followed by the Bug1119497 gtest. The rollup patch for 2.2 on bug 1117712 has them in the reverse order. I'm going with the rollup patch version.

Re-landed on inbound along with a proper fix for bug 1121033. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b04839eed789 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a4195efcb085

https://hg.mozilla.org/mozilla-central/rev/b04839eed789 https://hg.mozilla.org/mozilla-central/rev/a4195efcb085