Scrolling broken on Blackberry capacitive keyboard

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: eflores, Assigned: eflores)

Tracking

unspecified
Firefox 47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 8717509 [details] [diff] [review]
bb-scroll.patch

The capacitive keyboard scroll no longer works on the Blackberry Priv after native APZ was switched on.

Patch attached.
Created attachment 8717510 [details] [diff] [review]
apz-race.patch

Also ran into a race in APZC::DispatchStateChangeNotification.

Sometimes the monitored part of ~StateChangeNotificationBlocker can run between the monitored part of APZC::SetState and the |mNotificationBlockers| check in APZC::DispatchStateChangeNotification. And *then* ~StateChangeNotificationBlocker calls the same function. This can end in two of the same notification going out and causing assertion failures.
Assignee: nobody → edwin
Status: NEW → ASSIGNED
Attachment #8717510 - Flags: review?(bugmail.mozilla)
Attachment #8717509 - Flags: review?(bugmail.mozilla)
Comment on attachment 8717510 [details] [diff] [review]
apz-race.patch

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

I don't want to be holding the monitor while calling functions on GeckoContentController. Instead, can you extract the mNotificationBlockers>0 check into the call site and do the early return there? That way that check will still be inside the lock, but the call to DispatchStateChangeNotification can be outside the lock.
Attachment #8717510 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 8717509 [details] [diff] [review]
bb-scroll.patch

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

This is on the right track, but incomplete. When dealing with APZ, one does not simply call ReceiveInputEvent and walk away :p

::: mobile/android/base/java/org/mozilla/gecko/gfx/NativePanZoomController.java
@@ +84,5 @@
> +        android.util.TypedValue outValue = new android.util.TypedValue();
> +        if (view.getContext().getTheme().resolveAttribute(android.R.attr.listPreferredItemHeight, outValue, true)) {
> +            mPointerScrollFactor = -outValue.getDimension(view.getContext().getResources().getDisplayMetrics());
> +        } else {
> +            mPointerScrollFactor = -1.0f;

Why not also copy over the MAX_SCROLL bit from JavaPanZoomController?

@@ +102,3 @@
>      @Override
>      public boolean onMotionEvent(MotionEvent event) {
> +        return handleScrollEvent(event.getActionMasked(),

onMotionEvent will get called for all sorts of events, including joystick and whatnot - we'll need to filter the events here based on the source because the code you added is pretty specific to the bb keyboard and won't apply to other android input devices in general.

::: widget/android/nsWindow.cpp
@@ +695,5 @@
> +        ScreenPoint origin(aX, aY);
> +        ScreenPoint displacement(aXOffset, aYOffset);
> +
> +        // We don't know when the momentum scroll inputs will stop, so wrap
> +        // every scroll input in START/END inputs.

This is going to create a new input block for every event, which is less than ideal. I'd prefer having a timer that will send the momentum-end event after some time of not receiving any more momentum-move events.

More importantly though, you're not sending the notifications needed to process the input block in a timely manner, so every input block is going to be waiting for the full 300ms timeout if there's so much as a single wheel or touch listener on the document. There's a bunch more stuff that needs to be added here. You may wish to read through [1] if you haven't already.

[1] https://github.com/mozilla/gecko-dev/blob/master/gfx/doc/AsyncPanZoom.md#technical-details
Attachment #8717509 - Flags: review?(bugmail.mozilla)
This was fixed in bug 1225936, using ScrollWheelInput instead of faffing around with PanGestureInput.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1225936
Actually, a couple of issues still. Patch coming.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Created attachment 8720809 [details] [diff] [review]
1246984.patch

The Blackberry keyboard still sends events while other input devices are being used.

This patch ignores the ACTION_SCROLL once an ACTION_DOWN event with a different downTime is seen.
Attachment #8717509 - Attachment is obsolete: true
Attachment #8717510 - Attachment is obsolete: true
Attachment #8720809 - Flags: review?(bugmail.mozilla)
Comment on attachment 8720809 [details] [diff] [review]
1246984.patch

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

This looks ok, but couldn't we do the same check in Java-land instead of sending it down to c++? i.e. store the mLastEventStartTime in NativePanZoomController and do the check there.
Comment on attachment 8720809 [details] [diff] [review]
1246984.patch

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

I'll r+ this but if possible would prefer to have the version purely in java-land.
Attachment #8720809 - Flags: review?(bugmail.mozilla) → review+
Created attachment 8721314 [details] [diff] [review]
1246984-java.patch

Facepalm. That makes way more sense.
Attachment #8720809 - Attachment is obsolete: true
Attachment #8721314 - Flags: review?(bugmail.mozilla)
Comment on attachment 8721314 [details] [diff] [review]
1246984-java.patch

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

LGTM. Make sure you test the scenario where you put a finger down on the touchscreen, scroll a bit, and then without lifting the finger, do a keyboard swipe, and then continue moving the finger. From this patch it looks like we'll just abort the sequence of touch events which the APZ code *should* be able to handle but I don't think we've exercised that behaviour specifically before.
Attachment #8721314 - Flags: review?(bugmail.mozilla) → review+

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e0f184fd883
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
This patch is responsible for Bug 1251370. We may need to back it out until a proper solution can be found. What is going on is that downtime is per pointer, so a finger tap sets the mLastDownTime to the current time, but the mouse pointer device did not have a down event so it's event.getDownTime() is now before mLastDownTime so that any new scroll events from the mouse are ignored until the mouse pointer device has a new down event (from a mouse click) so that its event.getDownTime() is not the same as mLastDownTime.
Flags: needinfo?(edwin)
That should read: "so that its event.getDownTime() is *NOW* the same as mLastDownTime."
Edwin, do you know what TOOL_TYPE the blackberry keyboard-scrolling MotionEvent generates? We might be able to condition this behaviour on that.
Taking a look now. Should have a patch in a few hours.
Flags: needinfo?(edwin)
The TOOL_TYPE is the same (TOOL_TYPE_MOUSE), but the Blackberry keyboard's InputDevice has a source of SOURCE_TOUCHPAD. It seems reasonable to me to assume that touchpad devices will always report an ACTION_DOWN.
You need to log in before you can comment on or make changes to this bug.