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

RESOLVED FIXED in Firefox 22

Status

()

--
blocker
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: nirvn.asia, Assigned: cwiiis)

Tracking

({regression})

unspecified
Firefox 22
ARM
Android
regression
Points:
---

Firefox Tracking Flags

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

Details

(Whiteboard: [NavBar])

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 728827 [details]
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
(Reporter)

Comment 1

6 years ago
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

Updated

6 years ago
Duplicate of this bug: 854387

Updated

6 years ago
Blocks: 716403
No longer blocks: 846772
Keywords: regression

Updated

6 years ago
Duplicate of this bug: 854402

Updated

6 years ago
tracking-fennec: --- → ?
status-firefox19: --- → unaffected
status-firefox20: --- → unaffected
status-firefox21: --- → unaffected
status-firefox22: --- → affected
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.
(Reporter)

Comment 8

6 years ago
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).
tracking-firefox22: --- → ?
relnote-firefox: --- → ?
(Assignee)

Comment 12

6 years ago
(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.
(Reporter)

Comment 13

6 years ago
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).
(Assignee)

Comment 14

6 years ago
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)
(Reporter)

Comment 15

6 years ago
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)
(Assignee)

Comment 16

6 years ago
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.
(Reporter)

Comment 17

6 years ago
Good, I'm mostly seeing it on facebook too.
(Assignee)

Comment 18

6 years ago
Ah, found the other cause of the issue now, patch incoming.
(Assignee)

Comment 19

6 years ago
Created attachment 731520 [details] [diff] [review]
Fix incorrect mGeckoViewport setting
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+
(Assignee)

Comment 21

6 years ago
(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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22

Updated

6 years ago
status-firefox22: affected → fixed
tracking-firefox22: ? → +
relnote-firefox: ? → ---

Updated

6 years ago
Whiteboard: [NavBar]
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.