Closed Bug 1004516 Opened 10 years ago Closed 10 years ago

Scrolling around in contact list leaves things highlighted

Categories

(Core :: Panning and Zooming, defect)

32 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
b2g-v1.4 --- unaffected
b2g-v2.0 --- affected

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 2 obsolete files)

Running latest master code on my hamachi (also on helix), with the medium reference workload applied. When I scroll around in the contact list, contact items will remain focused/highlighted after I lift my finger. In fact it's quite easy to have it so that multiple items remain highlighted.

I'm filing this in contacts for now because I can't reproduce this in the settings app, for example. There the highlight behaviour seems to work as expected, with the highlight disappearing when I lift my finger.
Actually I'm seeing this in the messages app too, so it's probably a focus issue in gecko?
Component: Gaia::Contacts → Panning and Zooming
Product: Firefox OS → Core
Version: unspecified → 32 Branch
As far as I can tell the ActiveElementManager is behaving fine, but setting focus on the root to clear the focus from a sub-element doesn't seem to be working.
Actually there is also a bug in AEM - mCanBePanSet is is never reset back to false, meaning that multiple mSetActiveTask instances can be scheduled inflight.
Assignee: nobody → bugmail.mozilla
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> setting
> focus on the root to clear the focus from a sub-element doesn't seem to be
> working.

Strange. That is how BEP.js did it: http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js?from=BrowserElementPanning.js#463

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Actually there is also a bug in AEM - mCanBePanSet is is never reset back to
> false, meaning that multiple mSetActiveTask instances can be scheduled
> inflight.

Whoops! Yeah, mCanBePanSet should be cleared in HandleTouchEnd().
The "active" state does in fact seem to be getting cleared from the element, but for some reason the "focus" state is not. The CSS in gaia/shared/style/lists.css applies the same visual effect to :active and :hover so that's why it appears to remain active when in fact it remains hover'd. Looking into why the hover state is not cleared.
Attached patch Part 1 - fix (obsolete) — Splinter Review
Try build including this patch at https://tbpl.mozilla.org/?tree=Try&rev=e90323f27dfe
Attachment #8416074 - Flags: review?(botond)
Attached patch Part 2 - debug logging (obsolete) — Splinter Review
GDB is kinda unreliable, so I found this handy.
Attachment #8416075 - Flags: review?(botond)
Attachment #8416075 - Flags: review?(botond) → review+
Comment on attachment 8416074 [details] [diff] [review]
Part 1 - fix

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

::: gfx/layers/apz/util/ActiveElementManager.cpp
@@ +73,5 @@
>    // Otherwise, wait a bit to see if the user will pan or not.
>    if (!mCanBePan) {
>      SetActive(mTarget);
>    } else {
> +    MOZ_ASSERT(mSetActiveTask == nullptr);

For a given touch-start event, SetTargetElement() and HandleTouchStart() can be called in either order. If the user puts a second finger down, and we call HandleTouchStart() before SetTargetElement(), then:

  - mCanBePanSet will be true because the previous call to HandleTouchStart()
    will have set it but only HandleTouchEnd() clears it

  - mTarget will be true because the first call to SetTargetElement() will have
    set it but only HandleTouchEnd() clears it
  
  - therefore, TriggerElementActivation() will be called

  - mSetActiveTask will be non-nullptr since the call to TriggerElementActivation()
    (by whichever of HandleTouchStart() and SetTargetElement() was called second
    for the first touch) will have set it and only HandlePanStart() and
    HandleTouchEnd() clear it

  - therefore, we trigger this assertion

To work around this, I think we need to add a check in HandleTouchStart() to see if 'mCanBePanSet' is already set (much like how we check 'mTarget' in SetTargetElement(), and do the same {multiple fingers on screen}-handling logic that we do in SetTargetElement().
Comment on attachment 8416074 [details] [diff] [review]
Part 1 - fix

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

Minusing as per above comment.
Attachment #8416074 - Flags: review?(botond) → review-
Updated as requested. I could have pulled out another helper method for the duplicated code between the two functions but I got lazy. Let me know if you'd rather I pulled it out.
Attachment #8416074 - Attachment is obsolete: true
Attachment #8416176 - Flags: review?(botond)
Attached patch Part 2 - loggingSplinter Review
Rebased and adjusted slightly to take into account the new part 1. Carrying r+.

Try push with the new patch at https://tbpl.mozilla.org/?tree=Try&rev=bbd0c755570a
Attachment #8416075 - Attachment is obsolete: true
Attachment #8416177 - Flags: review+
Attachment #8416176 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/cc29ea949039
https://hg.mozilla.org/mozilla-central/rev/cc840267a394
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1169802
You need to log in before you can comment on or make changes to this bug.