Closed Bug 672483 Opened 8 years ago Closed 8 years ago

Find in Page toolbar scrolls down with the page when URL bar is displayed

Categories

(Firefox for Android Graveyard :: General, defect, P4, minor)

Firefox 7
ARM
Android

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 8

People

(Reporter: xti, Assigned: lucasr)

Details

Attachments

(2 files, 2 obsolete files)

Attached image Screenshot - Aurora 7
This issue occurs on:
Build id : Mozilla/5.0 (Android;Linux armv7l;rv:7.0a2)Gecko/20110718
Firefox/7.0a2 Fennec/7.0a2
Device: Motorola Droid 2
OS: Android 2.2

Steps to reproduce:
1. Switch device orientation to landscape
2. Open Fennec app
3. Scroll the page down until the URL bar is hidden
4. Tap on Menu > Find in Page
5. Scroll the page up until URL bar is on the screen

Expected result:
Find in Page toolbar doesn't scroll up/down and it maintains its position.

Actual result:
Find in Page toolbar scrolls down until it reaches the bottom edge of the screen.
Severity: normal → minor
I can reproduce this using a G2.
Priority: -- → P4
If there is no HKb, after step 4, hit the device Back button to dismiss the VKb and then perform step 5. There will be the same actual result.
Assignee: nobody → lucasr.at.mozilla
There seems to be a race condition on the getBoundingClientRect() when setting the position of the find bar (bindings.xml:1596, for reference) which causes the height to be calculated without the container padding. There's already a comment about the race in the respective code.

It only happens the first time you use the "Find on Page" while in landscape mode. It doesn't happen on the following times you activate it (at least on my device).

Adding a tiny 100ms timeout (instead of doing in a 0ms timeout) makes it work consistently on my device. This is a bit hacky but I couldn't think of a better way to work around something as fundamental as a race in getBoundingClientRect(). Any other ideas?
That code was added in bug 622578. Maybe Vivien knows a proper solution here.
Here's the tiny patch. I generally don't like adding timeout's like this because sometimes they only happen to work on certain situations, devices, etc. Might be worth testing this patch on a few different devices just to make sure it actually works fine.

As I said, other ideas to work around the race in getBoundingClientRect() are welcome.
Attachment #548932 - Flags: review?(mark.finkle)
Comment on attachment 548932 [details] [diff] [review]
Add a tiny timeout before updating find bar's initial position

Let's see if a nested setTimeout(..., 0) would work here. If not, I'd be interested at the minimum timeout needed for this to work. Is it 10ms, 40ms? If we need to use a fixed number, I'd like to know if we are 2 ro 3 times greater than the minimum.
Attachment #548932 - Flags: review?(mark.finkle) → review-
The nested timeouts seem to work fine. Less random than a magic number. I updated the comment to describe the new workaround.
Attachment #549006 - Flags: review?(mark.finkle)
Attachment #549006 - Flags: review?(mark.finkle) → review+
Attachment #548932 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to comment #6)
> Comment on attachment 548932 [details] [diff] [review] [review]
> Add a tiny timeout before updating find bar's initial position
> 
> Let's see if a nested setTimeout(..., 0) would work here. If not, I'd be
> interested at the minimum timeout needed for this to work. Is it 10ms, 40ms?
> If we need to use a fixed number, I'd like to know if we are 2 ro 3 times
> greater than the minimum.

I think the 2 setTimeout will make it works most of the time, even with 0. There will probably be some cases where it will fail but this will remind us to look inside the platform with it happens.
Why is a nested timeout necessary?
(In reply to comment #9)
> Why is a nested timeout necessary?

There is a race condition somewhere, that means the width/height of the toolbar should be set at the point one of the function is called, but it isn't (yet) for some reasons. Using setTimeout let the platforms finish some of it works to update the width/height. 
It looks like one setTimeout is not enough to have it done.
Would calling this.getBoundingClientRect(); twice help?
(In reply to comment #11)
> Would calling this.getBoundingClientRect(); twice help?

no :/
Attachment #549006 - Attachment is obsolete: true
Attachment #549868 - Flags: review?(mark.finkle)
Attachment #549868 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/10aece9bb796
Keywords: checkin-needed
Whiteboard: [inbound]
Target Milestone: --- → Firefox 8
http://hg.mozilla.org/mozilla-central/rev/10aece9bb796
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Verified Fixed
Mozilla/5.0 (Android; Linux armv7l; rv:8.0a1) Gecko/20110811 Firefox/8.0a1 Fennec/8.0a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.