Closed Bug 1014034 Opened 10 years ago Closed 10 years ago

Duck Duck Go content gets shifted down on every resize (keyboard dismissal)

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

Product:
Firefox for Android Graveyard
Component:
Toolbar
Version:
30 Branch
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox29 affected, firefox30 affected, firefox31 affected, firefox32 affected, fennec31+)

Status:
VERIFIED FIXED
Milestone:
Firefox 32
Tracking Flags:
Tracking Status
firefox29 --- affected
firefox30 --- affected
firefox31 --- affected
firefox32 --- affected
fennec 31+ ---

People

(Reporter: aaronmt, Assigned: kats)

References

Details

(Keywords: reproducible)

Attachments

(2 files)

screenshot.png
86.70 KB, image/png
Details
Patch
3.27 KB, patch
cwiiis
: review+
lmandel
: approval-mozilla-aurora-
Details | Diff | Splinter Review
Attached image screenshot.png — 
Currently on http://duckduckgo.com, on every resize (keyboard dismissal), the page content keeps getting shifted down from it's initial position after page-load altering it's original layout.

Chrome does not have this issue. On keyboard dismissal, after the resize the original layout is retained.

Steps to Reproduce

i) Visit http://duckduckgo.com
ii) Dismiss the keyboard from the initial field focus
iii) Invoke the keyboard, repeat step ii

--
Nightly (05/21)
LG Nexus 5 (Android 4.4.2)
Flags: needinfo?(bugmail.mozilla)
Component: General → Graphics, Panning and Zooming
As far as I can tell we're doing the right thing here. The page itself seems to be getting longer after following the STR in comment 0. That is, after dismissing the keyboard for the second time, you can scroll the page up and down significantly whereas you couldn't do that after dismissing the keyboard for the first time.

I assume the page is listening to resize events, because if I rotate the phone it fixes itself and resizes the page to fit. And I checked the values of window.innerWidth and window.innerHeight on resize, and they seem reasonable.
Flags: needinfo?(bugmail.mozilla)
Oh, interesting: when the page is in its "long" state, window.scrollMaxY is 0, but window.scrollY can go up to 329 (on my N4). That definitely seems wrong.
The <html> element of the content page has CSS properties:

height: 100%
min-height: 100%

and ends up with a height of 896px.

However the <browser> element that contains this document has a height of 567px. I don't know why the document is getting a height of 896px; perhaps somebody in layout can look into this.
Component: Graphics, Panning and Zooming → Layout
Product: Firefox for Android → Core
Version: Firefox 30 → 30 Branch
The size of the document and it's containing element are no longer linked once setCSSViewport is called. The caller of setCSSViewport will determine what the size of the document is.
Ok, I guess I should check what the CSS viewport is getting set to. I thought it was correct because the window.innerHeight value was right, but I forgot that the innerHeight actually comes from the scroll-position-clamping-scroll-port-size now.
Component: Layout → Graphics, Panning and Zooming
Product: Core → Firefox for Android
Version: 30 Branch → Firefox 30
I stepped through using the devtools debugger (amazing!) and saw that at [1] the page size is coming out to be 607x812 even though we just set the CSS viewport to be 384x519 a few lines above at [2]. This causes minScale to be ~1.26, which results in CSS viewport adjustment to make it 896px tall at [3].

When we read the page size at [2] it's the body's scrollWidth and scrollHeight that are larger than expected. Maybe we should be using getRootBounds() to get the page size instead of scrollWidth/scrollHeight?

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=b9c220c000a5#4257
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=b9c220c000a5#4217
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=b9c220c000a5#4281
Can we check for this in Fx29 and Fx30 to see what the range is?
Assignee: nobody → bugmail.mozilla
tracking-fennec: ? → 31+
Attached patch Patch — — Splinter Review 
Try push including this patch at https://tbpl.mozilla.org/?tree=Try&rev=37a728754cdc
Attachment #8427164 - Flags: review?(chrislord.net)
Comment on attachment 8427164 [details] [diff] [review]
Patch

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

Looks sound to me. I don't really know about getRootBounds to know if this is definitely what we should do, but I don't see this being any worse than what was there before. I assume you've tested that it works on a few pages, zooming in and out, rotating, etc. to make sure.
Attachment #8427164 - Flags: review?(chrislord.net) → review+
Yeah I'm not 100% sure either but we use getRootBounds in a bunch of places already to get the page size, so I think this change at least makes use the same things everywhere consistently. This was the last piece of the code that was using scrollWidth/Height to determine page size. I did some basic testing, yes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b14af86b2d6
https://hg.mozilla.org/mozilla-central/rev/3b14af86b2d6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Status: RESOLVED → VERIFIED
Comment on attachment 8427164 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown, a long time ago
User impact if declined: When dismissing the keyboard on duckduckgo.com, the page gets longer and is scrolled so as to be jarring to the user.
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): Might break some other scenarios. Affects Fennec only but I would put this at medium risk. I'd like to bake it on m-c for a week before uplifting, and even then I wouldn't it uplift it past aurora.
String or IDL/UUID changes made by this patch: none
Attachment #8427164 - Flags: approval-mozilla-aurora?
Kats - You don't sound very confident in comment 13. Note that a week of bake time on m-c will only leave us ~1 week of bake time on Aurora before uplift. Given that this bug has been around for a long time, that you categorize it as medium risk, and that we're late in the Aurora cycle, do you think it's reasonable to let this ride the 32 train instead of uplifting?
Flags: needinfo?(bugmail.mozilla)
Yeah, that's fair. We can let it ride the trains.
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8427164 [details] [diff] [review]
Patch

Aurora- based on comment 15.
Attachment #8427164 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: