[fig] Tapping a bookmark list item causes the page to scroll to the top

RESOLVED FIXED in Firefox 26

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Margaret, Assigned: mcomella)

Tracking

({regression})

Trunk
Firefox 26
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-fig)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
This happens for both short taps and long taps. The touch event is handled, but it's handled for the target under your finger after the page scrolls up, making it impossible to tap anything below the fold.

We can use tinderbox builds to track down a regression:
https://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/fig-android/
Priority: -- → P1
http://hg.mozilla.org/projects/fig/rev/6c4b716fe189 is the suspicious changeset that caused this problem.
Comment out the onInterceptTouchEvent() from HomePager brings back old memories -- I mean, bookmarks in a good state.
Oh! and I've been facing a problem where, when long pressed, the first item is always in focused state. May be that's again related to this changeset.
Assignee: nobody → michael.l.comella
Posted patch 01: Patch (obsolete) — Splinter Review
Changes as discussed on IRC.

https://tbpl.mozilla.org/?tree=Try&rev=785cd91d4ad7
Attachment #784004 - Flags: review?(sriram)
(Reporter)

Comment 4

6 years ago
(In reply to Sriram Ramasubramanian [:sriram] from comment #2)
> Oh! and I've been facing a problem where, when long pressed, the first item
> is always in focused state. May be that's again related to this changeset.

Shilpan filed bug 899395 about this. But maybe they have the same root cause.
(Reporter)

Updated

6 years ago
Comment on attachment 784004 [details] [diff] [review]
01: Patch

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

::: mobile/android/base/home/HomePager.java
@@ +67,5 @@
> +        // However, when an adapter is empty (e.g. Reading List), there are no focusable Views in
> +        // touch mode. To compensate, we allow the HomePager to be focused but only when there are
> +        // no other descendants that can receive focus.
> +        setFocusableInTouchMode(true);
> +        setDescendantFocusability(FOCUS_AFTER_DESCENDANTS);

All these can be set in XML.

@@ +194,5 @@
>  
>      @Override
>      public boolean onInterceptTouchEvent(MotionEvent event) {
>          if (event.getActionMasked() == MotionEvent.ACTION_DOWN) {
> +            requestFocus(); // to lower the virtual keyboard.

Comment should be above the LOC.
Attachment #784004 - Flags: review?(sriram) → review-
Posted patch 01a: Patch (obsolete) — Splinter Review
Move focusability to XML attr. as discussed on IRC.
Attachment #784004 - Attachment is obsolete: true
Attachment #784030 - Flags: review?(sriram)
Comment on attachment 784030 [details] [diff] [review]
01a: Patch

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

r+ with comment change.

::: mobile/android/base/home/HomePager.java
@@ +183,5 @@
>  
>      @Override
>      public boolean onInterceptTouchEvent(MotionEvent event) {
>          if (event.getActionMasked() == MotionEvent.ACTION_DOWN) {
> +            requestFocus(); // to drop soft keyboard.

This line show be below the big comment.
And the "// to drop soft keyboard" show be above "requestFocus();".
It's not a good practice to add "executable; //comment", unless its a trivial thing like a final static constant. e.g:

private static final int TOP_PADDING = 5; //in pixels.
Attachment #784030 - Flags: review?(sriram) → review+
Posted patch 01b: PatchSplinter Review
Update comment, move r+.

https://tbpl.mozilla.org/?tree=Try&rev=63d59dc16281
Attachment #784030 - Attachment is obsolete: true
Attachment #784060 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/329abf307997
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.