525997 - adjust row highlighting behaviour

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

As of bug 519577, rows highlight when tapped.  The current behaviour isn't quite right though.

The point of row highlighting is to give immediate feedback that a user has actually tapped on a row (that the tap has "taken").  At the moment, the highlighting also shows up when a user is trying to pan the screen, at the point of initial tap and continuing until the user's finger leaves the screen.  This is (a) unnecessary, in that there is enough feedback for panning in that the page moves, and (b) actually confusing because, when panning, the user _doesn't_ want a tap to have "taken" -- tapping usually produces some other effect (navigating to a page, opening a row for editing, etc.) than is not the desired one when the user is trying to pan.

So, in my perfect world, a row would never highlight if the user is panning and would highlight when the user is just tapping.  I realize that there are difficulties in making a distinction between the two in the first instants of "finger-down".

What if we highlight the row on finger-down but unhighlighted as soon as we know that the tap has become a pan?

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip

Working wip for hildon.
This wip set a "pan" attribute on the scroll element but i would be happy to not have to use an attribute if possible.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP-2

Wip using the inIDOMUtils interface to kill the :active style on a pan

Comment 3

[Benjamin Stover (:stechz)]

Comment on attachment 409919 [details] [diff] [review]
WIP-2

>diff -r 5d2d8a352d68 chrome/content/InputHandler.js
>@@ -501,16 +504,17 @@ MouseModule.prototype = {
>     if (targetScrollInterface) {
>+      this._active = true;
>       this._doDragStart(evInfo.event);
>     }

Active state shouldn't be necessary.  dragData.dragging is equivalent.

>+        if (this._active) {
>+          this._active = false
>+          let state = this._domUtils.getContentState(evInfo.event.target);
>+          this._domUtils.setContentState(evInfo.event.target, state ^ 0x00000004);
>+        }

Missed a semicolon.  Ah, so the flag is there to make sure target is deactivated once.  For mousemove event, can we be sure event.target will be the same as mousedown?  Maybe active should be the element instead of the flag (that's what I had done because I wasn't sure).

While we're here, could you also fix select boxes to be the same behavior?  I think all that will be needed is some modified CSS rules.  r- for this reason.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

This patch change the :active target when we start a pan by affecting :active to document.documentElement. I'm doing that because I've spend a few times trying to understand why I can't simply disabled the :active state of the current element before finding that I can't simply disabled it (nsEventStateManager.cpp).

The patch works everywhere we affect :active styling (so it can be used for the highlight/unhighlight behavior madhava want on content) and it shows one of the problem I have found on bug 524495 where panning kill us when we are on a sidebar (try to click on the new tab button and pan over it, the active styling will be dismissed with this patch, which means the click will be prevented : that the default behavior we are doing for a pan).


The only nit of this patch i think is the fact that i move the :active state from the current element to the document.documentElement. 

I'm wondering if i should use a _fake_ element to just receive the :active state instead?

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

This also correct bug 518696

Comment 7

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

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

We'll definitely need to test the affect of moving the :active state to the documentElement. We should test that the state is removed from the documentElement when the mouseup happens.

Comment 8

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

Waiting for post beta 5

Comment 9

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

pushed:
https://hg.mozilla.org/mobile-browser/rev/d36fe965dd06

Comment 10

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b3pre) Gecko/20091111 Firefox/3.6b2pre Fennec/1.0b5

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091111 Firefox/3.7a1pre Fennec/1.0b5


with follow-up bug 527941