User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_3) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.56 Safari/536.5 Steps to reproduce: Go to any app with several (big) input fields on the screen and try to scroll down Actual results: As soon as you click inside an input (event you were trying to scroll the app down) the Keyboard is shown Expected results: The keyboard shouldn't be shown. It should only when you *tap* in an input field.

Blocking+, Kaze confirmed happens in Settings too. Should a Keyboard dev be the one to fix this? Or is this in the input widgetry?

I would say is a platform issue. The keyboard is shown every time System receives a onfocuschange event, so the idea should be doesn't dispatch that event when detecting that is scrolling (even over an input)

See also https://github.com/mozilla-b2g/gaia/issues/3399.

This sounds a lot like https://github.com/mozilla-b2g/gaia/issues/3399 and I thought we had another platform bug for it but can't find it at the moment. Doug suggested we CC Matt and Wesley to see how Fennec handles this.

Ehsan, do you have time to take a quick look here? Am I crazy to ask you and there's someone who'd be a better owner? :)

Sorry I don't have time right now to investigate this. You need to ask someone who knows the IME interface that b2g uses, I think.

Tim, can you take a look here? Maybe Rudy can help?

(In reply to Andrew Overholt [:overholt] from comment #7) > Tim, can you take a look here? Maybe Rudy can help? Rudy can look into this first, he knows the current version of mozKeyboard than I am. If he cannot do it then I could try.

Rudy, can you give us an update here? Thanks.

Also blocks https://github.com/mozilla-b2g/gaia/issues/2213

I was told that this issue would be fixed with the following bug, so I did not dig into this issue, https://bugzilla.mozilla.org/show_bug.cgi?id=787549, Bug 787549 - B2G: Stop simulating mouse events unless there's a tap Right now, I have seen that this issue would not occur in browser app. but would still occur in other apps like contacts. Call for help or any clues on this since I am kind-of trapped in settings app. "feature" development. Thanks.

blassey asked me to investigate this bug.

Just provide my observation: bug 787594 only take effect on browser app because AsyncPanZoomController is only enabled for browser app.

STR: 1. Add a new contact 2. The keyboard is initially closed on the "Add contact" screen 3. Try to scroll down with a vertical swipe whose initial touch down is in a text field (e.g. "Comment" field) RESULT: The keyboard opens and sets the input focus to the "Comment" field.

Marking for C2, given this meets the criteria of known P1/P2 blocking-basecamp+ bugs at the end of C1.

Our touch code seems to interpret a finger on the screen as button press immediately, without the delay that iOS and Android do, potentially causing this and other issues (eg: scrolling in Settings triggers visual hit state). A slight delay in showing button hit state is preferable to triggering hits erroneously.

bug 804315 is related.

I have a potential fix. Unfortunately, I need to investigate why it works on the B2G desktop build but not my phone..

Hi Chris, any update here?

I have a patch, but I am debugging a regression I found. I hope to post my patch very soon.

Part 1: Don't open the keyboard when the user touches an input element when scrolling. Previously, we would open the keyboard on input focus. This is incredibly annoying when the user is just trying to scroll the page. With my change: 1. On focus, wait for mousemove (user is scrolling) or mouseup (user is tapping). 2. On mousemove, don't open the keyboard but listen for mousedown in case the user taps the same input element (because we won't get another focus event for this element). 3. On mouseup, open the keyboard (and listen for mousedown when the user is selecting text). Is the state flow I described above clear in my patch itself? Or should I add a bigger comment describing the state changes? Is there a simpler why to represent this state flow without adding and removing so many event listeners? I also removed the code that observes the ime-enabled-state-changed event. Gecko was firing this event for some internal IME state changes and causing us to prematurely close the keyboard. My change handles all the opening and closing of the keyboard in handleEvent().

Part 2: Don't close the keyboard when the user scrolls the page. When the keyboard is open and the user touches outside an input element (either the currently focused element or another) to scroll the page, the keyboard will close. This is annoying, particularly if the input element is a large textarea. Refocusing the input element to re-open the keyboard will cause the page to scroll (because we scrollIntoView() the focused input element when the keyboard opens). With my change, the keyboard will close if the user taps (blur then mouseup) outside the focused input element. If the user scrolls (blur then mousemove), then the page can scroll without closing the keyboard. Android and iOS implement slightly different behavior. They do not close if the user taps or scrolls outside the focused input element. To close the keyboard, the user must use some other UI control (such as Android's hardware Back button).

Comment on attachment 686700 [details] [diff] [review] part-1-dont-open-when-scrolling.patch Review of attachment 686700 [details] [diff] [review]: ----------------------------------------------------------------- I've put some comments in the code, but am giving an overall r-. My reasons: 1) This feels like the wrong place to be fixing the bug. I don't like that we're trying to override gecko's focus assignment here. Instead, let's just fix gecko so that it doesn't send focus events when the user is actually scrolling. Do we have the same bug in Fennec? If not, what do we do there that we can do here? 2) It looks like this patch can reassign focus to an element even as the user is scrolling away from that element. This could lead (I think) to a case where they keyboard is up, but the focused element is off the screen. That's probably a separate bug that we'll have to fix in gecko when we distinguish scrolling from focusing. 3) The event handling logic is so complex that it will be hard to really be confident that it is always right. (I have not actually taken the time to do that). If anything weird were to ever happen with the event sequence, though, I think the code could get into a broken state that would require a b2g reboot to recover from. What happens, for example, if the user touches the screen and then moves their finger off the screen without lifting it. I think this is supposed to cause a touchstart/touchmove/touchcancel sequence. How does that translate into mouse events? Will we get a mousedown/mouseup sequence? If for some reason the mouseup doesn't arrive, the event handler state will probably get messed up. If it is too late to fix this bug correctly, and we really do need a focus hack like this in forms.js, then in order for me to be comfortable giving it an r+, I'd need more documentation and the event handlers broken out into separate functions rather than all going through handleEvent. In complex situations like this, I find it helpful to think about finite state machines. What are the states (no focus, gecko has assigned focus but we don't believe it yet, we agree with gecko about who has focus, etc.) and what are the transitions (focus event, mouse up, etc) between those states? And, if we have to fix it here, I'd like to explore simpler options. What if we just set a timer on the focus event and if there was a scroll event soon after then we ignore the focus? Would that acheive the same result with much less complexity? ::: b2g/chrome/content/forms.js @@ -97,5 @@ > > if (target && this.isFocusableElement(target)) { > if (this.blurTimeout) { > this.blurTimeout = content.clearTimeout(this.blurTimeout); > - this.handleIMEStateDisabled(); Thanks for removing that line. It doesn't make sense to defer the blur if we're just going to hide the keyboard here anyway. @@ +91,5 @@ > } > + // User touched an input element. Ignore the focus change if the > + // user scrolls (mousemove) instead of tapping (mouseup). > + target.addEventListener("mousemove", this); > + target.addEventListener("mouseup", this); All the adding and removing of event handlers would be easier to understand if they were implemented as individual methods rather than all going through this one handleEvent method. That would even allow you to have two different sets of handlers. One dealing with the focus stuff and another dealing with the click-to-move-the-cursor stuff. @@ +119,5 @@ > + target.addEventListener("mousemove", this); > + } > + break; > + > + case "mousemove": Don't you need to have some kind of threshold detection here? If I'm understanding correctly, the only way to focus now would be to have an absolutely still tap, with no mousemove events between the mousedown and mouseup. It seems to me that that would be hard to do. @@ -205,5 @@ > }, > > observe: function fa_observe(subject, topic, data) { > switch (topic) { > - case "ime-enabled-state-changed": I've never understood what this was for, so I can't comment on its removal. But if it works without it, I think its great to get rid of it.

Comment on attachment 686717 [details] [diff] [review] part-2-dont-close-when-scrolling.patch Review of attachment 686717 [details] [diff] [review]: ----------------------------------------------------------------- r- for the same reasons as part 1. But it does seem like the "close the keyboard when we are existing to the homescreen" patch might be worth landing as a bug fix of its own. ::: b2g/chrome/content/forms.js @@ +98,5 @@ > > case "blur": > if (this.focusedElement) { > + // Close the keyboard when we are exiting to the home screen. > + if (!this.isFocusableElement(target)) { It seems like this is a fix to a separate bug. I don't understand how this condition detects "exiting to the home screen", but would be willing to r+ this fix for a separate bug if you can explain to me how it works. It seems like this piece is worth landing soon even if we can't figure out the overall scrolling thing. @@ +119,5 @@ > + self.handleIMEStateDisabled(); > + } > + }; > + addEventListener("mousemove", onBlurMouse); > + addEventListener("mouseup", onBlurMouse); I'm not convinced that deferring the registration of these events handlers is the right thing to do here. The point of this was to avoid closing the keyboard and opening it again when we transfer focus from one element another and get blur and focus events in quick succession. But if you defer the event handler registration, then maybe you miss the mouse events you're interested in.

Thanks for the feedback. I agree that this proposed change was uncomfortably complicated. I'll investigate whether I can fix this within Gecko. Firefox on Android does not have the problem where the keyboard will open when scrolling a page.

Does the change that just landed for #774458 help with this bug? If not, can we do something similar for focus event to what that patch did for click events?

Comment on attachment 686717 [details] [diff] [review] part-2-dont-close-when-scrolling.patch Review of attachment 686717 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Chris Peterson (:cpeterson) from comment #27) > Thanks for the feedback. I agree that this proposed change was uncomfortably > complicated. > > I'll investigate whether I can fix this within Gecko. Firefox on Android > does not have the problem where the keyboard will open when scrolling a page. Can you fix that by looking at dom/browser-element/BrowserElementScrolling.js ? The code path for web content and web applications is different when it comes to scrolling so fixing that at the 'gecko' level will involve hacking BrowserElementScrolling.js

Drive-by: I don't think we should even be thinking about uplifting a patch that changes how focus works in Gecko. I'd suggest taking the a fix in the b2g frontend now and filing a follow up to make platform changes so we can get a full test cycle.

I have started a wip that delay a little bit the stealing the focus. It still needs some love (text selection is broken right now) but it seems to perform OK in the contacts app.

vingtetun's WIP patch did not work for me. I was getting some JS errors (about missing element.focus property?), causing all element clicking to break.

Part 1: Remove "ime-enabled-state-changed" observer. The "ime-enabled-state-changed" event seems to fire often and redundantly. It causes us to close the keyboard when we don't need to. It is not necessary for tracking the keyboard state.

Because forms.js' keyboard code is tricky, I split this patch series into small steps that should be easier to review. This patch series fixes the reported bug, but makes some existing UI problems more visible. I will file follow-up bugs for these issues: * Keyboard flickers open and close when tapping focus from one input element to another. This problem had previously been addressed by forms.js' blurTimeout. * When the keyboard is open, touching outside the focused input element (i.e. touching another input element or the page background) to scroll the page will close the keyboard. * When tapping an input element that is "above the fold" of where the keyboard will be, the element is unnecessarily scrollIntoView()'d. This causes the page jump around, which looks bad.

Part 2: Handle element focus and IME state separately. This patch is a small refactoring that will be used in patch 3. It disentangles setFocusedElement() from handleIMEStateEnabled() and handleIMEStateDisabled().

Part 3: Open keyboard when user taps input element, not when panning. An input element will receive a 'focus' event when the user touches it to pan the page. It is valid for an element to have focus when the virtual keyboard is not open because the user might be using a hardware keyboard (e.g. B2G desktop build). This patch remembers the focused element in the 'focus' event handler, but only opens the keyboard if the user is tapping the element. If the user taps without dragging, Gecko will fire a 'click' event.

I glanced at this during a long build today. As an alternative Chris, you could try something like: 1.) Turn off normal CP event dispatch for touch events: http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#1546 2.) Instead start sending touch events from the AsyncPanZoomController (this is very similar to what Fennec does), somewhere around here: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#276 (same Fennec code is at: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/TouchEventHandler.java#205) 3.) Don't dispatch cross process touch/mouse events if the AsyncPanZoomController is in any panning/flinging/zooming states? Fennec at that point also sends a fake touchend event when we start panning so that content doesn't get confused. This may also break some B2G apps that depend on getting touch events, even if they're scrolling. I have no idea if any of those exist....

Comment on attachment 689997 [details] [diff] [review] part-1-remove-ime-state-observer.patch Review of attachment 689997 [details] [diff] [review]: ----------------------------------------------------------------- I don't have the slightest idea what the ime-enabled-state-changed event is for, so I can't r+ code that removes it, unless you can explain to me what it does and convince me that we don't need it here. I'm not saying that the patch is bad, just that I'm not really qualified to review it. I see it used in http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#463 and http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#852 You said that this event is redundant. Could we use it instead of the other touch and focus event stuff we have in forms.js? That is could we throw out the other stuff and just use this? The fact that it is used by TabParent makes me wonder about the case where you focus an element in App1, then hit the home button to go back to the homescreen and use some other app. Then, you switch back to app 1. Does the keyboard open back up so you can pick up where you left off? Does ime-enabled-state-changed have anything to do with that? Is this something that predates b2g and is used for Asian input methods, or is it something that someone hacked up for b2g? I'm not qualified to review this. We need someone who understands how events work in gecko. I don't. I'm going to ask Viven for additional review, because he's the only person I can think of. But maybe Vivien can at least tell us who would understand ime-enabled-state-changed.

Comment on attachment 690002 [details] [diff] [review] part-3-keyboard-on-click.patch Review of attachment 690002 [details] [diff] [review]: ----------------------------------------------------------------- This is so much simpler than the last version! All my concerns about code brittleness and verifiability are gone. I still question the overall approach, however. Wouldn't the ideal approach be that we just didn't get a focus event when the user was trying to pan? Why can't we do it that way? Is the problem just that it is too late to do that in code that has to land in mozilla-beta? Are we at the stage where we have to resort to injecting code into content to fix fundamental flaws in the events that gecko is sending us? Here's my big concern: won't this patch break any web content that manages its own focus? If a site uses JS to set the focus when you click a button, for example, won't it fail with this patch? (I don't actually know whether it would have worked without this patch.) This patch is one that I'm qualified to review in the sense that I can examine the code and make sure that it does what it intends to do. But I don't have enough of the big picture to know if what it intends to do is the right thing to do. Somewhere in Gecko is code that generates focus events. Are there different versions of that code for touch-based platforms vs mouse-based platforms? Somewhere at Mozilla are the engineers who understand that code, who can answer questions like these and who would be a better reviewer for these patches. Is anyone like that cc'ed on this bug? Its pretty late in the game for us to be farting around with something as fundamental as focus events. Chris: if you can make me understand that this is the right or the only thing we can do, I can review this. Or, you could find someone who understands how events work in gecko and get them to do the review and not have to educate me :-)

Comment on attachment 689999 [details] [diff] [review] part-2-separate-focus-and-ime.patch Review of attachment 689999 [details] [diff] [review]: ----------------------------------------------------------------- This seems straightforward, but since I've responded to the other two parts with a bunch of questions, I'm going to hold off reviewing this in any detail until I've got answers on the other parts.

Comment on attachment 689997 [details] [diff] [review] part-1-remove-ime-state-observer.patch Review of attachment 689997 [details] [diff] [review]: ----------------------------------------------------------------- You are correct in that these notifications are somewhat redundant as they fire before focus/blur events and would make the code definitely a little easier. I think we still *might* need them because of this: https://developer.mozilla.org/en-US/docs/CSS/ime-mode The web site/app can thus control IME state with CSS properties and toggle them using JavaScript. I don't really understand if watching this property on B2G make sense at all. IME on B2G is not an "extension for missing keys on the keyboard" but rather the whole keyboard itself, no? So maybe we can remove this completely because we kind of always support and enable IME on keyboard layouts that support/need it. I think I would r+ this if Vivien confirms my assumptions.

Comment on attachment 689999 [details] [diff] [review] part-2-separate-focus-and-ime.patch Review of attachment 689999 [details] [diff] [review]: ----------------------------------------------------------------- I agree with David here. This part looks good but it's of course dependent on the first part.

I applied your three patches and tried them. (In reply to Chris Peterson (:cpeterson) from comment #36) > * Keyboard flickers open and close when tapping focus from one input element > to another. The keyboard still flickers for me. > * When the keyboard is open, touching outside the focused input element > (i.e. touching another input element or the page background) to scroll the > page will close the keyboard. Still not fixed here. > * When tapping an input element that is "above the fold" of where the > keyboard will be, the element is unnecessarily scrollIntoView()'d. This > causes the page jump around, which looks bad. Still happens.

Tim, Chris says that those were pre-existing problems and that he's filed followup bugs to fix them. So the intent was that they would still happen. Chris, I'm in the same timezone as you and I don't have any C2 bugs of my own, so I'm available to help get something landed today.

(In reply to David Flanagan [:djf] from comment #47) > Chris says that those were pre-existing problems and that he's filed > followup bugs to fix them. So the intent was that they would still happen. Oh, I completely misread that. Sorry, please disregard my whole comment :|

looks like this is ready to land then given the last few comments. can we get this out of the way? this was a C2 item which we're running a day behind on it... :cpeterson is still the assignee. chris?

Part 1: Postpone focus of input elements until we know user is tapping, not panning. In BrowserElementScrolling.js, postpone focus of input elements from mousedown to mouseup, where we will know whether the user is single-tapping or panning. There might be a regression risk because the timing of the focus event (in relation to mousedown and mouseup events) may be different. The isFocusableElement() function is lifted from b2g/chrome/content/forms.js. Patch 2 will remove that function from forms.js, so this code won't be duplicated.

Part 2: forms.js no longer needs to listen for focus events because the "ime-enabled-state-changed" event is now fired on single-tap, not immediately on mousedown. Because the focus event handler is no longer needed, I don't think isFocusableElement() or isIMEDisabled() are needed either. In patch 1, BrowserElementScrolling.js is checking isFocusableElement() and should only be raising "ime-enabled-state-changed" for focusable input elements.

(In reply to Faramarz (:faramarz) from comment #49) > looks like this is ready to land then given the last few comments. can we > get this out of the way? this was a C2 item which we're running a day > behind on it... faramarz, I just posted a revised patch based on review feedback from ttaubert, djf, and schien. I am just waiting for review.

Comment on attachment 691176 [details] [diff] [review] part-1-postpone-input-focus.patch >diff --git a/dom/browser-element/BrowserElementScrolling.js b/dom/browser-element/BrowserElementScrolling.js >+ ignoredInputTypes: new Set([ >+ 'button', 'file', 'checkbox', 'radio', 'reset', 'submit', 'image' >+ ]), >+ >+ isFocusableElement: function cp_isFocusableElement(element) { We have pretty much the same code in forms.js, right? It would be nice to share that, maybe with a platform patch. It would be annoying for the logic to get out of sync and this bug pop back up. Can investigate in a followup.

Comment on attachment 691176 [details] [diff] [review] part-1-postpone-input-focus.patch Review of attachment 691176 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me and does fix the bug described here but we need to take care of the problem described in bug 819595 as well. So we could always call .preventDefault() on mousedown and then .focus() on mouseup when we're not panning. The isFocusableElement() check would then need to stay in forms.js. This way we leave the keyboard open when scrolling even if we're not touching a focusable element. Bug 819593 would be fixed with that solution as well. The only problem we currently have is that we can't actually use .focus() here because that for example focuses the keyboard when touching a key and makes it disappear. The same things happens when we end scrolling while the keyboard is shown with the finger above the keyboard.

(In reply to Tim Taubert [:ttaubert] from comment #54) > The same things happens when we end scrolling while the > keyboard is shown with the finger above the keyboard. We can prevent this by only focusing evt.target if the mousedown target is the same as the mouseup target.

(In reply to Tim Taubert [:ttaubert] from comment #54) > The only problem we currently have is that we can't actually use .focus() > here because that for example focuses the keyboard when touching a key and > makes it disappear. On mouseup, we only want to focus elements that would have been focused normally. This means we actually need to find out if BES called .preventDefault() or another part of the code. If it was another part we don't want to call .focus().

Sorry to keep bombarding you with patch revisions! This touch focus code is quite tricky.. <:) This patch implements ttaubert's suggestions from comments 54 and 56. In BrowserElementScrolling.js: 1. On mousedown/touchstart, *always* call preventDefault() to stop Gecko from automatically focusing the element (even for non-input elements). 2. On mouseup/touchend, compare the mouseup and (most recent) mousedown targets before focusing. This change fixes bug 819593 (keyboard closes when touching outside focused element to scroll page) and bug 819595 (keyboard flickers when switching input elements).

Comment on attachment 691564 [details] [diff] [review] postpone-input-focus-v4.patch Review of attachment 691564 [details] [diff] [review]: ----------------------------------------------------------------- PreventDefault() on every touch start event is harmful for webapps since all mousedown/click event will disappear and there is no APZC to fire additional TapEvents. I tried this patch on phone and found that I can't even click the 'Next' button on FTU. If we are going to solve this bug be prevent default action on touch start, then we'll have to simulate all the other action of nsFrame::HandlePress in BES. I don't think this is an easy way IMO.

(In reply to Shih-Chiang Chien [:schien] from comment #58) > If we are going to solve this bug be prevent default action on touch start, s/be prevent/by preventing/

Comment on attachment 691564 [details] [diff] [review] postpone-input-focus-v4.patch Oh, good catch! I forgot about this.

(In reply to Shih-Chiang Chien [:schien] from comment #58) > PreventDefault() on every touch start event is harmful for webapps since all > mousedown/click event will disappear and there is no APZC to fire additional > TapEvents. > I tried this patch on phone and found that I can't even click the 'Next' > button on FTU. > If we are going to solve this bug be prevent default action on touch start, > then we'll have to simulate all the other action of nsFrame::HandlePress in > BES. I don't think this is an easy way IMO. That is very strange. I tested my BES fix on my Unagi device and I did not have any FTU problems. schien, are you suggesting that BES mousedown should *never* call preventDefault()? Or that it should not call preventDefault() for elements that are not focusable? If you don't think BES is an appropriate place to fix this bug, do you have any other suggestions? I'm not sure where else to look..

As we're on a touch device there actually are no mousedown/up and click events, right? So when receiving a "touchclick" action we'd need to emulate those legacy events as described here http://www.quirksmode.org/mobile/advisoryTouch.html#link6 If I understand this correctly, when (event.mozInputSource == MOZ_SOURCE_TOUCH) we should block all mouse* events and simulate them if needed. I *think* that's the only way to solve all these various problems we currently have with the keyboard, but I might be missing something here.

(In reply to Chris Peterson (:cpeterson) from comment #61) > That is very strange. I tested my BES fix on my Unagi device and I did not > have any FTU problems. I re-apply your patch again and it is still not working correctly. :( gecko version is m-c: 115789:fd919eb97465 gaia version is ba1f14083f36500cd49397d83e8ddd111684764a I updated my codebase this afternoon. > > schien, are you suggesting that BES mousedown should *never* call > preventDefault()? Or that it should not call preventDefault() for elements > that are not focusable? Actually, your modification in BES will preventDefault() on all touchstart event, and gecko will not synthesize mouse event on a preventDefault-ed touch event. So, it will eventually break lots of web pages and apps since most of them are still using mouse event. > > If you don't think BES is an appropriate place to fix this bug, do you have > any other suggestions? I'm not sure where else to look.. I think your idea of delaying the focus until touch end is correct, however, doing it in BES means you need to take care of everything done by nsFrame::HandlePress(). My suggestion is to implement this logic in layout/generic/nsFrame.cpp. @cjones, have any suggestion?

We need to land a fix early this week - this is one of our longest standing bugs, and represents a non-zero amount of risk (needs significant testing given the back and forth here in comments). Marking needinfo for cjones given comment 63. If we don't need to block on cjones, please don't.

(In reply to Alex Keybl [:akeybl] from comment #64) > We need to land a fix early this week - this is one of our longest standing > bugs, and represents a non-zero amount of risk (needs significant testing > given the back and forth here in comments). I am still investigating a core Gecko fix for this bug, but I don't have an ETA for that fix. However, I think the patches submitted in comment 35, comment 37, and comment 38 could be a reasonable band-aid fix. Those patches are pretty well isolated. They only change B2G JS code related to IME. djf and ttaubert commented on those patches in comment 40 through 48.

(In reply to Chris Peterson (:cpeterson) from comment #65) > (In reply to Alex Keybl [:akeybl] from comment #64) > > We need to land a fix early this week - this is one of our longest standing > > bugs, and represents a non-zero amount of risk (needs significant testing > > given the back and forth here in comments). > > I am still investigating a core Gecko fix for this bug, but I don't have an > ETA for that fix. > > However, I think the patches submitted in comment 35, comment 37, and > comment 38 could be a reasonable band-aid fix. Those patches are pretty well > isolated. They only change B2G JS code related to IME. I do agree with you. > djf and ttaubert commented on those patches in comment 40 through 48.

cjones says this bug is probably related to sync vs async scrolling. I created a simple test page with a text input element. I see now that part of the problem is that Gecko is sending the touch and mouse events in a *different order* if my test page is viewed in the B2G browser compared to when it is run as a webapp. * If I touch the input element and drag in the B2G browser, Gecko fires these events: touchstart, touchend. * If I touch and drag when the page is launched as a webapp (bookmark saved to Home Screen), Gecko fires: touchstart, mousedown, focus (which opens the keyboard), touchend, mouseup, click

Let me know if we're blocked on me.

Driver retriage: Blocking on the band-aid fix mentioned in comment #65 only. Can you land those patches, and file a followup for a more comprehensive Gecko-level fix?

The bandaid fix patches have been obsoleted so they're not there for me to look at again. If you put them back up, Chris, I'll review one more time. As I recall, the fixes seemed solid to me, I just thought they tackled the problem at the wrong spot, so, now that we're up against the wall, I can probably give a quick r+. Though if you can find someone else to review on short notice, having one more pair of eyes on this critical bit of code would be helpful.

(In reply to David Flanagan [:djf] from comment #70) > The bandaid fix patches have been obsoleted so they're not there for me to > look at again. If you click "Show Obsolete" at the bottom of the attachments list, then click "Details" for the patch, you can click "Edit Details" on the attachment details page, then uncheck the obsolete checkbox. Bugzilla attachments never die! :)

The problem is that Gecko synthesizes mouse events from touch events in a different order for synchronous scrolling content (like Gaia and webapps) than asynchronous scrolling content (like web content in the B2G browser). Sync scrolling content receives mouse events in a desktop-like order: touchstart, MOUSEDOWN, focus, touchend, MOUSEUP, click Async scrolling content receives mouse events in the standard mobile browser order [1]: touchstart, touchend, MOUSEMOVE, MOUSEDOWN, focus, MOUSEUP, click This difference causes bugs when web content is run as a B2G webapp or when Gaia developers write code that expects mouse events to work like they do in mobile browsers (including the B2G browser). For sync scrolling content, TabChild synthesizes [2] a mousedown event immediately after touchstart. The mousedown can cause Gecko to inadvertently focus the input element (and open the keyboard) when the user is simply panning the page. To match the mouse event order used by mobile browers, we should only synthesize mousemove, mousedown, and mouseup in response to single-tap (like TabChild::RecvHandleSingleTap() [3] does for async scrolling content in the browser). Unfortunately, when I implement that behavior in TabChild::DispatchSynthesizedMouseEvents(), most of the Gaia UI breaks because much of it relies on mouse events (delivered in a desktop-like order) instead of touch events. I have a passable workaround (comment 65) in the B2G keyboard code, but I expect the mousedown events will cause compatibility problems for third-party webapps. The "Right Thing" fix would rewrite the Gaia UI to use touch events instead of mouse events, but I don't think we dare do that for B2G 1.0. [1] https://developer.apple.com/library/safari/#documentation/AppleApplications/Reference/SafariWebContent/HandlingEvents/HandlingEvents.html [2] https://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1353 [3] https://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1276

I will rebase and post my band-aid patches momentarily.

Part 1: Remove "ime-enabled-state-changed" observer. If we fix the real problem for sync scrolling in TabChild.cpp, these band-aid patches should be reverted! The "ime-enabled-state-changed" event will then be accurate.

Part 2: Handle element focus and IME state separately.

Part 3: Open keyboard when user taps input element, not when panning.

cpeterson, can you post your "real" fix for this bug here? We're dealing with some similar problems with bug 814252. We may just have to bite the bullet here :/.

cpeterson: I'm going to r+ all three of your bandaid patches, in case you and cjones decide to go that route. Note, however: 1) patches 2 and 3 conflict with the patches I just reviewed for bug 818893, so depending on which one lands first, someone will have a merge conflict to resolve 2) forms.js is a file of suck. You have the opportunity in this bug to improve it somewhat, and I'll be including some suggestions in one of my review comments below.

Comment on attachment 693515 [details] [diff] [review] part-2-separate-focus-and-ime-v2.patch Review of attachment 693515 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/forms.js @@ +216,5 @@ > > return disabled; > }, > > + handleIMEStateEnabled: function fa_handleIMEStateEnabled() { This file would be clearer if we renamed this function to something obvious like "showKeyboard" And then you could rename the tryShowIME() function it calls to something like sendKeyboardState() or something @@ +229,2 @@ > > handleIMEStateDisabled: function fa_handleIMEStateDisabled() { And this one could be "hideKeyboard"

Comment on attachment 693516 [details] [diff] [review] part-3-keyboard-on-click-v2.patch Review of attachment 693516 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/forms.js @@ +127,5 @@ > this.tryShowIme(this.focusedElement); > } > break; > > + case 'click': I don't like that we use this one handleEvent() function for handlers registered on different events. I'd prefer it if the click and mouseup and mousedown handlers were separate functions, or were a separate object, so that we could keep distinct which events were happening on the whole window and which were just on the currently focused element. But, failing that, you could at least add a comment before this click case reminding readers that this is just for clicks on the currently focused element. And, on that note, is it ever possible for this to be triggered when this.focusedElement is not defined? Do you really need the test below?

I renamed the functions as you suggested: s/handleIMEStateEnabled/showKeyboard/ s/handleIMEStateDisabed/hideKeyboard/ s/tryShowIME/sendKeyboardState/ (In reply to David Flanagan [:djf] from comment #80) > But, failing that, you could at least add a comment before this click case > reminding readers that this is just for clicks on the currently focused > element. I'll add a comment. I agree that separately the event handlers is a good idea, but I'm nervous about changing too much code in these band-aid patches. If these patches get reverted because we can implement the "real" touch event fix in TabChild.cpp, then we don't want to revert these suggested cleanups, too. (I isolated the function renames in a separate patch for this same reason.) > And, on that note, is it ever possible for this to be triggered when > this.focusedElement is not defined? Do you really need the test below? I'm not sure. this.focusedElement is set to null when forms.js receives a "blur" event from Gecko or a"Forms:Select:Blur" message from the Keyboard app's MozKeyboard.js. Our "mouseup" handler assumes this.focusedElement is not null. Since the "click" event is dispatched immediately (?) after "mouseup", it is probably safe to assume we won't receive a "blur" event or "Forms:Select:Blur" message between "mouseup" and "click".

WIP! Only synthesize mouse events for tap gesture. Previously, TabChild synthesized mousedown after touchstart and mousemove after touchmove. With this patch, TabChild synthesizes mousemove+mousedown+mouseup after touchstart+touchup (without a touchmove), i.e. a "tap" gesture. NB: This patch breaks parts of the Gaia UI that depend on panning, swiping, and dragging. For example, you can run apps on the home screen, but you can't swipe to Everything.me or the other app screens.

(In reply to Chris Peterson (:cpeterson) from comment #82) > NB: This patch breaks parts of the Gaia UI that depend on panning, swiping, > and dragging. For example, you can run apps on the home screen, but you > can't swipe to Everything.me or the other app screens. Which is I *think* actually the right way. We're on a touch platform and should listen to touch events. Legacy mouse events should only be emulated for touchclick actions. Not to say we're pretty late in the game :)

If you decide to pull the trigger on this and fix it the right way, I can help with the conversion to touch events. Chris: do you allow small motions between the touchstart and touchend events to still be interpreted as a tap, or does the tap have to be perfectly still? It seems to me that there ought to be some sort of movement threshold that is ignored. Does the system provide that? And do you have any time threshold? Does the touchend have to occur soon after the touch start to count as a tap?

(In reply to David Flanagan [:djf] from comment #84) > If you decide to pull the trigger on this and fix it the right way, I can > help with the conversion to touch events. Thanks. I'm investigating how much work this would be. Part of the difficulty is maintaining the B2G desktop build, which generates real mouse events and no touch events. Do we want the B2G desktop build to translate its mouse events to fake touch events (which will be translated into synthetic mouse events by Gecko)? Or do we want to maintain support for *both* mouse and touch events through out Gaia? Both are ugly options. Gaia has about 110 addEventListener() calls for mouse events and 38 calls for touch events. > Chris: do you allow small motions between the touchstart and touchend events > to still be interpreted as a tap, or does the tap have to be perfectly > still? It seems to me that there ought to be some sort of movement > threshold that is ignored. Does the system provide that? And do you have > any time threshold? Does the touchend have to occur soon after the touch > start to count as a tap? I did not add any movement or time thresholds. Clicking on my Unagi is not very difficult, so I assume Gecko or the drivers have some touch heuristics. It would be very easy to add and tune thresholds later.

Here is a WIP patch that ports Gaia's lockscreen and homescreen from mouse to touch events. With the TabChild patch, this actually works. :D My port changes (in pseudo-regular expressions): s/mousedown/touchstart/ s/mousemove/touchmove/ s/mouseup/touchend/ s/evt.[client|page|screen][X|Y]/evt.changedTouches[0].[client|page|screen][X|Y]/ s/evt.target/evt.changedTouches[0].target/ The changedTouches[0] might not work as expected in cases where the user is using multitouch. In some cases, we might need to track changedTouches[0].identifier.

Chris, what bugs will we *not* be able to fix we *don't* make the full conversion? That's the other side of the tradeoff we need to measure. This approach isn't quite enough to make the right conversion, so that's bad :(.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #87) > Chris, what bugs will we *not* be able to fix we *don't* make the full > conversion? That's the other side of the tradeoff we need to measure. cjones, off the top of my head, we probably can't (cleanly) fix bug 819593 or bug 819595. They are annoying keyboard UX bugs, but not critical. > This approach isn't quite enough to make the right conversion, so that's bad What other changes would we need to make?

(In reply to Chris Peterson (:cpeterson) from comment #88) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #87) > > Chris, what bugs will we *not* be able to fix we *don't* make the full > > conversion? That's the other side of the tradeoff we need to measure. > > cjones, off the top of my head, we probably can't (cleanly) fix bug 819593 > or bug 819595. They are annoying keyboard UX bugs, but not critical. Is it possible to fix them uncleanly? Also, only bug 819595 is a blocker. > > This approach isn't quite enough to make the right conversion, so that's bad > > What other changes would we need to make? We need to preventDefault() on touchstart, track the first touch ID, only respond to touchmove and touchend for that ID, and fall back on mouse events.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #89) > We need to preventDefault() on touchstart, track the first touch ID, only > respond to touchmove and touchend for that ID, and fall back on mouse events. Is the touchstart preventDefault() required? Or just an optimization to prevent unused mouse events? Rather than porting all of Gaia from mouse to touch events now, wesj suggested that we might be able to write a .js shim to map real touch events to fake desktop-style mouse events. Gaia apps could require("mouse_event_shim.js") until they are ported correctly.

(In reply to Chris Peterson (:cpeterson) from comment #90) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #89) > > We need to preventDefault() on touchstart, track the first touch ID, only > > respond to touchmove and touchend for that ID, and fall back on mouse events. > > Is the touchstart preventDefault() required? Or just an optimization to > prevent unused mouse events? My understanding is that it's required to see touchmove. > Rather than porting all of Gaia from mouse to touch events now, wesj > suggested that we might be able to write a .js shim to map real touch events > to fake desktop-style mouse events. Gaia apps could > require("mouse_event_shim.js") until they are ported correctly. What would such a library look like?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #91) > > Is the touchstart preventDefault() required? Or just an optimization to > > prevent unused mouse events? > > My understanding is that it's required to see touchmove. In my testing, we get touchmove without needing to preventDefault() on touchstart. https://people.mozilla.com/~cpeterson/focus.html > What would such a library look like? A hypothetical shim library would map touchstart/move/end to fake mousedown/move/up events with some callbacks. The fake mouse events would need to copy the touch events' changedTouches[0].target/etc attributes. It would need to preventDefault() on touchstart to prevent Gecko from sending mousemove/down/up after touchend and perhaps filtering out multitouch events. It sounds is like more trouble than it's worth. We would still need to change all Gaia js files that want the fake mouse events. We would be adding an extra layer of voodoo to some code that is already confusing.

For now, I've landed the keyboard workaround that djf reviewed. If we decide to bite the bullet on the TabChild fix that requires fixes to many Gaia .js files, we can back these changes out. https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b6362784c2 https://hg.mozilla.org/integration/mozilla-inbound/rev/69b6541efafc https://hg.mozilla.org/integration/mozilla-inbound/rev/8351e1baffcb https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4a7e6f0e0b

(In reply to Chris Jones [:cjones] [:warhammer] from comment #91) > (In reply to Chris Peterson (:cpeterson) from comment #90) > > Rather than porting all of Gaia from mouse to touch events now, wesj > > suggested that we might be able to write a .js shim to map real touch events > > to fake desktop-style mouse events. Gaia apps could > > require("mouse_event_shim.js") until they are ported correctly. > > What would such a library look like? It might look like shared/js/gesture_detector.js If I understand correctly, apps that are just interested in clicks would still work as they are. So the porting burden is only for apps that are trying to do pans and swipes. Some of those apps (browser, calendar, clock, email, gallery, and system) already use the gesture detector library. So they should be fine (though I suppose that gesture_detector.js itself might need to be ported...) If there are few enough apps that aren't using gesture_detector, then porting those apps to use that utility library might be the way to go.

Based on new evidence in bug 814252, I think we should very seriously consider this. To do that we need to estimate the work to convert gaia. Left on the table if we decide not to do this is - breaking touch event standard and being stuck with that "forever". Fixing that is currently blocking+. - compatibility fallout from that - different behavior in "browser" and "app" content, visible to authors - bug 814252 and several blocking+ followups that require it - bug 819593, blocker, and bug 819595, soft-blocker currently So my initial idea was to make a shim that implements the necessary part of the pointer events proposal (not much). This is a straightforward change and is somewhat forwards compatible. My concern is that the gesture detector is overkill for the users who just need sane down/move*/up events for the primary touch point, and it moves more of platform "look and feel" into gaia. I know we've been losing that battle bigtime for v1, but I don't want to give more ground ;). We have 54 references to mousemove, which probably means <27 real users. That's a lot. The remaining data I'm going to gather is how painful converting a few to pointer events would be. Let's say homescreen and lockscreen. djf, if you want to do the same with gesture_detector, please do. That's another option we can consider.

As far as i know the homescreen uses the mousemove event because the drag&drop implementation is based on it. The problem is that when an item's ontouchmove event handler is called, its event.touches[0].target always points to the originating HTML element (the item) and not the element which is currently under the finger. So we decided to use mousemove event where the target of our gesture is the icon under the finger (the draggable scaled icon defines pointer events none)

Chris, It does seem like the kind of shim you're advocating ought to be straightforward. I imagine there are issues lurking there, but I wouldn't know what they are until I set out to write such a shim. (In reply to crdlc from comment #96) > The problem is that when an item's > ontouchmove event handler is called, its event.touches[0].target always > points to the originating HTML element (the item) and not the element which > is currently under the finger. This is one of those issues, and I remember now that it affected Margaret when converting the keyboard to touch events. I dinged her in a review for calling elementFromPoint() on every touchmove event. In this case, though, I don't see how we could avoid it. When cpeterson said he got the homescreen working with touch events, I suspect he meant just panning left and right. Or did you also port this icon dragging code, Chris?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #95) > The remaining data I'm going to gather is how painful converting a few to > pointer events would be. Let's say homescreen and lockscreen. See comment #86. Those are the two that cpeterson did. Though as I noted above, I don't know that he got the icon dragging code ported... maybe his patch is just for the panning from page to page. > djf, if you want to do the same with gesture_detector, please do. That's > another option we can consider. I agree that gesture_detector may well be overkill. Though for the lockscreen, if we're just interested in swipes it is at the right level of abstraction. apps/system/js/notifications.js uses GestureDetector, so apps/system/js/lockscreen.js could or should use it too. But I agree that that is really a separate issue. If we could write a shim that would just do the right thing, that would be a win.

I'm attempting a shim library based on the FSM infrastructure of gesture_detector, because that has proven itself to be stable and useful. I haven't applied cpeterson's patch to have anything to test it against yet, though, so this will just be a trial sketch of a shim library. I'll attach it here when I've got something.

Here's a first (completely untested, like there are probably syntax errors in the code) pass at a shim library. It is based on the finite state machine design of gesture_detector.js, but is much simpler than that library. The idea is that this would live in gaia/shared/js/ and apps that wanted traditional mouse events would just include it and everything would work. Though for apps that care about mouseover/mouseout or mouseenter/mouseleave or events, there might be some kind of additional configuration step required.

Let's take this discussion to bug 823619.

Comment on attachment 693969 [details] [diff] [review] WIP-TabChild-mouse-events.patch We're doing this evaluation in bug 823619.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #101) > Let's take this discussion to bug 823619. Since I landed my forms.js keyboard workaround on m-i, I'm going to allow this bug to get closed. We can continue the discussion of synthetic mouse/pointer events in bug 823619, as cjones suggested.

Agreed.

https://hg.mozilla.org/mozilla-central/rev/c2b6362784c2 https://hg.mozilla.org/mozilla-central/rev/69b6541efafc https://hg.mozilla.org/mozilla-central/rev/8351e1baffcb https://hg.mozilla.org/mozilla-central/rev/ae4a7e6f0e0b

https://hg.mozilla.org/releases/mozilla-b2g18/rev/6fcb7f267277 https://hg.mozilla.org/releases/mozilla-b2g18/rev/09351d706b32 https://hg.mozilla.org/releases/mozilla-b2g18/rev/620e73e281c8 https://hg.mozilla.org/releases/mozilla-b2g18/rev/2d6b0c741827

Reopening because I backed out from m-c and m-b2g18 for regressions like bug 823832 and in preparation for a real fix from bug 823619: https://hg.mozilla.org/mozilla-central/rev/feb09cb872f1 https://hg.mozilla.org/releases/mozilla-b2g18/rev/78295924efe8

Please provide a current status on this bug.

(In reply to Dylan Oliver [:doliver] from comment #108) > Please provide a current status on this bug. This bug will most likely be fixed by bug 823619. I was leaving this bug open to confirm after bug 823619 lands.

Chris, the policy is that all bb+ bugs must land on both Aurora and b2g18 when being uplifted. Why was that not done in this bug?

(In reply to Ryan VanderMeulen from comment #110) > Chris, the policy is that all bb+ bugs must land on both Aurora and b2g18 > when being uplifted. Why was that not done in this bug? Sorry, I thought that Gecko changes that only affected B2G were not wanted on Beta 18. I mistakenly thought that applied to Aurora 19, too. Part of my patch was backed out from Nightly 20 and b2g18, but I can uplift the portion that stuck to Aurora 19.

Please do, thanks.

I landed patch parts 0 and 1 on Aurora 19: https://hg.mozilla.org/releases/mozilla-aurora/rev/0d24defc39b0 https://hg.mozilla.org/releases/mozilla-aurora/rev/9742066c4eb8

This has been fixed by bug 823619.