Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41328

Tinymce: when editing is on, can not move caret position on iOS 6

    Details

    • Testing Instructions:
      Hide
      1. On iOS
      2. Make sure TinyMCE is the configured editor
      3. Turn editing on
      4. Edit settings for a course and test the tinymce editor in the summary
      5. Make sure you can type some text, click somewhere else and keep typing without closing the virtual keyboard.
      6. Repeat the test using a desktop browser (any).
      Show
      On iOS Make sure TinyMCE is the configured editor Turn editing on Edit settings for a course and test the tinymce editor in the summary Make sure you can type some text, click somewhere else and keep typing without closing the virtual keyboard. Repeat the test using a desktop browser (any).
    • Workaround:
      Hide

      Type something.

      Close the keyboard.

      Click where you want to move to.

      Type something.

      Show
      Type something. Close the keyboard. Click where you want to move to. Type something.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-41328-master

      Description

      on iPad/iPhone/iPod Touch:

      • connect as admin or teacher
      • go to a course. Turn editing on.
      • Edit Course settings
      • in the description enter:
        "This is the beginning of the desc...
        this is the end of the desc."
      • now single tap between the text. Your caret should have move where you tapped, you should be able to add more text there. You currently can't.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Changing to blocker, as suggested by Damyon.

            Show
            ankit_frenz Ankit Agarwal added a comment - Changing to blocker, as suggested by Damyon.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I don't think that this is necessarily a blocker, and I really suspect that this is a browser issue. I'd be very surprised if there is a workaround in JS.

            MDL-36803 is a workaround to a pretty rubbish browser bug, and I suspect that this is related.

            That said, my gut suspects that this is the same focus issue - or rather than a touch event steals focus from the contenteditable (when it's in an iframe). We may be able to add a delegated touch listener to keep focus on the iframe when the touch is over the iframe.

            In writing this, I also wonder whether this focus stealing could be related to a layering issue of some kind...

            Show
            dobedobedoh Andrew Nicols added a comment - I don't think that this is necessarily a blocker, and I really suspect that this is a browser issue. I'd be very surprised if there is a workaround in JS. MDL-36803 is a workaround to a pretty rubbish browser bug, and I suspect that this is related. That said, my gut suspects that this is the same focus issue - or rather than a touch event steals focus from the contenteditable (when it's in an iframe). We may be able to add a delegated touch listener to keep focus on the iframe when the touch is over the iframe. In writing this, I also wonder whether this focus stealing could be related to a layering issue of some kind...
            Hide
            nebgor Aparup Banerjee added a comment -

            another work around is to press until a magnifier comes up, then drag it around so you see the cursor show up where you want it.

            Show
            nebgor Aparup Banerjee added a comment - another work around is to press until a magnifier comes up, then drag it around so you see the cursor show up where you want it.
            Hide
            damyon Damyon Wiese added a comment -

            Requesting a peer review for this (will add backports).

            This solution works by completely disabling YUI use of touchevents if the browser is ios and tinymce is the current editor.

            Downsides:

            • No drag and drop for this combination (mitigated this on master by allowing the accessible drag and drop menu to be activated by clicking on the drag handles).
            • If a third party module uses touchevents directly not via YUI this will break again.
            • The drag and drop menu cannot be backported
            Show
            damyon Damyon Wiese added a comment - Requesting a peer review for this (will add backports). This solution works by completely disabling YUI use of touchevents if the browser is ios and tinymce is the current editor. Downsides: No drag and drop for this combination (mitigated this on master by allowing the accessible drag and drop menu to be activated by clicking on the drag handles). If a third party module uses touchevents directly not via YUI this will break again. The drag and drop menu cannot be backported
            Hide
            damyon Damyon Wiese added a comment -

            Backports added.

            Show
            damyon Damyon Wiese added a comment - Backports added.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I'm not sure that I'm comfortable with this kind of change, and I foresee a number of future pitfalls should we go ahead with it:

            1. this essentially prevents any drag/drop/touch access for iOS users where TinyMCE is present. Where TinyMCE is not present, drag/drop/touch will work. This will produce a mass of inconsistent interfaces across Moodle depending on whether TinyMCE is present;
            2. we don't know what other kinds of touch interface we may add in the future. For example, we may want to add a swipe gesture listener when browsing thumbnails in the file manager. Since almost every file manager is on the same page as a TinyMCE editor, this won't work on any iOS device - except for those very rare situations where TinyMCE is not present and it magically works (cue lots of bug reports)
            3. we may add dynamically loaded tinyMCE editors in the future (this is a change I've been thinking about a lot of late). In these instances, tinyMCE will be the inconsistent part as touch will already be enabled and TinyMCE will be broken still.

            All in all, this seems like we're throwing the baby out with the bathwater over what is an odd bug in a single browser which the vendor should fix. It's still possible to use the editor. You can still move around the textarea with the Loupe.

            Looking at this quickly, I've found an alternate fix which improves the state of play though doesn't fix it entirely. The fix we found for the keyboard was to add a keydown listener - I've just created a touchend listener which does the same thing - e.g. focus the editor when the contentDocument is interacted with via touch. This lets you move from work to word, but doesn't seem to let you focus mid-word.

            See https://github.com/andrewnicols/moodle/compare/MDL-41328-m for my fix.

            In summary, the current patch for this gets a massive -1 from me. We should see if we can find a way of raising these issues with Apple somehow rather than killing parts of the UI in certain conditions. This will only ever confuse users and hamstring us later on when we want to make use of touch events.

            Show
            dobedobedoh Andrew Nicols added a comment - I'm not sure that I'm comfortable with this kind of change, and I foresee a number of future pitfalls should we go ahead with it: this essentially prevents any drag/drop/touch access for iOS users where TinyMCE is present. Where TinyMCE is not present, drag/drop/touch will work. This will produce a mass of inconsistent interfaces across Moodle depending on whether TinyMCE is present; we don't know what other kinds of touch interface we may add in the future. For example, we may want to add a swipe gesture listener when browsing thumbnails in the file manager. Since almost every file manager is on the same page as a TinyMCE editor, this won't work on any iOS device - except for those very rare situations where TinyMCE is not present and it magically works (cue lots of bug reports) we may add dynamically loaded tinyMCE editors in the future (this is a change I've been thinking about a lot of late). In these instances, tinyMCE will be the inconsistent part as touch will already be enabled and TinyMCE will be broken still. All in all, this seems like we're throwing the baby out with the bathwater over what is an odd bug in a single browser which the vendor should fix. It's still possible to use the editor. You can still move around the textarea with the Loupe. Looking at this quickly, I've found an alternate fix which improves the state of play though doesn't fix it entirely. The fix we found for the keyboard was to add a keydown listener - I've just created a touchend listener which does the same thing - e.g. focus the editor when the contentDocument is interacted with via touch. This lets you move from work to word, but doesn't seem to let you focus mid-word. See https://github.com/andrewnicols/moodle/compare/MDL-41328-m for my fix. In summary, the current patch for this gets a massive -1 from me. We should see if we can find a way of raising these issues with Apple somehow rather than killing parts of the UI in certain conditions. This will only ever confuse users and hamstring us later on when we want to make use of touch events.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Adding Moxiecode Systems AB as a watcher. Spock may be interested in this issue too.

            Show
            dobedobedoh Andrew Nicols added a comment - Adding Moxiecode Systems AB as a watcher. Spock may be interested in this issue too.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            This gets a -1 from me for the above reasons.

            I have still peer reviewed some of the code for your interest though:

            Additional comments:

            Why call the M.util.init_drag_and_drop as a function using js_init_call ?
            js_init_call adds code to the footer, and you may find that the touch events have already been listened by that point. You'd be far better creating the function in javascript-static.js (as you already do) and invoking it immediately (non dogs ). You're doing it based on the value of M.cfg.disabledraganddrop so:

            (function() {
              if (M.cfg.disabledraganddrop) {
                YUI.Env.UA.touchEnabled = false;
              }
            }());
            

            Since javascript-static.js is loaded synchronously after both M.cfg is defined, and after SimpleYUI is loaded, this will be invoked as it's processed and won't wait until the footer to take effect. It will also tidy up the code.

            Show
            dobedobedoh Andrew Nicols added a comment - This gets a -1 from me for the above reasons. I have still peer reviewed some of the code for your interest though: Additional comments: Why call the M.util.init_drag_and_drop as a function using js_init_call ? js_init_call adds code to the footer, and you may find that the touch events have already been listened by that point. You'd be far better creating the function in javascript-static.js (as you already do) and invoking it immediately (non dogs ). You're doing it based on the value of M.cfg.disabledraganddrop so: (function() { if (M.cfg.disabledraganddrop) { YUI.Env.UA.touchEnabled = false; } }()); Since javascript-static.js is loaded synchronously after both M.cfg is defined, and after SimpleYUI is loaded, this will be invoked as it's processed and won't wait until the footer to take effect. It will also tidy up the code.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Andrew,

            My general thoughts on this issue:

            • it is hugely important to the user experience
            • users have to discover that they can use the loupe in order to use the workaround which is unrealistic and frustrating
            • this issue has been hanging around and needs to be resolved
            • a nicely working editor is much more important that drag and drop, given that everywhere we use drag and drop, we need to implement alternatives anyway
            • removing the drag and drop makes the the editor behave 100% reliably
            • this patch does not disable drag and drop on some pages and not others - it disables drag and drop 100% on iOS when tinymce is the configured text editor for this user (If the user changes their preferred format to plain text - they will be able to use drag and drop - but not tinymce).

            Re: calling the function directly in javascript-static or from the footer - the current code will call the function from the footer - but it will be the first, because this is the first call to js_init_function - but that is probably unreliable so agree should just run the code directly in javascript-static.

            I'll test your additional workaround and comment on whether I think the behaviour is good enough.

            Show
            damyon Damyon Wiese added a comment - Thanks Andrew, My general thoughts on this issue: it is hugely important to the user experience users have to discover that they can use the loupe in order to use the workaround which is unrealistic and frustrating this issue has been hanging around and needs to be resolved a nicely working editor is much more important that drag and drop, given that everywhere we use drag and drop, we need to implement alternatives anyway removing the drag and drop makes the the editor behave 100% reliably this patch does not disable drag and drop on some pages and not others - it disables drag and drop 100% on iOS when tinymce is the configured text editor for this user (If the user changes their preferred format to plain text - they will be able to use drag and drop - but not tinymce). Re: calling the function directly in javascript-static or from the footer - the current code will call the function from the footer - but it will be the first, because this is the first call to js_init_function - but that is probably unreliable so agree should just run the code directly in javascript-static. I'll test your additional workaround and comment on whether I think the behaviour is good enough.
            Hide
            damyon Damyon Wiese added a comment -

            Tested the additional workaround - and I think it works well enough. I'll switch this issue to use your patch and put me as PR on it.

            Thanks Andrew

            Show
            damyon Damyon Wiese added a comment - Tested the additional workaround - and I think it works well enough. I'll switch this issue to use your patch and put me as PR on it. Thanks Andrew
            Hide
            damyon Damyon Wiese added a comment -

            The code in Andrews patch looks good to me - this is my PR +1. (I just added backport branches).

            Show
            damyon Damyon Wiese added a comment - The code in Andrews patch looks good to me - this is my PR +1. (I just added backport branches).
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I think till the user knows on iOS that the drag and drop has been disabled, Damyon solution is fine. Maybe displayed an alert message like the drag and drop one (when you turn editing on in the course) => when someone login: "Drag and drop has been disabled for your iOS device. To enable it for your iOS device, you need to use a different HTML editor than tinymce."

            Show
            jerome Jérôme Mouneyrac added a comment - I think till the user knows on iOS that the drag and drop has been disabled, Damyon solution is fine. Maybe displayed an alert message like the drag and drop one (when you turn editing on in the course) => when someone login: "Drag and drop has been disabled for your iOS device. To enable it for your iOS device, you need to use a different HTML editor than tinymce."
            Hide
            damyon Damyon Wiese added a comment -

            -1 for that confusing message Jerome - and I think andrews patch addresses the issue well enough. (Note my branches on this issue have been updated to include Andrews patch and not my one from Friday - they are on my repo just because I added backports).

            Show
            damyon Damyon Wiese added a comment - -1 for that confusing message Jerome - and I think andrews patch addresses the issue well enough. (Note my branches on this issue have been updated to include Andrews patch and not my one from Friday - they are on my repo just because I added backports).
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            Andrew's patch is the same solution than yours, so I was speaking about your solution (with Andrew's patch). If I mentioned to let the user knows about the situation it's because I agree with Andrew when he says that the solution will bring inconsistency and bug reports if the user doesn't know why drag and drop doesn't work.

            Show
            jerome Jérôme Mouneyrac added a comment - - edited Andrew's patch is the same solution than yours, so I was speaking about your solution (with Andrew's patch). If I mentioned to let the user knows about the situation it's because I agree with Andrew when he says that the solution will bring inconsistency and bug reports if the user doesn't know why drag and drop doesn't work.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks everyone, integrated to master, 25 and 24

            Show
            poltawski Dan Poltawski added a comment - Thanks everyone, integrated to master, 25 and 24
            Hide
            salvetore Michael de Raadt added a comment -

            Test result: Success!

            Tested on 2.4, 2.5 and master. Well done, Andrew.

            Show
            salvetore Michael de Raadt added a comment - Test result: Success! Tested on 2.4, 2.5 and master. Well done, Andrew.
            Hide
            poltawski Dan Poltawski added a comment -

            Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke:

            A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

            Show
            poltawski Dan Poltawski added a comment - Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke: A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Sep/13