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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Mathieu Pellerin]

Attachment: 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

Comment 1

[Mathieu Pellerin]

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.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Confirming. This makes you mis-click every link unless you pan first, so putting severity as blocker.

Comment 5

[Aaron Train [:aaronmt]]

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bb9dce2a4977&tochange=74c06a7308d1

Comment 6

[Aaron Train [:aaronmt]]

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?

Comment 7

[Aaron Train [:aaronmt]]

This issue may have been resolved via bug 853831.

Comment 8

[Mathieu Pellerin]

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.

Comment 11

[Kevin Brosnan [Ex-Mozilla]]

Is there work on this? I would recommend turning off the dynamic toolbar when this hits Aurora (middle of next week).

Comment 12

[Chris Lord [:cwiiis]]

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

Comment 13

[Mathieu Pellerin]

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

Comment 14

[Chris Lord [:cwiiis]]

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?

Comment 15

[Mathieu Pellerin]

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.

Comment 16

[Chris Lord [:cwiiis]]

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.

Comment 17

[Mathieu Pellerin]

Good, I'm mostly seeing it on facebook too.

Comment 18

[Chris Lord [:cwiiis]]

Ah, found the other cause of the issue now, patch incoming.

Comment 19

[Chris Lord [:cwiiis]]

Attachment: Fix incorrect mGeckoViewport setting

Comment 20

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

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

Comment 21

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f48fcad11ed