Closed Bug 988882 Opened 7 years ago Closed 7 years ago
Regression: fixed position header and site interaction broken on Duck
Duck Go Goodies; content hidden behind the address-bar
11.97 KB, patch
|Details | Diff | Splinter Review|
3.09 KB, patch
|Details | Diff | Splinter Review|
1.23 KB, patch
|Details | Diff | Splinter Review|
15.50 KB, patch
|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.
tracking-fennec: --- → ?
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
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
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...
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.
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.
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.
Updated patch to address comment 9. And no, that wasn't nitpicky at all. It made the code quite a bit more readable!
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.
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 #8399811 - Attachment is obsolete: true
Sorry, last patch was slightly outdated. This is the good one.
Attachment #8400423 - Flags: review?(tnikkel) → review+
Rebased patch to apply to latest trunk (bug 989897 touched the same code). Carrying r+.
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)
Try push with the three patches together: https://tbpl.mozilla.org/?tree=Try&rev=fa86eec4e220
Attachment #8403008 - Flags: review?(tnikkel) → review+
Attachment #8403009 - Flags: review?(tnikkel) → review+
Whoops, can't do *= on a gfxSize. ReTrying: https://tbpl.mozilla.org/?tree=Try&rev=a9454e042507
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+
(In reply to Botond Ballo [:botond] from comment #23) > ReTrying: https://tbpl.mozilla.org/?tree=Try&rev=a9454e042507 Looks clean. Relanded. https://hg.mozilla.org/integration/mozilla-inbound/rev/6625164b8e4f https://hg.mozilla.org/integration/mozilla-inbound/rev/29f1e7607879 https://hg.mozilla.org/integration/mozilla-inbound/rev/9a1549d94504
https://hg.mozilla.org/mozilla-central/rev/6625164b8e4f https://hg.mozilla.org/mozilla-central/rev/29f1e7607879 https://hg.mozilla.org/mozilla-central/rev/9a1549d94504
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Verified on Nightly 31.0a1 (2014-04-09), Device: LG Optimus 4X HD (4.1.2)
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.