766066 - mozKeyboard.onfocuschange shouldn't be raised when you are scrolling

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[alberto.pastor]

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.

Comment 1

[Dietrich Ayala (:dietrich)]

Blocking+, Kaze confirmed happens in Settings too.

Should a Keyboard dev be the one to fix this? Or is this in the input widgetry?

Comment 2

[alberto.pastor]

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)

Comment 3

[Fabien Cazenave [:kaze]]

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

Comment 4

[Andrew Overholt [:overholt]]

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.

Comment 5

[Andrew Overholt [:overholt]]

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?  :)

Comment 6

[(no longer active)]

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.

Comment 7

[Andrew Overholt [:overholt]]

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

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 9

[Andrew Overholt [:overholt]]

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

Comment 10

[Dietrich Ayala (:dietrich)]

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

Comment 11

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

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.

Comment 12

[Chris Peterson [:cpeterson]]

blassey asked me to investigate this bug.

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 14

[Chris Peterson [:cpeterson]]

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.

Comment 16

[Alex Keybl [:akeybl]]

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

Comment 17

[Josh Carpenter [:jcarpenter]]

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.

Comment 18

[Josh Carpenter [:jcarpenter]]

bug 804315 is related.

Comment 19

[Chris Peterson [:cpeterson]]

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

Comment 21

[Dietrich Ayala (:dietrich)]

Hi Chris, any update here?

Comment 22

[Chris Peterson [:cpeterson]]

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

Comment 23

[Chris Peterson [:cpeterson]]

Attachment: part-1-dont-open-when-scrolling.patch

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().

Comment 24

[Chris Peterson [:cpeterson]]

Attachment: part-2-dont-close-when-scrolling.patch

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 25

[David Flanagan [:djf]]

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 26

[David Flanagan [:djf]]

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.

Comment 27

[Chris Peterson [:cpeterson]]

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.

Comment 28

[David Flanagan [:djf]]

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 29

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 33

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip

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.

Comment 34

[Chris Peterson [:cpeterson]]

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.

Comment 35

[Chris Peterson [:cpeterson]]

Attachment: part-1-remove-ime-state-observer.patch

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.

Comment 36

[Chris Peterson [:cpeterson]]

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.

Comment 37

[Chris Peterson [:cpeterson]]

Attachment: part-2-separate-focus-and-ime.patch

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().

Comment 38

[Chris Peterson [:cpeterson]]

Attachment: part-3-keyboard-on-click.patch

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.

Comment 39

[Wesley Johnston (:wesj)]

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 40

[David Flanagan [:djf]]

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 41

[David Flanagan [:djf]]

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 42

[David Flanagan [:djf]]

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 44

[Tim Taubert [:ttaubert] (inactive)]

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 45

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 46

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 47

[David Flanagan [:djf]]

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.

Comment 48

[Tim Taubert [:ttaubert] (inactive)]

(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 :|

Comment 49

[Faramarz Rashed]

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?

Comment 50

[Chris Peterson [:cpeterson]]

Attachment: part-1-postpone-input-focus.patch

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.

Comment 51

[Chris Peterson [:cpeterson]]

Attachment: part-2-remove-unused-code.patch

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.

Comment 52

[Chris Peterson [:cpeterson]]

(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 53

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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 54

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 55

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 56

[Tim Taubert [:ttaubert] (inactive)]

(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().

Comment 57

[Chris Peterson [:cpeterson]]

Attachment: postpone-input-focus-v4.patch

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 58

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 59

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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 60

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 691564 [details] [diff] [review]
postpone-input-focus-v4.patch

Oh, good catch!  I forgot about this.

Comment 61

[Chris Peterson [:cpeterson]]

(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..

Comment 62

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 63

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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?

Comment 64

[Alex Keybl [:akeybl]]

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.

Comment 65

[Chris Peterson [:cpeterson]]

(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.

Comment 66

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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.

Comment 67

[Chris Peterson [:cpeterson]]

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

Comment 68

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Let me know if we're blocked on me.

Comment 69

[Dietrich Ayala (:dietrich)]

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?

Comment 70

[David Flanagan [:djf]]

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.

Comment 71

[:Margaret Leibovic]

(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! :)

Comment 72

[Chris Peterson [:cpeterson]]

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

Comment 73

[Chris Peterson [:cpeterson]]

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

Comment 74

[Chris Peterson [:cpeterson]]

Attachment: part-1-remove-ime-state-observer-v2.patch

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.

Comment 75

[Chris Peterson [:cpeterson]]

Attachment: part-2-separate-focus-and-ime-v2.patch

Part 2: Handle element focus and IME state separately.

Comment 76

[Chris Peterson [:cpeterson]]

Attachment: part-3-keyboard-on-click-v2.patch

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

Comment 77

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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 :/.

Comment 78

[David Flanagan [:djf]]

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 79

[David Flanagan [:djf]]

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 80

[David Flanagan [:djf]]

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?

Comment 81

[Chris Peterson [:cpeterson]]

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".

Comment 82

[Chris Peterson [:cpeterson]]

Attachment: WIP-TabChild-mouse-events.patch

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.

Comment 83

[Tim Taubert [:ttaubert] (inactive)]

(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 :)

Comment 84

[David Flanagan [:djf]]

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?

Comment 85

[Chris Peterson [:cpeterson]]

(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.

Comment 86

[Chris Peterson [:cpeterson]]

Attachment: WIP-gaia-touch-events.patch

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.

Comment 87

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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 :(.

Comment 88

[Chris Peterson [:cpeterson]]

(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?

Comment 89

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 90

[Chris Peterson [:cpeterson]]

(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.

Comment 91

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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?

Comment 92

[Chris Peterson [:cpeterson]]

(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.

Comment 93

[Chris Peterson [:cpeterson]]

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

Comment 94

[David Flanagan [:djf]]

(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.

Comment 95

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 96

[Cristian Rodriguez (:crdlc)]

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)

Comment 97

[David Flanagan [:djf]]

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?

Comment 98

[David Flanagan [:djf]]

(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.

Comment 99

[David Flanagan [:djf]]

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.

Comment 100

[David Flanagan [:djf]]

Attachment: wip synthetic mouse event shim library

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.

Comment 101

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Let's take this discussion to bug 823619.

Comment 102

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 693969 [details] [diff] [review]
WIP-TabChild-mouse-events.patch

We're doing this evaluation in bug 823619.

Comment 103

[Chris Peterson [:cpeterson]]

(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.

Comment 104

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Agreed.

Comment 105

[Ed Morley [:emorley]]

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

Comment 106

[Chris Peterson [:cpeterson]]

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

Comment 107

[Chris Peterson [:cpeterson]]

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

Comment 108

[Dylan Oliver [:doliver]]

Please provide a current status on this bug.

Comment 109

[Chris Peterson [:cpeterson]]

(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.

Comment 110

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 111

[Chris Peterson [:cpeterson]]

(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.

Comment 112

[Ryan VanderMeulen [:RyanVM]]

Please do, thanks.

Comment 113

[Chris Peterson [:cpeterson]]

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

Comment 114

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

This has been fixed by bug 823619.