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

Editor shrinks using collapse button

    Details

    • Testing Instructions:
      Hide

      Please test this in as many supported browsers as possible.

      1. Go to a typical form including a HTML editor (e.g. make a forum post) and verify that nothing has changed there.
      2. Create a new Multiple choice question, and admire how much more compact the form is with all the HTML editing tools hidden (apart from question text and general feedback).
      3. Verify that you can hide or reveal the editing tools on any Editor field where they are initially hidden.
      4. Show the editor tools and adjust the size of the editor text area.
      5. Show and hide the editor tools and few times. Watch for the size of the toolbars and text area. Confirm they stay the correct size.

      Show
      Please test this in as many supported browsers as possible. 1. Go to a typical form including a HTML editor (e.g. make a forum post) and verify that nothing has changed there. 2. Create a new Multiple choice question, and admire how much more compact the form is with all the HTML editing tools hidden (apart from question text and general feedback). 3. Verify that you can hide or reveal the editing tools on any Editor field where they are initially hidden. 4. Show the editor tools and adjust the size of the editor text area. 5. Show and hide the editor tools and few times. Watch for the size of the toolbars and text area. Confirm they stay the correct size.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:

      Description

      When editor is resized by the user the text area slowly loses height and width each time the editor show/hide toggle is used.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            colchambers Colin Chambers added a comment - - edited

            Fixed and pushed to MDL-39220. Tinymce has some standard show/hide methods for toolbar and pathbar that work fine in recent browser. To support IE 6 and 7 Had to add extra code using using tinymce methods for resizing.

            That was over a year ago. Things have moved on. Removing this extra code made the show/hide method work correctly in chrome/firefox/IE8-9. IE-7 hides the toolbar and pathbar but doesn't collapse the areas they took up so you don't save any space.

            Can handle older browsers if required but won't unless asked. Will just remove the show/hide toolbar option for browsers like IE <6 that can't support the feature. Too awkward to fix properly.

            Show
            colchambers Colin Chambers added a comment - - edited Fixed and pushed to MDL-39220 . Tinymce has some standard show/hide methods for toolbar and pathbar that work fine in recent browser. To support IE 6 and 7 Had to add extra code using using tinymce methods for resizing. That was over a year ago. Things have moved on. Removing this extra code made the show/hide method work correctly in chrome/firefox/IE8-9. IE-7 hides the toolbar and pathbar but doesn't collapse the areas they took up so you don't save any space. Can handle older browsers if required but won't unless asked. Will just remove the show/hide toolbar option for browsers like IE <6 that can't support the feature. Too awkward to fix properly.
            Hide
            colchambers Colin Chambers added a comment - - edited

            Summary
            Fixed by removing editor resizing support for legacy browsers e.g. IE <8. Tested in Chrome, Firefox, IE 8-9. Works perfectly.

            Explanation
            Every time the show/hide editor tools was click the editor textarea and toolbars would shrink to until reaching a default size.

            By removing code to resize the editor for older browsers the text area and toolbars keep the correct size.

            The editor will still work perfectly in older browsers. The show/hide button will hide and show the toolbars. The overall space taken up won't change though so there will be no benefit. It won't stop anyone using the editor though.

            The git diff only shows the code removed. Just above that you find the code that hides and shows the toolbars. This is doing the work and makes the code I removed redundant for most browsers. I removed the code because it didn't work correctly for most browsers. It's more maintainable to remove the feature for older browsers than provide code for them.

            To help review the code this is what precedes the code I removed.

            // Toggle the various states and update the button text to suit
            if (newstate === 0)

            { toolbar.hide(); statusbar.hide(); button.setContent(M.util.get_string('showeditortoolbar', 'form')); }

            else

            { toolbar.show(); statusbar.show(); button.setContent(M.util.get_string('hideeditortoolbar', 'form')); }
            Show
            colchambers Colin Chambers added a comment - - edited Summary Fixed by removing editor resizing support for legacy browsers e.g. IE <8. Tested in Chrome, Firefox, IE 8-9. Works perfectly. Explanation Every time the show/hide editor tools was click the editor textarea and toolbars would shrink to until reaching a default size. By removing code to resize the editor for older browsers the text area and toolbars keep the correct size. The editor will still work perfectly in older browsers. The show/hide button will hide and show the toolbars. The overall space taken up won't change though so there will be no benefit. It won't stop anyone using the editor though. The git diff only shows the code removed. Just above that you find the code that hides and shows the toolbars. This is doing the work and makes the code I removed redundant for most browsers. I removed the code because it didn't work correctly for most browsers. It's more maintainable to remove the feature for older browsers than provide code for them. To help review the code this is what precedes the code I removed. // Toggle the various states and update the button text to suit if (newstate === 0) { toolbar.hide(); statusbar.hide(); button.setContent(M.util.get_string('showeditortoolbar', 'form')); } else { toolbar.show(); statusbar.show(); button.setContent(M.util.get_string('hideeditortoolbar', 'form')); }
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Colin,

            This looks good to me. I think that there's only so much we can do to fully accommodate older browsers and we should't break new functionality for all other browsers to try and make something work for them.

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Colin, This looks good to me. I think that there's only so much we can do to fully accommodate older browsers and we should't break new functionality for all other browsers to try and make something work for them.
            Hide
            fred Frédéric Massart added a comment -

            Awesome!

            Show
            fred Frédéric Massart added a comment - Awesome!
            Hide
            colchambers Colin Chambers added a comment -

            Thanks Guys.

            Andrew. That's my view too. Was checking what Moodles view is. The cost of supporting older browsers is akin to older cars. There's only so much maintenance that's worth doing. Eventually the pressure to upgrade to a new browser must be allowed to overcome the challenge of doing so.

            Show
            colchambers Colin Chambers added a comment - Thanks Guys. Andrew. That's my view too. Was checking what Moodles view is. The cost of supporting older browsers is akin to older cars. There's only so much maintenance that's worth doing. Eventually the pressure to upgrade to a new browser must be allowed to overcome the challenge of doing so.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Colin, this has been integrated now.

            Could you please update the testing instructions to include testing with a version of IE < 8 to check what changes.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Colin, this has been integrated now. Could you please update the testing instructions to include testing with a version of IE < 8 to check what changes. Many thanks Sam
            Hide
            dmonllao David Monllaó added a comment -

            It passes. All working as expected, tested in

            • Linux + Chrome 26.0.1410.63
            • Linux + Firefox 20.0
            • OS X + Safari 4.0.3
            • Windows 7 + IE9

            Thanks for including regressions checks in the testing instructions

            Show
            dmonllao David Monllaó added a comment - It passes. All working as expected, tested in Linux + Chrome 26.0.1410.63 Linux + Firefox 20.0 OS X + Safari 4.0.3 Windows 7 + IE9 Thanks for including regressions checks in the testing instructions
            Hide
            damyon Damyon Wiese added a comment -

            Reverting this patch at least gets rid of the padding in MDL-39271 - but the editor has too few rows either way.

            Show
            damyon Damyon Wiese added a comment - Reverting this patch at least gets rid of the padding in MDL-39271 - but the editor has too few rows either way.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I feel myself really alone tonight! So was time to push your fixes upstream!

            "Lest we forget. We will remember them."

            Thanks and ciao!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13