Regression: fixed position header and site interaction broken on DuckDuckGo Goodies; content hidden behind the address-bar

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: paul.feher, Assigned: botond)

Tracking

({regression, reproducible})

30 Branch
Firefox 31
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox29 unaffected, firefox30 verified, firefox31 verified, fennec30+)

Details

()

Attachments

(4 attachments, 6 obsolete attachments)

11.97 KB, patch
botond
: review+
Details | Diff | Splinter Review
3.09 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.23 KB, patch
botond
: review+
Details | Diff | Splinter Review
15.50 KB, patch
tnikkel
: feedback+
Details | Diff | Splinter Review
Build:Nightly 31.0a1 (2014-03-27),
Aurora 30.0a2 (2014-03-27)
Device: LG Optimus 4X HD (4.1.2)

Steps to reproduce:
1. Make sure the dynamic toolbar option is enabled
2. Load http://duckduckgo.com/goodies

Expected Results: 
The page header is displayed correct.

Actual Results:
The header is stuck behind our address-bar.

Note: I think this is a regression of Bug 923969.
Last good revision: 082761b7bc54 (2014-03-18)
First bad revision: 3bc3b9e2cd99 (2014-03-19)

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

Inbound:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=892dd317097f&tochange=b8f26064637f

b8f26064637f	Botond Ballo — Bug 983208 - Workaround for composition bounds being wrong on android. r=tnikkel Go back to using widget bounds. The root content document on android is a child document but it's treated like a root document in many ways. Do this until we can do what would technically be correct.
Blocks: 983208
tracking-fennec: --- → ?
Flags: needinfo?(chrislord.net)
Flags: needinfo?(botond)
Flags: in-testsuite?
Keywords: reproducible
Summary: Site interaction broken DuckDuckGo Goodies content hidden behind the address-bar → Regression: fixed position header and site interaction broken on DuckDuckGo Goodies; content hidden behind the address-bar
Version: Firefox 31 → Firefox 30
We should probably fix bug 985992 and back out the workaround.
Taking a look at the calculation of the root composition bounds here:

  - The view bounds (what we used prior to the regressing patch) are: [0, 540] x [0, 779]
  - The nearest widget bounds (what we use with the patch) are:       [0, 540] x [0, 850]

It seems the former excludes the toolbar's height (which is what we want), and the latter does not.

However, we can't just revert the patch and use the view bounds because that gives us the wrong result (e.g. very large numbers like [0, 1470] x [0, 2315]) on other pages.

I can discuss this with Timothy and try to come up with another hack to patch this up, but the proper fix is to fix bug 985992 like Kats suggests.
Depends on: 985992
Flags: needinfo?(chrislord.net)
Flags: needinfo?(botond)
Assignee: nobody → chrislord.net
tracking-fennec: ? → 30+
Moving this over to Botond, as he understands this code a lot better than I do.
Assignee: chrislord.net → botond
Posted patch bug988882.patch (obsolete) — Splinter Review
I discussed this with Timothy on Thursday and we came up with the idea of going back to clamping the composition bounds to the widget bounds (what we used to do prior to bug 935219) on Fennec, but only clamping the height and not the width. This seems to resolve the problem. My intuition is that it should not regress other things, but given how many time we got this not quite right I'm not at all certain about that...
Attachment #8399320 - Flags: review?(tnikkel)
Comment on attachment 8399320 [details] [diff] [review]
bug988882.patch

I talked to Chris on irc about this. Simplifying what I learned from him, setCSSViewport will set the height to the screen size if the page is big enough (ie scrollable), if it's smaller than the height will be the screen size minus the toolbar height.

So using the height of the view/frame as a sentinel in this way should be fine with how fennec currently works.

It looks like this patch uses the width from the view, and then min(view.height, widget.height) for the height. I think we want to use the width from the widget (the height is fine).

Also, please comment this in a clearer fashion.
Attachment #8399320 - Flags: review?(tnikkel)
For the sake of being able to find this later:

Layout doesn't have enough information to be able to calculate the correct composition bounds here. The bounds of the root frame are controlled via setCSSViewport and can be anything, there is nothing forcing them to have any relation to anything else. The widget bounds are the screen size. We don't know if fennec will render the page with the toolbar perma-visible (so our composition bounds should be screensize minus toolbar) or with a disappearing toolbar. With knowledge of how Fennec calls setCSSViewport we can use the frame height to determine which case we are in.
Posted patch bug988882.patch (obsolete) — Splinter Review
Updated patch to address comment 6.
Attachment #8399795 - Flags: review?(tnikkel)
Comment on attachment 8399795 [details] [diff] [review]
bug988882.patch

I think this issue is a separate issue from bug 985992. They are related, so the same fix might fix both, but we need to track both so the proper fix considers both (whether it be two separate fixes or one).

Can you make the commit msg more informative? Something like "special case handling of composition bounds for fennec when the toolbar is perma-visible".

Since this code is already a little complex I think it's worth getting the cleanest code here. Would it be clearer to structure it as

ifdef ANDROID
  GetNearestWidget
end

and then when we get the widget bounds (for both android and non-android) just have an ifdef for the special case height handling? To me that would be easier to follow.

Sorry if this feels nitpicky.
Posted patch bug988882.patch (obsolete) — Splinter Review
Updated patch to address comment 9.

And no, that wasn't nitpicky at all. It made the code quite a bit more readable!
Attachment #8399320 - Attachment is obsolete: true
Attachment #8399795 - Attachment is obsolete: true
Attachment #8399795 - Flags: review?(tnikkel)
Attachment #8399811 - Flags: review?(tnikkel)
Attachment #8399811 - Flags: review?(tnikkel) → review+
Comment on attachment 8399811 [details] [diff] [review]
bug988882.patch

Oh, and sadly we now have three places that calculate composition bounds. (That needs to be fixed.) Please change nsLayoutUtils::CalculateCompositionSizeForFrame and CalculateRootCompositionSize as well.
Duplicate of this bug: 989924
Posted patch bug988882.patch (obsolete) — Splinter Review
Updated patch to also modify the other two places the root composition bounds is computed. Re-requesting review.

It wasn't immediately obvious to me how to effectively reuse the code across these three places, so I haven't done so, but it seems like it should be possible. I will think some more about it and hopefully refactor things in a follow-up.
Attachment #8400422 - Flags: review?(tnikkel)
Attachment #8399811 - Attachment is obsolete: true
Posted patch bug988882.patch (obsolete) — Splinter Review
Sorry, last patch was slightly outdated. This is the good one.
Attachment #8400422 - Attachment is obsolete: true
Attachment #8400422 - Flags: review?(tnikkel)
Attachment #8400423 - Flags: review?(tnikkel)
Attachment #8400423 - Flags: review?(tnikkel) → review+
Rebased patch to apply to latest trunk (bug 989897 touched the same code). Carrying r+.
Attachment #8400423 - Attachment is obsolete: true
Attachment #8401707 - Flags: review+
hi, had to break this out since it seems this caused https://tbpl.mozilla.org/php/getParsedLog.php?id=37268539&tree=Mozilla-Inbound on debug builds. Seems at least 03:05:27     INFO -  [Parent 1275] WARNING: NS_ENSURE_TRUE(parentDocShell && treeItem != parentTreeItem) failed: file /builds/slave/m-in-osx64-d-00000000000000000/build/layout/base/nsPresShell.cpp, line 6091 is a lot of times in the logs
(In reply to Carsten Book [:Tomcat] from comment #18)
> hi, had to break this out since it seems this caused
> https://tbpl.mozilla.org/php/getParsedLog.php?id=37268539&tree=Mozilla-
> Inbound on debug builds. Seems at least 03:05:27     INFO -  [Parent 1275]
> WARNING: NS_ENSURE_TRUE(parentDocShell && treeItem != parentTreeItem)
> failed: file
> /builds/slave/m-in-osx64-d-00000000000000000/build/layout/base/nsPresShell.
> cpp, line 6091 is a lot of times in the logs

There are two failures here:

First, a timeout in a devtools test which seems unrelated and covered by bug 961004.

Second, an overflowing log, which seems to be caused by a spew of debug messages from PresShell::GetParentPresShell. I talked to :tn about this and it seems that PresShell::GetCumulativeResolution should not be using PresShell::GetParentPresShell, but instead be using GetParentPresContext. (My patch caused GetCumulativeResolution to be called a bit more often, hence the apparent regression.) Patch forthcoming.
To discourage people from calling it instead of calling GetParentPresContext.
Attachment #8403009 - Flags: review?(tnikkel)
Attachment #8403008 - Flags: review?(tnikkel) → review+
Attachment #8403009 - Flags: review?(tnikkel) → review+
Comment on attachment 8403014 [details] [diff] [review]
Use GetParentPresContext instead of GetParentPresShell in PresShell::GetCumulativeResolution

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

Old patch got r+ while I was uploading the new one, carrying.
Attachment #8403014 - Flags: review?(tnikkel) → review+
Flags: needinfo?(paul.feher)
Keywords: verifyme
Verified on Nightly 31.0a1 (2014-04-09),
Device: LG Optimus 4X HD (4.1.2)
Flags: needinfo?(paul.feher)
Status: RESOLVED → VERIFIED
Keywords: verifyme
Timothy, can we uplift this to aurora? It affects fennec 30.
Attachment #8407634 - Flags: feedback?(tnikkel)
Comment on attachment 8407634 [details] [diff] [review]
Rollup patch for aurora uplift

Assuming this is a straighforward uplift of the patches. I agree that we should uplift this to fix the regression.
Attachment #8407634 - Flags: feedback?(tnikkel) → feedback+
Comment on attachment 8407634 [details] [diff] [review]
Rollup patch for aurora uplift

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 983208
User impact if declined: Pages with position:fixed headers can have their headers hidden under the dynamic toolbar. The underlying problem has the potential to cause other types of incorrect rendering as well.
Testing completed (on m-c, etc.): Locally, baking on m-c for over a week.
Risk to taking this patch (and alternatives if risky): Not any more risky than uplifting bug 983208 was.
String or IDL/UUID changes made by this patch: None
Attachment #8407634 - Flags: approval-mozilla-aurora?
Attachment #8407634 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in build 30.0a2 (2014-04-18);
Devices: Motorola Razr (Android 4.0.4) and Lenovo Yoga Tab 10 (Android 4.2.2).
You need to log in before you can comment on or make changes to this bug.