1000423 - position: fixed elements are misplaced

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Mike Taylor [:miketaylr]]

Note this only reproduces in Aurora (30) or newer.

I noticed this in https://bugzilla.mozilla.org/show_bug.cgi?id=997728#c1, and reduced it down to: https://miketaylr.com/bzla/mango.html

1) Go to https://miketaylr.com/bzla/mango.html
2) Click on the "No click?" anchor element

Expected: an alert that says "click" (there's an inline onclick handler)
Actual: Nothing.

Also in logcat I get the following when I click on the "No click?" anchor:

W/TextureGenerator(26732): Clearing GL error: 0x500
W/Adreno-ES20(26732): <core_glBindTexture:484>: GL_INVALID_ENUM

Unsure if related. But it's consistent.

Somehow position:fixed is at fault here, as the position: absolute anchor receives click events just fine.

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: 1000423.html

Just noticed my testcase had a bug in it. Uploading fixed version.

Comment 2

[Aaron Train [:aaronmt]]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=082761b7bc54&tochange=3bc3b9e2cd99

Perhaps?

7364948c6566	Chris Lord — Bug 874950 - Don't let fixed position content occlude displayports. r=tn Don't let opaque fixed position items occlude display items that are in display ports and thus may be asynchronously scrolled.

There's this issue in the test-case where the visibility of the no-click <div> is completely opaque on older builds, you can still access the element and it receives the click event.

I think's masquerading as the issue here.

CC'ing Kats/Chris

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: simplified testcase

I think the problem here is drawing the fixed item at the wrong place. It should be at the very bottom of the document (release does that correctly). If you tap at the very bottom of the document where it should be you do get the click.

So maybe the positioning of the fixed layer in the pan zoom controller is done based on the visible region? That would explain the regression range.

Comment 4

[Timothy Nikkel (:tnikkel)]

The bug doesn't happen on b2g, and it also has a different pan zoom controller.

Comment 5

[Timothy Nikkel (:tnikkel)]

(In reply to Aaron Train [:aaronmt] from comment #2)
> Perhaps?
> 
> 7364948c6566	Chris Lord — Bug 874950 - Don't let fixed position content
> occlude displayports. r=tn Don't let opaque fixed position items occlude
> display items that are in display ports and thus may be asynchronously
> scrolled.

Hmm, disabling this locally didn't fix anything for me. In the same regression range we have bug 983208; without bug 983208 we are just completely messed up so that confounds things.

Comment 6

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

tn, assigning to you for now, but feel free to hand it off to Chris when he gets back if you think he would be a better person to look at this.

Comment 7

[Timothy Nikkel (:tnikkel)]

Pretty sure this is caused by bug 935219.

Without the fix from bug 983208 (fixing a regression from bug 935219) this is basically impossible to test. The simplified testcase is basically unusable. So I applied the fix from bug 983208 to the revision that landed bug 935219 and I can reproduce the bug there. The revision before doesn't have the same problem.

Comment 8

[Timothy Nikkel (:tnikkel)]

I think this is basically bug 988882 except here we have resolution != 1 and the calculation of the viewbounds is not correct for resolution different from 1.

There is also an existing problem here: if you load the simplified testcase (before bug 935219 landed) the fixed pos element will be correctly at the very bottom of the screen. However if you zoom it will be a little bit up from the bottom. I think this is due to some interaction of how we handle the disappearing toolbar in conjunction with fixed position elements on a page that fits completely on the screen (so that is is not scrollable). I'll file a separate bug for this.

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Talked to Botond on irc about this.

Bug 935219 changed from using the cumulative resolution to the parent resolution. According to our units system this works out. But it is not correct. It is a problem in our units system. (Just because the units work out doesn't mean it is correct.)

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #8)
> There is also an existing problem here: if you load the simplified testcase
> (before bug 935219 landed) the fixed pos element will be correctly at the
> very bottom of the screen. However if you zoom it will be a little bit up
> from the bottom. I think this is due to some interaction of how we handle
> the disappearing toolbar in conjunction with fixed position elements on a
> page that fits completely on the screen (so that is is not scrollable). I'll
> file a separate bug for this.

Filed bug 1005686 for this.

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8417072 [details] [diff] [review]
patch

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

::: layout/base/nsDisplayList.cpp
@@ +724,5 @@
>    nsIFrame* frameForCompositionBoundsCalculation = aScrollFrame ? aScrollFrame : aForFrame;
>    nsRect compositionBounds(frameForCompositionBoundsCalculation->GetOffsetToCrossDoc(aReferenceFrame),
>                             frameForCompositionBoundsCalculation->GetSize());
>    metrics.mCompositionBounds = RoundedToInt(LayoutDeviceRect::FromAppUnits(compositionBounds, auPerDevPixel)
> +                                            * LayoutDeviceToParentLayerScale(metrics.mCumulativeResolution.scale));

Please use 

  metrics.mCumulativeResolution 
* LayerToScreenScale(1.0)
* ScreenToParentLayerScale(1.0)

and add a comment saying that the ScreenToParentLayerScale(1.0) should really be metrics.mTransformScale, which is not calculated yet, and we can assume it's 1.0 because we are disregarding CSS transforms.

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8417141 [details] [diff] [review]
patch v2

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

It might be worth factoring out a variable (of type LayoutDeviceToParentLayerScale) rather than repeating the calculation and comment.

Comment 14

[Timothy Nikkel (:tnikkel)]

Good idea, I meant to do that but forgot.

Comment 15

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d3997797094

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8417141 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 935219
User impact if declined: position fixed items will have problems on page that don't have enough content to scroll
Testing completed (on m-c, etc.): just landed, this just needs a couple days on nightly to make sure there aren't obvious problems before uplift.
Risk to taking this patch (and alternatives if risky): pretty safe. this is just the fix for bug 988882 expanded to another case (that it should have covered already if it fixed the full problem). that patch has been landed for a month with no problems.
String or IDL/UUID changes made by this patch: none

note, we don't need this on b2g because b2g doesn't hit this same situation.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3d3997797094

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage drive-by: leaving this nomination to bake on Nightly for a few days, we'll get this into the Thursday Beta if all goes well.

Comment 19

[Flaviu Cos]

Verified as fixed in build 32.0a1 (2014-05-08);
Device: Motorola Razr (Android 4.0.4).

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8417141 [details] [diff] [review]
patch v2

approving for uplift now that we have verification.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2587f8841961
https://hg.mozilla.org/releases/mozilla-beta/rev/d7abd8044d61

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d7abd8044d61

Comment 23

[Flaviu Cos]

Verified as fixed in build 31.0a2 (2014-05-12);
Device: Motorola Razr (Android 4.0.4).

Comment 24

[Teodora Vermesan (:TeoVermesan)]

After tapping "No click?", a pop-up appears that says "clicked" so,
Verified fixed on:
Build: Firefox for Android 30 Beta 4 
Device: Alcatel One Touch (Android 4.1.2).