Closed Bug 525997 Opened 10 years ago Closed 10 years ago

adjust row highlighting behaviour

Categories

(Firefox for Android Graveyard :: General, defect)

Fennec 1.1
ARM
Maemo
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: madhava, Assigned: vingtetun)

Details

(Whiteboard: [polish])

Attachments

(1 file, 3 obsolete files)

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?
tracking-fennec: --- → ?
Whiteboard: [polish]
Attached patch wip (obsolete) — Splinter Review
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.
Attached patch WIP-2 (obsolete) — Splinter Review
Wip using the inIDOMUtils interface to kill the :active style on a pan
Attachment #409912 - Attachment is obsolete: true
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.
Attachment #409919 - Flags: review-
Attached patch Patch (obsolete) — Splinter Review
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?
Assignee: nobody → 21
Attachment #409919 - Attachment is obsolete: true
Attached patch PatchSplinter Review
Attachment #410217 - Attachment is obsolete: true
Attachment #410271 - Flags: review?(mark.finkle)
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.
Attachment #410271 - Flags: review?(mark.finkle) → review+
Waiting for post beta 5
pushed:
https://hg.mozilla.org/mobile-browser/rev/d36fe965dd06
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Component: Linux/Maemo → General
OS: Mac OS X → Linux (embedded)
QA Contact: maemo-linux → general
Hardware: x86 → ARM
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.