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

Layout regressions with tables inside mutliple choice options

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.6, 2.5.2
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      This testing needs to be done in all of Firefox, Chrome and IE.

      1. Create a multiple choice question with choices
        • {A table}
        • {A numbered list}
        • {Anything else interesting you can think of}
      2. Also set feedback for each choice containing similar things.
      3. Preview the question.
        • Ensure the preview options at the bottom of the pop-up are set to show everything.
        • Ensure that the choices are lined up OK. (Before this fix, the contents of table cells was shifted left 25px in IE and Chrome.)
      4. Select a choice and submit.
        • Check that the feedback is inside the yellow background.
      Show
      This testing needs to be done in all of Firefox, Chrome and IE. Create a multiple choice question with choices {A table} {A numbered list} {Anything else interesting you can think of} Also set feedback for each choice containing similar things. Preview the question. Ensure the preview options at the bottom of the pop-up are set to show everything. Ensure that the choices are lined up OK. (Before this fix, the contents of table cells was shifted left 25px in IE and Chrome.) Select a choice and submit. Check that the feedback is inside the yellow background.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      The text-indent: -25px; added in MDL-39420 breaks tables or lists inside multiple-choice choices in Chrome or IE. (Strangely, Firefox is not affected.)

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            quen Sam Marshall added a comment -

            Looks like the correct change to solve this problem, +1 from me

            Show
            quen Sam Marshall added a comment - Looks like the correct change to solve this problem, +1 from me
            Hide
            timhunt Tim Hunt added a comment -

            Thanks sam. Submitting for integration now.

            Show
            timhunt Tim Hunt added a comment - Thanks sam. Submitting for integration now.
            Hide
            stronk7 Eloy Lafuente (stronk7) 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
            stronk7 Eloy Lafuente (stronk7) 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
            timhunt Tim Hunt added a comment -

            Rebased.

            Show
            timhunt Tim Hunt added a comment - Rebased.
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated to master, 25 and 24 - thanks Tim.

            In interests of getting better at good/bad CSS practice i'm starting to use csslint to check things and just noting that csslint warns about adjoining classes. (Seems to be an IE6 problem, I just a

            styles.css
            4: warning at line 11, col 1
            Don't use adjoining classes.
            .que.multichoice .answer div.r0 label,
             
            styles.css
            5: warning at line 12, col 1
            Don't use adjoining classes.
            .que.multichoice .answer div.r1 label,
             
            styles.css
            6: warning at line 13, col 1
            Don't use adjoining classes.
            .que.multichoice .answer div.r0 div.specificfeedback,
             
            styles.css
            7: warning at line 14, col 1
            Don't use adjoining classes.
            .que.multichoice .answer div.r1 div.specificfeedback {
            

            Show
            poltawski Dan Poltawski added a comment - Integrated to master, 25 and 24 - thanks Tim. In interests of getting better at good/bad CSS practice i'm starting to use csslint to check things and just noting that csslint warns about adjoining classes. (Seems to be an IE6 problem, I just a styles.css 4: warning at line 11, col 1 Don't use adjoining classes. .que.multichoice .answer div.r0 label,   styles.css 5: warning at line 12, col 1 Don't use adjoining classes. .que.multichoice .answer div.r1 label,   styles.css 6: warning at line 13, col 1 Don't use adjoining classes. .que.multichoice .answer div.r0 div.specificfeedback,   styles.css 7: warning at line 14, col 1 Don't use adjoining classes. .que.multichoice .answer div.r1 div.specificfeedback {
            Hide
            timhunt Tim Hunt added a comment -

            Dan, those rules like .que.multichoice have been there for years (probably since long before IE6 was released) and no-one has ever reported a bug. So, I don't know what IE bug they are worried about, but it never bit us, and anyway we don't supported IE6 any more. So, quaint historical thingy, thanks for sharing, but let's move on (and perhaps reconfigure our CSSlint?).

            Show
            timhunt Tim Hunt added a comment - Dan, those rules like .que.multichoice have been there for years (probably since long before IE6 was released) and no-one has ever reported a bug. So, I don't know what IE bug they are worried about, but it never bit us, and anyway we don't supported IE6 any more. So, quaint historical thingy, thanks for sharing, but let's move on (and perhaps reconfigure our CSSlint?).
            Hide
            quen Sam Marshall added a comment -

            For info, here is the link about this (google result):

            https://github.com/stubbornella/csslint/wiki/Disallow-adjoining-classes

            Although Tim's response may have been a bit, er, robust, I agree this doesn't seem like it should be relevant to the 21st century. (Also, from that documentation we see that the csslint maintainer somehow thinks it's better to require you to change the html to add a new class? Uhhhhhh....)

            Show
            quen Sam Marshall added a comment - For info, here is the link about this (google result): https://github.com/stubbornella/csslint/wiki/Disallow-adjoining-classes Although Tim's response may have been a bit, er, robust, I agree this doesn't seem like it should be relevant to the 21st century. (Also, from that documentation we see that the csslint maintainer somehow thinks it's better to require you to change the html to add a new class? Uhhhhhh....)
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for your justifications, but as demonstrated by integrating it - I wasn't worried about it

            Although you 'Uhhh' about adding the new class, i'd be interested to know what a designers POV would be, it might be one of those 'programmers point of view' situations.

            Show
            poltawski Dan Poltawski added a comment - Thanks for your justifications, but as demonstrated by integrating it - I wasn't worried about it Although you 'Uhhh' about adding the new class, i'd be interested to know what a designers POV would be, it might be one of those 'programmers point of view' situations.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Sorry for the noise in testing instructions. Just trying to make it clearer whether every part needs to be tested in every browser, or just the results (i.e. do I need to create a new question for each browser, or just preview in each browser - I'm going for the latter as it appears that it's the preview that is being tested, not the question creation)

            Show
            dobedobedoh Andrew Nicols added a comment - Sorry for the noise in testing instructions. Just trying to make it clearer whether every part needs to be tested in every browser, or just the results (i.e. do I need to create a new question for each browser, or just preview in each browser - I'm going for the latter as it appears that it's the preview that is being tested, not the question creation)
            Hide
            timhunt Tim Hunt added a comment -

            Andrew, we are testing the preview.

            Dan, think about the rule you are commenting on: .que.multichoice. According to the link Sam gave, IE6 will see that as .multichoice, which is actually not a big deal. The point is, it would be presumptuous of the question bank to think that it was the only place in Moodle that might output a <div class="multichoice">, so it outputs <div class="que multichoice">, and the style rule matches the .que too.

            (Actually, the outer div gets more classes than that <div class="que multichoice adaptive correct">.)

            Now, if we were starting today we might go for class="qtype_multichoice" but Gustav Delius made these choices back in 2006. (One of the commits is 1d0f9df57ea3cca9a6fd87e66c003e1027744f6e.) and, as I said before, no-one has ever reported a bug relating to this.

            Show
            timhunt Tim Hunt added a comment - Andrew, we are testing the preview. Dan, think about the rule you are commenting on: .que.multichoice. According to the link Sam gave, IE6 will see that as .multichoice, which is actually not a big deal. The point is, it would be presumptuous of the question bank to think that it was the only place in Moodle that might output a <div class="multichoice">, so it outputs <div class="que multichoice">, and the style rule matches the .que too. (Actually, the outer div gets more classes than that <div class="que multichoice adaptive correct">.) Now, if we were starting today we might go for class="qtype_multichoice" but Gustav Delius made these choices back in 2006. (One of the commits is 1d0f9df57ea3cca9a6fd87e66c003e1027744f6e.) and, as I said before, no-one has ever reported a bug relating to this.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I'm seeing some strange issues with the .specificfeedback class not being applied correctly to some of the unusual styling I've played with - specifically tables, and <p> tags so far. Attaching a screenshot.

            Show
            dobedobedoh Andrew Nicols added a comment - I'm seeing some strange issues with the .specificfeedback class not being applied correctly to some of the unusual styling I've played with - specifically tables, and <p> tags so far. Attaching a screenshot.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Passing after discussion with Tim about the background.

            Raising a few new issues out of this though

            Show
            dobedobedoh Andrew Nicols added a comment - Passing after discussion with Tim about the background. Raising a few new issues out of this though
            Hide
            poltawski Dan Poltawski added a comment -

            Hurrah! Thanks for your contribution - this fix is part of Moodle.

            Show
            poltawski Dan Poltawski added a comment - Hurrah! Thanks for your contribution - this fix is part of Moodle.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Nov/13