Closed Bug 997235 Opened 10 years ago Closed 10 years ago

Followup to bug 986752 - CSS :active states get stuck with multiple on-screen touches

Categories

(Core :: DOM: Core & HTML, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.4+
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: jsmith, Assigned: vingtetun)

References

Details

(Keywords: regression)

Attachments

(2 files)

Followup to bug 986752, since it still doesn't work.
Blocks: 986752
blocking-b2g: --- → 1.4?
Keywords: regression
blocking-b2g: 1.4? → 1.4+
Vivien, is this something you can take?

FWIW, Milan tells me that we can't uplift the patches in bug 976605 for 1.4.
Flags: needinfo?(21)
Seems like this one is a different issue than the one with the dialer keypad. I can reproduce it and I will try to see if I can provide a patch.
Flags: needinfo?(21)
Thanks, Vivien!  To reduce unease, I'm going to set you as the assignee so we don't have it set to nobody; I hope that's okay.
Assignee: nobody → 21
I can reproduce the issue using the latest m-c/Gaia master. Seems like we need to reset a bit more states when a new target is set but there was already one.

I'm still trying to understand what's going on on 1.4, which will likely require a completely different patch :/ 

Is there any reason why we can't uplift the patch that moves from JS to CPP ?
Attachment #8411055 - Flags: review?(botond)
Comment on attachment 8411055 [details] [diff] [review]
bug997235.FFOS.1.5.patch

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

Thanks for the patch!
Attachment #8411055 - Flags: review?(botond) → review+
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #4)
> I'm still trying to understand what's going on on 1.4, which will likely
> require a completely different patch :/ 
> 
> Is there any reason why we can't uplift the patch that moves from JS to CPP ?

The C++ code in ActiveElementManager isn't a line-by-line transcription of the active-element handling code in BrowserElementPanning.js (and can't easily be, as that code is entangled with other code in BEP.js which would also have to be ported over (like _findPannable(), which I think is unnecessary to port as we can get the same info more accurately from APZ)). As a result, there is a greater chance of this patch introducing regressions than the average bugfix patch.

That said, I realize that there is also a cost to having to fix a bug twice in two different places in the code. Let's see what Milan thinks.
Flags: needinfo?(milan)
Line by line explain why I can reproduce the patch on 1.5 with the exact same symptoms than the one in the video. So let's reset more states here as well :)

Justin, can you try this patch on 1.4 and see if it fixes the issue for you ?
Attachment #8411100 - Flags: review?(botond)
Flags: needinfo?(jdarcangelo)
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #7)
> Created attachment 8411100 [details] [diff] [review]
> bug997235.FFOS.1.4.patch
> 
> Line by line explain why I can reproduce the patch on 1.5 with the exact
> same symptoms than the one in the video. So let's reset more states here as
> well :)
> 
> Justin, can you try this patch on 1.4 and see if it fixes the issue for you ?

Fwiw, if we don't reset the states, we trigger the this.notify(this._activationTimer) call in the onTouchEnd method. So the :active state is correctly resetted during the onTouchStart, but it is set again during the onTouchEnd.

I think that's what happen in the video but this is hard to see since Justin's fingers are above the button.
Uplifting BrowserElementPanning.js to C++ patch to code that is 11 weeks apart, and almost beta, is a scary proposition. If it "doesn't break anything" then it's OK - how confident are we that we wouldn't be introducing other issues?  How many other dependencies (hidden or obvious) would we need to pick up?
Flags: needinfo?(milan)
(In reply to Milan Sreckovic [:milan] from comment #9)
> Uplifting BrowserElementPanning.js to C++ patch to code that is 11 weeks
> apart, and almost beta, is a scary proposition. If it "doesn't break
> anything" then it's OK - how confident are we that we wouldn't be
> introducing other issues?  How many other dependencies (hidden or obvious)
> would we need to pick up?

Let's see if the small alternate patch for 1.4 fix the issue for justin and if he can find any other regressions.
Attachment #8411100 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #6)
> The C++ code in ActiveElementManager isn't a line-by-line transcription of
> the active-element handling code in BrowserElementPanning.js

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #7)
> Line by line explain why I can reproduce the patch on 1.5 with the exact
> same symptoms than the one in the video.

I said it _wasn't_ a line-by-line transcription. Of course, that doesn't mean I didn't manage to port over some bugs in the original code :)
Vivien: Patch looks good! Tested here http://jsbin.com/vorib and also in the Camera and Clock apps. Two fingers no longer holds the :active state. Let's get this landed!
Flags: needinfo?(jdarcangelo)
(In reply to Botond Ballo [:botond] from comment #11)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> needinfo? please) from comment #7)
> > Line by line explain why I can reproduce the patch on 1.5 with the exact
> > same symptoms than the one in the video.
> 
> I said it _wasn't_ a line-by-line transcription. Of course, that doesn't
> mean I didn't manage to port over some bugs in the original code :)

Oups! I need to focus more when reading comments!
https://hg.mozilla.org/mozilla-central/rev/4554b016a72b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.