TryGhost / Ghost

Independent technology for modern publishing, memberships, subscriptions and newsletters.
https://ghost.org
MIT License
47.48k stars 10.36k forks source link

Code Injection Field Needs Double Tap to Select #5717

Closed dbalders closed 7 years ago

dbalders commented 9 years ago

Issue Summary

Device: iPhone 6

This may or may not be a bug, but you have to double tap in order to enter either field on the Code Injection page. Is a little confusing cause enterable fields are usually single tap.

Steps to Reproduce

  1. Log into the admin interface on an iPhone\
  2. Open code injection
  3. Try to tap into the code injection page

    Technical details:

    • Ghost Version: Master
    • Client OS: Mac 10.10
    • Server OS: Mac 10.10
    • Browser: Safari Mobile
    • Database: SQLite
ErisDS commented 9 years ago

The code injection fields use CodeMirror, which until recently, didn't really work on mobile at all.

As of version 5.0 it is supposed to support mobile and iOS, so I'm not sure if this is a limitation or something to do with our implementation. I tried upgrading to the latest version 5.7.0 and that didn't help.

Please can someone investigate whether this is always the case for CodeMirror or whether it's specific to our setup inside of ember? If it's always the case then we can close this and open an issue against CM, or if it can be fixed a PR would be much appreciated.

program247365 commented 9 years ago

Investigation Using iPhone 6 And Safari Dev Tools

So I've found the following:

https://github.com/codemirror/CodeMirror/blob/de7b15f97f6a2f85ae0b09dea8c649f5d3c29159/lib/codemirror.js#L3425-L3440

    on(d.scroller, "touchend", function(e) {
      var touch = d.activeTouch;
      if (touch && !eventInWidget(d, e) && touch.left != null &&
          !touch.moved && new Date - touch.start < 300) {
        var pos = cm.coordsChar(d.activeTouch, "page"), range;
        if (!touch.prev || farAway(touch, touch.prev)) // Single tap
          range = new Range(pos, pos);
        else if (!touch.prev.prev || farAway(touch, touch.prev.prev)) // Double tap
          range = cm.findWordAt(pos);
        else // Triple tap
          range = new Range(Pos(pos.line, 0), clipPos(cm.doc, Pos(pos.line + 1, 0)));
        cm.setSelection(range.anchor, range.head);
        cm.focus();
        e_preventDefault(e);
      }
      finishTouch();

touch.start is always a large number here. Like this:

screen shot 2015-10-11 at 11 25 45 pm

So it evaluates false:

screen shot 2015-10-11 at 11 27 25 pm

So here we are CodeMirror knows the touchevent happened, and once you get in there, it skips the 'single tap' if statement every time because of this and results in this Kevin being sad (http://sadkev.in), because suddenly we can't have nice things on mobile safari:

http://sadkev.in/images/3.gif

So if I modify the value in the console, as it's at my breakpoint before this if statement to be something less than 300:

screen shot 2015-10-11 at 11 47 57 pm

Lo' and behold now that statement evaluates true, and it gets to the single tap, and on one tap, we do get the cursor to show in the codemirror field. :smiley:

Conclusion

So I just navigated to http://codemirror.net/, and tried on my same iPhone, and on one tap, it does make the code field active, and the code in question is the same.

So is it Ghost's use of FastClick? Or something else. Signs do point to @ErisDS's assessment, that it's something that Ghost is doing, not CM itself. Delaying the event with Fastclick? Something else? Race condition? A delay of the event itself, causing codemirror not to register the single click?

I'll keep investigating...

kevinansfield commented 9 years ago

touch.start should be a large number here as new Date will also be a large number (try running (new Date) - 0 in the console).

I'm not sure but there may also be some issues with checking it in the debugger as re-evaluating new Date - touch.start < 300 will always give you a new number that will evaluate to false as it's been more than 300ms since touch.start.

If however that is where the problem arises then something is causing a > 300 ms delay between touch.start and the if statement being run resulting in the statement being false.

horner commented 8 years ago

FYI, I found your post while burning two days on this same bug. What finally fixed it for me was removing the call to blur in fastclick. It has been in a pull request for over a year. See: https://github.com/ftlabs/fastclick/pull/350

Update: Nope, It did not work. Had to do this: https://github.com/ftlabs/fastclick/pull/448

ErisDS commented 7 years ago

We no longer have fastclick in Ghost Admin. Assuming this bug has gone away.