Closed Bug 956075 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

29 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 + fixed
firefox29 + verified
firefox30 --- verified
b2g-v1.3 --- fixed
fennec 28+ ---

People

(Reporter: kats, Assigned: wesj)

References

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

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")
OS: Mac OS X → Android
Hardware: x86 → ARM
tracking-fennec: --- → ?
What device/Android version? Unable to reproduce on a Nexus 5.
Galaxy Q running android 2.3.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> Galaxy Q running android 2.3.

Kats - Can you try Aurora and Beta to help narrow down the range?
Latest aurora is affected, 27.0b2 is not.
That makes me suspect bug 768667.
Blocks: 768667
tracking-fennec: ? → 28+
Can you have a look at this?
Flags: needinfo?(wjohnston)
Yes. Brad is bringing a Galaxy Q next week so I can debug.
Assignee: nobody → wjohnston
Flags: needinfo?(wjohnston)
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)
Confirmed bug 768667.
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.
Attached patch WIP patch (obsolete) — Splinter Review
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.
(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.
(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
Attached patch Patch v2 (obsolete) — Splinter Review
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...
Attachment #8363974 - Attachment is obsolete: true
Attachment #8364796 - Flags: review?(lucasr.at.mozilla)
Just tested the build on my Nexus S and it works. Also tested on my DroidPro and it works.
Attached patch Patch v2 (obsolete) — Splinter Review
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....
Attachment #8364796 - Attachment is obsolete: true
Attachment #8364796 - Flags: review?(lucasr.at.mozilla)
Attachment #8366242 - Flags: review?(lucasr.at.mozilla)
Status: NEW → ASSIGNED
Attached patch Patch v3Splinter Review
Attachment #8366242 - Attachment is obsolete: true
Attachment #8366242 - Flags: review?(lucasr.at.mozilla)
Attachment #8366257 - Flags: review?(lucasr.at.mozilla)
Attachment #8366257 - Flags: review?(lucasr.at.mozilla) → review+
Is this on central?
Attached patch PatchSplinter Review
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
Attachment #8369284 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8369284 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/1da612492939
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
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.
Attachment #8366257 - Flags: approval-mozilla-beta?
Attachment #8366257 - Flags: approval-mozilla-aurora?
Attachment #8366257 - Flags: approval-mozilla-beta?
Attachment #8366257 - Flags: approval-mozilla-beta+
Attachment #8366257 - Flags: approval-mozilla-aurora?
Attachment #8366257 - Flags: approval-mozilla-aurora+
Verified as fixed on builds:
- 29.0a2 (2014-02-10);
- 30.0a1 (2014-02-10).
Device: Samsung Galaxy S (Android 2.3.4).
You need to log in before you can comment on or make changes to this bug.