956075 - Long-pressing at top of page with URL bar hidden triggers URL bar context menu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Galaxy Q running recent nightly. STR:

1. Load a page (e.g. mozilla.org) and scroll down so that the URL bar scrolls off the screen.
2. Position the content so that a link is at the top of the visible area (where the URL would be). On mozilla.org I use the "Firefox different by design" linked image.
3. Long-press on the link

Expected:

Context menu for link appears (includes options like "Open in new tab")

Actual:

Context menu for URL bar appears (includes options like "Edit site settings")

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

What device/Android version? Unable to reproduce on a Nexus 5.

Comment 2

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

Galaxy Q running android 2.3.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Galaxy Q running android 2.3.

Kats - Can you try Aurora and Beta to help narrow down the range?

Comment 4

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

Latest aurora is affected, 27.0b2 is not.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

That makes me suspect bug 768667.

Comment 6

[Benjamin Kerensa [:bkerensa]]

Can you have a look at this?

Comment 7

[Wesley Johnston (:wesj)]

Yes. Brad is bringing a Galaxy Q next week so I can debug.

Comment 8

[Flaviu Cos]

Good build 11.23.2013;
Bad build 11.24.2013;
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=98aa428a392c&tochange=74ab61b8d0f0

Tested on Samsung Galaxy S (Android 2.3.4)

Comment 9

[Aaron Train [:aaronmt]]

Confirmed bug 768667.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

I can reproduce this bug on a DroidPro running Gingerbread. Do we think this is screen size related or Gingerbread related?

Hmm, I can test on my Nexus S running Gingerbread too...  Yep, it's Gingerbread related. It happens on my Nexus S too.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: WIP patch

This is a WIP that attempts to hide the view flipper when its offscreen. I tried this the other day with no success, but trying it today I noticed some "Updating Views off the UI thread" errors I hadn't seen before. I'm not sure if that's because I'm testing on 4.3 now or if they weren't there on 2.3, but might be worth trying again:

http://dl.dropboxusercontent.com/u/72157/hideView.apk

If that doesn't work, I'd also like to try actually setting the scroll on the view, or maybe adding a third "empty" view to the view flipper.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #11)

> http://dl.dropboxusercontent.com/u/72157/hideView.apk
> 
> If that doesn't work, I'd also like to try actually setting the scroll on
> the view, or maybe adding a third "empty" view to the view flipper.

Tested on my Nexus S. Does not fix the problem. Tapping still displays the BrowseerSearch UI. Long pressing displays the URLBar context menu.

Comment 13

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

(In reply to Wesley Johnston (:wesj) from comment #11)
> http://dl.dropboxusercontent.com/u/72157/hideView.apk
> 

ARMv7 build? Crashes on startup on my Galaxy Q

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch v2

This overrides the ViewFlipper with our own class that ignores touches if the ViewFlipper is hidden. armv7 build at:
http://people.mozilla.org/~wjohnston/hideView.apk

I still haven't been able to pin down exactly where/how this was broken in GB phones, but the code changed drastically. For instance, 4.4 has these canViewReceivePointerEvents checks:

http://androidxref.com/4.2.2_r1/xref/frameworks/base/core/java/android/view/ViewGroup.java#1861

that aren't there anywhere in the 2.3 dispatchTouchEvent code:
http://androidxref.com/2.3.7/xref/frameworks/base/core/java/android/view/ViewGroup.java#824

I can't believe that all ViewGroups were this broken in 2.3 though...

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Just tested the build on my Nexus S and it works. Also tested on my DroidPro and it works.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch v2

This gets rid of the listeners and just uses the touch coordinate directly. Its a bit of a hack in that it assumes our view is at 0,0. For now that's fine here....

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Comment 18

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/4e6fcc7b5d13

Comment 19

[Aaron Train [:aaronmt]]

Is this on central?

Comment 20

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

No, it was backed out in https://hg.mozilla.org/integration/fx-team/rev/76b9e0793b76

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch

I'm not sure what's going on, but robocop seems to use a hit test when finding these views (I can't seem to find in their source where that is, but I see dispatchTouchEvent being hit). The touch that's used for that has its rawY set slightly outside the bounds of the element, while getX() returns a value thats in the center of the element.

getX/Y are probably better to use here anyway. Using them fixes the test locally for me. Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=82fdaec23a17

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 8369284 [details] [diff] [review]
Patch

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

Looks good.

::: mobile/android/base/widget/GeckoViewFlipper.java
@@ +11,5 @@
>  import android.graphics.Rect;
>  import android.os.Build;
>  import android.view.MotionEvent;
>  import android.widget.ViewFlipper;
> +import android.util.Log;

Unrelated, remove.

Comment 23

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/1da612492939

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1da612492939

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 8366257 [details] [diff] [review]
Patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): actionmode stuff
User impact if declined: can't tap top of screen
Testing completed (on m-c, etc.): landed on mc this week. seems smooth
Risk to taking this patch (and alternatives if risky): Pretty low risk. There aren't any other really good solutions I could come up with, beyond dropping 2.3 support or backing out the whole thing.
String or IDL/UUID changes made by this patch: None.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f7b84245670d
https://hg.mozilla.org/releases/mozilla-beta/rev/1b369b538f59

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/1b369b538f59

Comment 28

[Flaviu Cos]

Verified as fixed on builds:
- 29.0a2 (2014-02-10);
- 30.0a1 (2014-02-10).
Device: Samsung Galaxy S (Android 2.3.4).