Moodle
  1. Moodle
  2. MDL-39351

Collapsible editor not quite working on qtype_match edit form

    Details

    • Testing Instructions:
      Hide

      1. Put the attached test.php in the root of your Moodle insall.

      2. To there. You will see a form with many editors, with different values for the rows="" option. For each size, there will be a collpased TinyMCE, a normal TinyMCE, and a plain text area.

      3. Make sure that, in each case, the size of the editing area in the collapsed TinyMCE is the same as the editing area of the un-collapsed TinyMCE.

      Please test all browsers you can get your hands on.

      Show
      1. Put the attached test.php in the root of your Moodle insall. 2. To there. You will see a form with many editors, with different values for the rows="" option. For each size, there will be a collpased TinyMCE, a normal TinyMCE, and a plain text area. 3. Make sure that, in each case, the size of the editing area in the collapsed TinyMCE is the same as the editing area of the un-collapsed TinyMCE. Please test all browsers you can get your hands on.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      49982

      Description

      the white edit area has grey bands top and bottom, because for some reason the table is too high.

      It seems to be related to the height of the editor.

      • In multichoice (rows="1") the editor is fine.
      • In match (rows="3") it breaks like this.
      • In truefalse (rows="10") the editor is fine.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Test form with editors size 1 to 10 rows.

          Show
          Tim Hunt added a comment - Test form with editors size 1 to 10 rows.
          Hide
          Tim Hunt added a comment -

          Acutally, all sizes other than 1 row are broken.

          Show
          Tim Hunt added a comment - Acutally, all sizes other than 1 row are broken.
          Hide
          Tim Hunt added a comment -

          Right, I just used git bisect for the first time, and found that this bug was caused by MDL-39220.

          Show
          Tim Hunt added a comment - Right, I just used git bisect for the first time, and found that this bug was caused by MDL-39220 .
          Hide
          Tim Hunt added a comment -

          OK. Here is an interesting thing.
          1. Click Show editing tools.
          2. Resize editor/
          3. Click Hide editing tools.

          And, it goes back to the size it should be. Or not, because acutally it is too small, but anyway, not grey bars.

          Show
          Tim Hunt added a comment - OK. Here is an interesting thing. 1. Click Show editing tools. 2. Resize editor/ 3. Click Hide editing tools. And, it goes back to the size it should be. Or not, because acutally it is too small, but anyway, not grey bars.
          Hide
          Tim Hunt added a comment -

          Right. This fixes it on IE, Firefox and Chrome for me.

          Show
          Tim Hunt added a comment - Right. This fixes it on IE, Firefox and Chrome for me.
          Hide
          Tim Hunt added a comment -

          Updated test.php file.

          Show
          Tim Hunt added a comment - Updated test.php file.
          Hide
          Colin Chambers added a comment -

          Great work Tim. I like the fix. Nice and simple

          Show
          Colin Chambers added a comment - Great work Tim. I like the fix. Nice and simple
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Damyon Wiese added a comment -

          Editors 1-4

          Show
          Damyon Wiese added a comment - Editors 1-4
          Hide
          Damyon Wiese added a comment -

          Hi Tim,

          I don't think this solution is quite right. By removing the height, you are forcing the number of rows in the collapsed editor to be the same as the number of rows shown in the expanded editor, but because the expanded editor includes the toolbars, it doesn't show more than 1 row until the original text area is 8 rows big (Screenshots from your test page attached).

          So this forces the collapsed editors to only show a single text row everywhere unless the original textarea was really huge.

          I'll add Martin to get his opinion on this - I think that the collapsed editor should take up roughly the same amount of room as the original text area.

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Tim, I don't think this solution is quite right. By removing the height, you are forcing the number of rows in the collapsed editor to be the same as the number of rows shown in the expanded editor, but because the expanded editor includes the toolbars, it doesn't show more than 1 row until the original text area is 8 rows big (Screenshots from your test page attached). So this forces the collapsed editors to only show a single text row everywhere unless the original textarea was really huge. I'll add Martin to get his opinion on this - I think that the collapsed editor should take up roughly the same amount of room as the original text area. Thanks, Damyon
          Hide
          Damyon Wiese added a comment -

          Martin - can you please comment on how you would like this to work?

          Thanks!

          Show
          Damyon Wiese added a comment - Martin - can you please comment on how you would like this to work? Thanks!
          Hide
          Martin Dougiamas added a comment -

          Well, looking at the screenshots, that behaviour is not ideal IMO.

          The editor looks awkward and difficult with three big header rows and only one line of text-editing. And it looks like this might be the case most of the time.

          At least can we have a minimum text-editor size of something like 3 header rows and 3 actual text rows?

          Show
          Martin Dougiamas added a comment - Well, looking at the screenshots, that behaviour is not ideal IMO. The editor looks awkward and difficult with three big header rows and only one line of text-editing. And it looks like this might be the case most of the time. At least can we have a minimum text-editor size of something like 3 header rows and 3 actual text rows?
          Hide
          Tim Hunt added a comment -

          Damyon / Martin.

          I think that the most important thing is that the size of the collapsed editor and the size of the editor is the same when you toggle.

          All the sizing is controlled by TinyMCE, and the way it sizes itself is burried deep in the TinyMCE code. I am not changing that.

          The minimal size editor works very well when editing Multiple choice questions. I would not want to see the size of the box on that form get any bigger, so I vote against any increase in the minimum size.

          So, what does that leave us with? The solution I made. The size of the editable region is up to TinyMCE, and that means all size specifications 1-7 give the same one-line editor. It's weird, but that's the way it is, and you (as a developer creating a form) can get an editor of the size you want if you can find the right rows= parameter.

          Hence I think this is the best fix possible. If you want to create something better, please re-open and assign this bug to yourself. Actually, I suggest you integrate this fix anyway, because the current behaviour is buggy, and then create a new issue assigned to yourself to fix things further.

          Show
          Tim Hunt added a comment - Damyon / Martin. I think that the most important thing is that the size of the collapsed editor and the size of the editor is the same when you toggle. All the sizing is controlled by TinyMCE, and the way it sizes itself is burried deep in the TinyMCE code. I am not changing that. The minimal size editor works very well when editing Multiple choice questions. I would not want to see the size of the box on that form get any bigger, so I vote against any increase in the minimum size. So, what does that leave us with? The solution I made. The size of the editable region is up to TinyMCE, and that means all size specifications 1-7 give the same one-line editor. It's weird, but that's the way it is, and you (as a developer creating a form) can get an editor of the size you want if you can find the right rows= parameter. Hence I think this is the best fix possible. If you want to create something better, please re-open and assign this bug to yourself. Actually, I suggest you integrate this fix anyway, because the current behaviour is buggy, and then create a new issue assigned to yourself to fix things further.
          Hide
          Martin Dougiamas added a comment -

          I don't agree that a fix that makes every textarea with 1-7 lines collapse down to one line is a good idea just because it looks good on one page. I do agree that it's a good reason for Tim not to care.

          However, we probably all agree that the ideal is for TinyMCE to just respect the original size of the textarea, then we solve everywhere in Moodle perfectly at once. And this is pretty important as this is a MAJOR interface element for users.

          So how hard is this to solve, really?

          Going to drag in others to look.

          Show
          Martin Dougiamas added a comment - I don't agree that a fix that makes every textarea with 1-7 lines collapse down to one line is a good idea just because it looks good on one page. I do agree that it's a good reason for Tim not to care. However, we probably all agree that the ideal is for TinyMCE to just respect the original size of the textarea, then we solve everywhere in Moodle perfectly at once. And this is pretty important as this is a MAJOR interface element for users. So how hard is this to solve, really? Going to drag in others to look.
          Hide
          Tim Hunt added a comment -

          This fix does not change the fact that "every textarea with 1-7 lines collapse down to one line", That is standard TinyMCE behaviour. It has always been like that. It is probably why most of our editors are 10 or 15 'rows' in the code.

          TinyMCE does "respect the original size of the textarea" in the sense that the editor + toolbars ends up the same size as the textarea. This means that the size of the editable area is smaller than the text area that was replaced. I don't think that is a problem.

          So, in my opinion, there is nothing to solve.

          There is, however, a regression. All collapsed editors get big ugly grey bars when the page loads. I really think you should integrate this fix.

          Show
          Tim Hunt added a comment - This fix does not change the fact that "every textarea with 1-7 lines collapse down to one line", That is standard TinyMCE behaviour. It has always been like that. It is probably why most of our editors are 10 or 15 'rows' in the code. TinyMCE does "respect the original size of the textarea" in the sense that the editor + toolbars ends up the same size as the textarea. This means that the size of the editable area is smaller than the text area that was replaced. I don't think that is a problem. So, in my opinion, there is nothing to solve. There is, however, a regression. All collapsed editors get big ugly grey bars when the page loads. I really think you should integrate this fix.
          Hide
          Martin Dougiamas added a comment -

          Ah, OK, if it's true that it's always been like that for 1-7 rows, then I agree with you Tim. This should be integrated to just fix the regression from collapsed editors.

          But I still think that the size of the editable area should always start the same as the size of the textarea and we should solve that in a following issue.

          Show
          Martin Dougiamas added a comment - Ah, OK, if it's true that it's always been like that for 1-7 rows, then I agree with you Tim. This should be integrated to just fix the regression from collapsed editors. But I still think that the size of the editable area should always start the same as the size of the textarea and we should solve that in a following issue.
          Hide
          Damyon Wiese added a comment -

          Thanks Tim (and Martin for clarifying).

          This patch fixes the issue described - I'll add a new one as suggested by Martin.

          Integrated to master.

          Show
          Damyon Wiese added a comment - Thanks Tim (and Martin for clarifying). This patch fixes the issue described - I'll add a new one as suggested by Martin. Integrated to master.
          Hide
          Adrian Greeve added a comment -

          Tested on all the browsers I could get my hands on.
          The size of the editing area was the same for collapsed and non-collapsed.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on all the browsers I could get my hands on. The size of the editing area was the same for collapsed and non-collapsed. Test passed.
          Hide
          Dan Poltawski added a comment -

          Thanks! You're changes are now spread to the world through this git and our source control repositories.

          No time to rest though, we've got days to make 2.5 the best yet!

          ciao

          Show
          Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: