Closed Bug 854289 Opened 8 years ago Closed 8 years ago

when dynamic toolbar activated, html elements mapping shifted downwards, affecting hyperlink touch and others

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

ARM
Android
defect
Not set
blocker

Tracking

(firefox19 unaffected, firefox20 unaffected, firefox21 unaffected, firefox22+ fixed)

RESOLVED FIXED
Firefox 22
Tracking Status
firefox19 --- unaffected
firefox20 --- unaffected
firefox21 --- unaffected
firefox22 + fixed

People

(Reporter: nirvn.asia, Assigned: cwiiis)

References

Details

(Keywords: regression, Whiteboard: [NavBar])

Attachments

(2 files)

Attached image annotated screenshot
Since dynamic toolbar was enabled, mapping of html element in relation to viewport is vertically shifted downwards. This is hugely problematic as touching on the screen over an hyperlink to open it will most of the time fail. When the user move the viewport, things are then mapped properly.

It must have something to do with failing to take into account the dynamic toolbar as the vertical shift downwards feels like it's roughly the height of the toolbar. I believe bug 852565 might have caused this regression.

See attached screenshot, which might does a better job than my description at explaining the issue :)

Step to reproduce:
1. Launch Firefox
2. Open m.facebook.com (good candidate as wildly used)
3. Click on the "Photo" hyperlink at the top, and notice how it'll actually select the text box below and open the generic "update status" panel without a file input to select photos
Alternative steps to reproduce:
1. Launch Firefox
2. Open m.facebook.com
3. Click on the discussion bubbles icon on the upper toolbar, and notice how it'll actually select the "Photo" hyperlink below the icon

This makes it more obvious.
Confirming. This makes you mis-click every link unless you pan first, so putting severity as blocker.
Blocks: 846772
Severity: normal → blocker
OS: Windows 7 → Android
Hardware: x86 → ARM
Duplicate of this bug: 854387
Blocks: dynamic-toolbar
No longer blocks: 846772
Keywords: regression
Duplicate of this bug: 854402
tracking-fennec: --- → ?
I'm unable to reproduce it on today's Nightly, when I tap the camera on Facebook I get an update-status box appear with the camera selected and a file-upload form appear correctly.

Can anyone else still reproduce?
This issue may have been resolved via bug 853831.
Not resolved over here, I can still reproduce this issue on all tries with a tinderbox build that includes commit from bug 853831.

When this bug is fixed, might be worth coming with a testcase - if possible - to detect touch location regressions in the future.
Duplicate of this bug: 855355
Duplicate of this bug: 855355
Is there work on this? I would recommend turning off the dynamic toolbar when this hits Aurora (middle of next week).
(In reply to Kevin Brosnan [:kbrosnan] from comment #11)
> Is there work on this? I would recommend turning off the dynamic toolbar
> when this hits Aurora (middle of next week).

I'm reasonably certain this won't be a big issue to solve, but currently on PTO - will be back on Tuesday. Just to add to the description, as I understand it, events aren't permanently displaced, just displaced until the viewport is changed (either by the user or the page scrolling) - this is likely the code to offset in setFirstPaintViewport (or whatever it's called...) being incorrect.

I'm happy to have this off and turn it on once this is fixed (or have the feature wait it out another version, though I question the usefulness of aurora in that case...), but if it can wait until Tuesday, even better.
This issue is really bad, highly visible impacts on most visited sites (twitter, facebook, etc.) If it's not fixed by time m-c is uplifted to Aurora, it should definitively be pref'ed off. 

Chris, it also happens if you scroll all the way to the top quickly (and past top so you get the elastic viewport effect).
Got a patch that fixes comment #1/comment #2, it's what I thought it would be in comment #12.

As for comment #13, I can't reproduce that. If I scroll down, then back up, events are offset correctly. Could you confirm that this is still an issue and post STR if it is?
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Flags: needinfo?(nirvn.asia)
Chris, I'll test things out thoroughly when your patch is applied. I have a lot of difficulty repeating problem raised in comment 13, and it might never be seen again when your patch lands.
Flags: needinfo?(nirvn.asia)
Hmm, I take it back, I can reproduce on facebook.com... Not sure why it was ok on other sites, will wait until my build finishes to see if this fixes the issue there.
Good, I'm mostly seeing it on facebook too.
Ah, found the other cause of the issue now, patch incoming.
Attachment #731520 - Flags: review?(bugmail.mozilla)
Comment on attachment 731520 [details] [diff] [review]
Fix incorrect mGeckoViewport setting

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

r=me if we do the update unconditionally like I described below. If there's a reason we can't do that then we should discuss that more.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +398,5 @@
>                                   newMetrics.pageRectTop - oldMetrics.fixedLayerMarginTop);
> +            } else {
> +                // We only need to update geckoMetrics on PAGE_SIZE messages, as
> +                // the setViewportMetrics call below will update mGeckoViewport
> +                // for UPDATE messages.

I feel a bit uncomfortable about this - tracing through the code I think statement is true if getRedrawHint() returns true, and I don't think that is guaranteed to always be the case. Can we just do this block unconditionally (i.e. move it out of the else clause so that it happens for both types of messages after the special-casing of UPDATE messages above)?
Attachment #731520 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)
> Comment on attachment 731520 [details] [diff] [review]
> Fix incorrect mGeckoViewport setting
> 
> Review of attachment 731520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if we do the update unconditionally like I described below. If there's
> a reason we can't do that then we should discuss that more.
> 
> ::: mobile/android/base/gfx/GeckoLayerClient.java
> @@ +398,5 @@
> >                                   newMetrics.pageRectTop - oldMetrics.fixedLayerMarginTop);
> > +            } else {
> > +                // We only need to update geckoMetrics on PAGE_SIZE messages, as
> > +                // the setViewportMetrics call below will update mGeckoViewport
> > +                // for UPDATE messages.
> 
> I feel a bit uncomfortable about this - tracing through the code I think
> statement is true if getRedrawHint() returns true, and I don't think that is
> guaranteed to always be the case. Can we just do this block unconditionally
> (i.e. move it out of the else clause so that it happens for both types of
> messages after the special-casing of UPDATE messages above)?

No reason why not, and well spotted - it was outside of the else clause before and I thought it was a mistake, but I just hadn't considered that.

Changed, pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/4f48fcad11ed
https://hg.mozilla.org/mozilla-central/rev/4f48fcad11ed
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Whiteboard: [NavBar]
tracking-fennec: ? → ---
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.