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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul Feher]

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.

Comment 1

[Aaron Train [:aaronmt]]

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.

Comment 2

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

We should probably fix bug 985992 and back out the workaround.

Comment 3

[Botond Ballo [:botond]]

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.

Comment 4

[Chris Lord [:cwiiis]]

Moving this over to Botond, as he understands this code a lot better than I do.

Comment 5

[Botond Ballo [:botond]]

Attachment: bug988882.patch

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 6

[Timothy Nikkel (: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.

Comment 7

[Timothy Nikkel (: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.

Comment 8

[Botond Ballo [:botond]]

Attachment: bug988882.patch

Updated patch to address comment 6.

Comment 9

[Timothy Nikkel (: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.

Comment 10

[Botond Ballo [:botond]]

Attachment: bug988882.patch

Updated patch to address comment 9.

And no, that wasn't nitpicky at all. It made the code quite a bit more readable!

Comment 11

[Botond Ballo [:botond]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=e92e7ede561c

Comment 12

[Timothy Nikkel (:tnikkel)]

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.

Comment 14

[Botond Ballo [:botond]]

Attachment: bug988882.patch

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.

Comment 15

[Botond Ballo [:botond]]

Attachment: bug988882.patch

Sorry, last patch was slightly outdated. This is the good one.

Comment 16

[Botond Ballo [:botond]]

Attachment: bug988882.patch

Rebased patch to apply to latest trunk (bug 989897 touched the same code). Carrying r+.

Comment 17

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bae32cf47dec

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Botond Ballo [:botond]]

(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.

Comment 20

[Botond Ballo [:botond]]

Attachment: Part 2 - Use GetParentPresContext instead of GetParentPresShell in PresShell::GetCumulativeResolution

Comment 21

[Botond Ballo [:botond]]

Attachment: Rename GetParentPresShell to GetParentPresShellForEventHandling

To discourage people from calling it instead of calling GetParentPresContext.

Comment 22

[Botond Ballo [:botond]]

Try push with the three patches together: https://tbpl.mozilla.org/?tree=Try&rev=fa86eec4e220

Comment 23

[Botond Ballo [:botond]]

Attachment: Use GetParentPresContext instead of GetParentPresShell in PresShell::GetCumulativeResolution

Whoops, can't do *= on a gfxSize.

ReTrying: https://tbpl.mozilla.org/?tree=Try&rev=a9454e042507

Comment 24

[Botond Ballo [:botond]]

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.

Comment 25

[Botond Ballo [:botond]]

(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

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6625164b8e4f
https://hg.mozilla.org/mozilla-central/rev/29f1e7607879
https://hg.mozilla.org/mozilla-central/rev/9a1549d94504

Comment 27

[Paul Feher]

Verified on Nightly 31.0a1 (2014-04-09),
Device: LG Optimus 4X HD (4.1.2)

Comment 28

[Botond Ballo [:botond]]

Attachment: Rollup patch for aurora uplift

Timothy, can we uplift this to aurora? It affects fennec 30.

Comment 29

[Timothy Nikkel (: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.

Comment 30

[Botond Ballo [:botond]]

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/439d286ed29a

Comment 32

[Flaviu Cos]

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).