Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.4
    • Fix Version/s: 2.4
    • Component/s: Accessibility, Themes
    • Labels:
    • Testing Instructions:
      Hide
      • Using Firefox on moodle 2.3, create a text page that contains <div class="blink">This text is blinking</div>
      • Check the text is blinking (Webkit doesn't seem to support the blinking text decoration)
      • Upgrade moodle to master and check the text is no longer blinking
      Show
      Using Firefox on moodle 2.3, create a text page that contains <div class="blink">This text is blinking</div> Check the text is blinking (Webkit doesn't seem to support the blinking text decoration) Upgrade moodle to master and check the text is no longer blinking
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-30898-master
    • Rank:
      33906

      Description

      There are CSS rules defined for .blink (blinking text) and .justifytext (fully justified text). It is not apparent that either of these rules are being used in the standard theme, but they should not be used in most cases as they both have accessibility problems.

        Activity

        Hide
        Mary Evans added a comment - - edited

        I've just got notice about this issue, because it was automatically assigned to me, but changed to Jason. However I would just like to point out that "justify" in theme/standard/style/core.css refers to the CSS property attributed to the class selector "justifytext" in two places, which is correct, so I don't think this should be changed. If there has to be a change, then one needs to look for the "justifytext" class selector in Moodle core, as it is probably hard coded there.

        I found only one reference to "blink", which again is styling a class selector in this case "blink", which is correct, so again I do not think this should be changed in the CSS. If changes need making this should be done in core code as explained above.

        So my advice to Jason is "proceed with caution".

        Show
        Mary Evans added a comment - - edited I've just got notice about this issue, because it was automatically assigned to me, but changed to Jason. However I would just like to point out that "justify" in theme/standard/style/core.css refers to the CSS property attributed to the class selector "justifytext" in two places, which is correct, so I don't think this should be changed. If there has to be a change, then one needs to look for the "justifytext" class selector in Moodle core, as it is probably hard coded there. I found only one reference to "blink", which again is styling a class selector in this case "blink", which is correct, so again I do not think this should be changed in the CSS. If changes need making this should be done in core code as explained above. So my advice to Jason is "proceed with caution".
        Hide
        Jason Fowler added a comment -

        Thanks Mary, I had planned to scour the core looking for use of these classes, and see if they are used, if they aren't, there is no need to do anything really, I may remove the .blink as there is no call for that kind of horror in moodle, but the .justifytext should be fine, I don't see how justified text is inaccessible.

        Show
        Jason Fowler added a comment - Thanks Mary, I had planned to scour the core looking for use of these classes, and see if they are used, if they aren't, there is no need to do anything really, I may remove the .blink as there is no call for that kind of horror in moodle, but the .justifytext should be fine, I don't see how justified text is inaccessible.
        Hide
        Jason Fowler added a comment -

        As stated, I can see no where that these CSS rules are using in any of the themes shipped with Moodle.

        The only thing that concerns me is this:

        theme/base/style/core.css:.blink

        {text-decoration: blink;}

        I will raise the issue in the dev chat and see what comes up.

        Show
        Jason Fowler added a comment - As stated, I can see no where that these CSS rules are using in any of the themes shipped with Moodle. The only thing that concerns me is this: theme/base/style/core.css:.blink {text-decoration: blink;} I will raise the issue in the dev chat and see what comes up.
        Hide
        Rossiani Wijaya added a comment -

        This look fine.

        Does this need to be backported to 2.2 and 2.3?

        Show
        Rossiani Wijaya added a comment - This look fine. Does this need to be backported to 2.2 and 2.3?
        Hide
        Jason Fowler added a comment -

        I think, because it is removing CSS, this should be master only, as it could potentially break non-core themes...

        Show
        Jason Fowler added a comment - I think, because it is removing CSS, this should be master only, as it could potentially break non-core themes...
        Hide
        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
        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
        Jason Fowler added a comment -

        rebased

        Show
        Jason Fowler added a comment - rebased
        Hide
        Aparup Banerjee added a comment -

        Hi Jason,
        thoughts on this:

        • i can understand how it might break accessibility being blinking and all but it would be good for posterity reasons to specify what exactly was broken here. Does a screen reader read only when the text is visible?
        • title of MDL needs updating to define this actual issue.
        • as this is Themes related, i'll wait for Mary to add any +1's
        • The test instructions need to go about some regression test such as, a gross and very vague example is. " create a example page or content or custom theme css that uses the blink (in 23?). backup and restore into master. does it break? are there any regressions?"

        aside from the above, this looks like its good to go in.

        Show
        Aparup Banerjee added a comment - Hi Jason, thoughts on this: i can understand how it might break accessibility being blinking and all but it would be good for posterity reasons to specify what exactly was broken here. Does a screen reader read only when the text is visible? title of MDL needs updating to define this actual issue. as this is Themes related, i'll wait for Mary to add any +1's The test instructions need to go about some regression test such as, a gross and very vague example is. " create a example page or content or custom theme css that uses the blink (in 23?). backup and restore into master. does it break? are there any regressions?" aside from the above, this looks like its good to go in.
        Hide
        Mary Evans added a comment -

        As Jason pointed out there is no reason to have the blink CSS if the class selector is not used.
        Why it was added in the first palce is a mystery. A fall back from Moodle 1.9 perhaps? Anyway blink is bad, I agree.

        So happy to go with this.

        Thanks
        Mary

        Show
        Mary Evans added a comment - As Jason pointed out there is no reason to have the blink CSS if the class selector is not used. Why it was added in the first palce is a mystery. A fall back from Moodle 1.9 perhaps? Anyway blink is bad, I agree. So happy to go with this. Thanks Mary
        Hide
        Mary Evans added a comment -

        @Aparup

        I agree that the title of this issue should be changed. May be to something like remove redundant blink CSS in Base theme! That way this would not need testing. However to test it you could try adding something like <span class="blink">BLINKING TEXT!</span> in Site Topic or a Forum or Section 0 in a course page.

        I hope that answers your question?

        Show
        Mary Evans added a comment - @Aparup I agree that the title of this issue should be changed. May be to something like remove redundant blink CSS in Base theme! That way this would not need testing. However to test it you could try adding something like <span class="blink">BLINKING TEXT!</span> in Site Topic or a Forum or Section 0 in a course page. I hope that answers your question?
        Hide
        Mary Evans added a comment -

        I have just tested this in Themes forum...

        http://moodle.org/mod/forum/discuss.php?d=211026#p919889

        Show
        Mary Evans added a comment - I have just tested this in Themes forum... http://moodle.org/mod/forum/discuss.php?d=211026#p919889
        Hide
        Jason Fowler added a comment -

        Blinking Text:
        Blinking Text can easily get missed by some screen reading devices.

        Show
        Jason Fowler added a comment - Blinking Text: Blinking Text can easily get missed by some screen reading devices.
        Hide
        Jason Fowler added a comment -

        All updated Aparup, thanks for the feedback

        Show
        Jason Fowler added a comment - All updated Aparup, thanks for the feedback
        Hide
        Aparup Banerjee added a comment - - edited

        Thanks all, that has been integrated into master only

        Show
        Aparup Banerjee added a comment - - edited Thanks all, that has been integrated into master only
        Hide
        Adrian Greeve added a comment -

        I created a test file in 2.3. The text was blinking. I upgraded to master and the text stopped blinking.
        It seems to work to me.
        Test passed.

        Show
        Adrian Greeve added a comment - I created a test file in 2.3. The text was blinking. I upgraded to master and the text stopped blinking. It seems to work to me. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio

        This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

        Thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: