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)
Tracking
()
People
(Reporter: jsmith, Assigned: vingtetun)
References
Details
(Keywords: regression)
Attachments
(2 files)
883 bytes,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
1012 bytes,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
Followup to bug 986752, since it still doesn't work.
Reporter | ||
Updated•10 years ago
|
blocking-b2g: --- → 1.4?
Keywords: regression
Updated•10 years ago
|
blocking-b2g: 1.4? → 1.4+
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
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
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Comment 6•10 years ago
|
||
(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)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8411100 -
Flags: review?(botond) → review+
Comment 11•10 years ago
|
||
(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 :)
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
(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!
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4554b016a72b
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4554b016a72b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 16•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/5973192ff096
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
status-firefox29:
--- → wontfix
status-firefox30:
--- → fixed
status-firefox31:
--- → fixed
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•