position: fixed elements are misplaced

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: miketaylr, Assigned: tnikkel)

Tracking

({regression, reproducible})

30 Branch
Firefox 32
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 unaffected, firefox30+ verified, firefox31+ verified, firefox32+ verified, b2g-v1.4 fixed, b2g-v2.0 fixed, fennec30+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
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.
Reporter

Updated

5 years ago
Blocks: 997728
tracking-fennec: --- → ?
Reporter

Updated

5 years ago
No longer blocks: 997728
See Also: → 997728
Reporter

Comment 1

5 years ago
Posted file 1000423.html
Just noticed my testcase had a bug in it. Uploading fixed version.
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
Blocks: 874950
OS: Mac OS X → Android
Hardware: x86 → ARM
Assignee

Comment 3

5 years ago
Posted file 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.
Assignee

Updated

5 years ago
Attachment #8412753 - Attachment is patch: false
Attachment #8412753 - Attachment mime type: text/plain → text/html
Assignee

Comment 4

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

Comment 5

5 years ago
(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.
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.
Assignee: nobody → tnikkel
Component: General → Graphics, Panning and Zooming
Summary: position: fixed element doesn't receive click event → position: fixed elements are misplaced
Assignee

Comment 7

5 years ago
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.
Blocks: 935219
No longer blocks: 874950
tracking-fennec: ? → 30+
Assignee

Comment 8

5 years ago
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.
Assignee

Comment 9

5 years ago
Posted patch patch (obsolete) — Splinter Review
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.)
Attachment #8417072 - Flags: review?(botond)
Assignee

Comment 10

5 years ago
(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 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.
Attachment #8417072 - Flags: review?(botond) → review-
Assignee

Comment 12

5 years ago
Posted patch patch v2Splinter Review
Attachment #8417072 - Attachment is obsolete: true
Attachment #8417141 - Flags: review?(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.
Attachment #8417141 - Flags: review?(botond) → review+
Assignee

Comment 14

5 years ago
Good idea, I meant to do that but forgot.
Assignee

Comment 16

5 years ago
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.
Attachment #8417141 - Flags: approval-mozilla-beta?
Attachment #8417141 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3d3997797094
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
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

5 years ago
Verified as fixed in build 32.0a1 (2014-05-08);
Device: Motorola Razr (Android 4.0.4).
Comment on attachment 8417141 [details] [diff] [review]
patch v2

approving for uplift now that we have verification.
Attachment #8417141 - Flags: approval-mozilla-beta?
Attachment #8417141 - Flags: approval-mozilla-beta+
Attachment #8417141 - Flags: approval-mozilla-aurora?
Attachment #8417141 - Flags: approval-mozilla-aurora+

Comment 23

5 years ago
Verified as fixed in build 31.0a2 (2014-05-12);
Device: Motorola Razr (Android 4.0.4).
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).
Status: RESOLVED → VERIFIED
See Also: → 1085569
You need to log in before you can comment on or make changes to this bug.