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:
    • Rank:
      49827

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          Frédéric Massart added a comment -

          Awesome!

          Show
          Frédéric Massart added a comment - Awesome!
          Hide
          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
          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
          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
          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
          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
          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 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 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
          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
          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: