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

row highlighting in tables missing from core base/canvas themes.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.4
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      1) Test Standard theme for tables (user accounts page) and see that they are highlighted between rows.
      2) Test that no graphic conflicts occur due to highlighting.

      Show
      1) Test Standard theme for tables (user accounts page) and see that they are highlighted between rows. 2) Test that no graphic conflicts occur due to highlighting.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-31847_master

      Description

      while on MDL-31712, Daniele Cordella mentioned that row highlighting should be in core themes. It makes sense too, but i couldn't find a relevant issue so here it is.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Side note: Some themes, Anomaly, for example and also Formal White (since MDL-31712) already have own alternation CSS selectors.

            Also, I'm not sure if base or canvas should define them at all (or perhaps yes defaulting to bg-color = none or so), just to allow others to know about their existance.

            Or, alternatively perhaps all core themes should specify them, but always independently. From my knowledge base and canvas use to be color-free (detail-free). And this, although really important because it helps a lot visually, it's just one color detail.

            So, sided-mind here... I leave this on expert hands (anybody but me). Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Side note: Some themes, Anomaly, for example and also Formal White (since MDL-31712 ) already have own alternation CSS selectors. Also, I'm not sure if base or canvas should define them at all (or perhaps yes defaulting to bg-color = none or so), just to allow others to know about their existance. Or, alternatively perhaps all core themes should specify them, but always independently. From my knowledge base and canvas use to be color-free (detail-free). And this, although really important because it helps a lot visually, it's just one color detail. So, sided-mind here... I leave this on expert hands (anybody but me). Ciao
            Hide
            daniss Daniele Cordella added a comment - - edited

            Yes, Eloy.
            I thought about the same problem.
            The alternation of colors is a general issue but the particular color of each theme is a not general at all.
            I see two ways:
            -> the one you proposed: all core themes should specify the row highlight independently.
            -> each theme has to provide two colors in its setting page (or hardcoded in its config) and the general highlight will use them

            IMHO your solution is faster unless deeper afterthought about themes is going to come.

            Show
            daniss Daniele Cordella added a comment - - edited Yes, Eloy. I thought about the same problem. The alternation of colors is a general issue but the particular color of each theme is a not general at all. I see two ways: -> the one you proposed: all core themes should specify the row highlight independently. -> each theme has to provide two colors in its setting page (or hardcoded in its config) and the general highlight will use them IMHO your solution is faster unless deeper afterthought about themes is going to come.
            Hide
            nebgor Aparup Banerjee added a comment -

            i think the row colour alternation is a general accessibility issue so this should be addressed in core - as far as readability should be ensured.

            Show
            nebgor Aparup Banerjee added a comment - i think the row colour alternation is a general accessibility issue so this should be addressed in core - as far as readability should be ensured.
            Show
            daniss Daniele Cordella added a comment - - edited I wrote a proposal of general solution in: https://github.com/kordan/moodle/tree/MDL-31712_generalsolution_master or here: https://github.com/kordan/moodle/compare/master...MDL-31712_generalsolution_master if you prefer
            Hide
            daniss Daniele Cordella added a comment -

            Aparup and Mary
            I wrote my proposal of general management of each color related problem in http://moodle.org/mod/forum/discuss.php?d=201604

            Show
            daniss Daniele Cordella added a comment - Aparup and Mary I wrote my proposal of general management of each color related problem in http://moodle.org/mod/forum/discuss.php?d=201604
            Hide
            lazydaisy Mary Evans added a comment -

            Thanks Danielle

            Show
            lazydaisy Mary Evans added a comment - Thanks Danielle
            Hide
            lazydaisy Mary Evans added a comment -

            This issue had never been Triaged so this has not been picked up and could do with going into 2.4...

            It's triaged now.

            Show
            lazydaisy Mary Evans added a comment - This issue had never been Triaged so this has not been picked up and could do with going into 2.4... It's triaged now.
            Hide
            poltawski Dan Poltawski added a comment -

            I've integrated this now, thanks Mary.

            Show
            poltawski Dan Poltawski added a comment - I've integrated this now, thanks Mary.
            Hide
            markn Mark Nelson added a comment -

            At first I did not notice a difference in colour whatsoever. I then looked at the commit and then at the CSS being applied on the page for rows r0 and r1 and saw they were using #F0F0F0 and #FAFAFA specified in the diff. However, really these colours appear identical. I am failing because I do not believe the difference in colours is enough for the human eye to distinguish between the two, not because the logic failed.

            Show
            markn Mark Nelson added a comment - At first I did not notice a difference in colour whatsoever. I then looked at the commit and then at the CSS being applied on the page for rows r0 and r1 and saw they were using #F0F0F0 and #FAFAFA specified in the diff. However, really these colours appear identical. I am failing because I do not believe the difference in colours is enough for the human eye to distinguish between the two, not because the logic failed.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            Mark, I think you may need to get yours eyes tested. This is a perfect combination of row background colour.
            See attached image.

            Show
            lazydaisy Mary Evans added a comment - - edited Mark, I think you may need to get yours eyes tested. This is a perfect combination of row background colour. See attached image.
            Hide
            poltawski Dan Poltawski added a comment -

            Mary: I looked at this too, but I couldn't see it either!

            Show
            poltawski Dan Poltawski added a comment - Mary: I looked at this too, but I couldn't see it either!
            Hide
            lazydaisy Mary Evans added a comment -

            Do either of you see the light-grey background when you hover over the comments here? Because that is the same colour #F0F0F0 which is what I have for row r0. What if I keep that for r0 and make the r1 row white? Would that be better?

            Show
            lazydaisy Mary Evans added a comment - Do either of you see the light-grey background when you hover over the comments here? Because that is the same colour #F0F0F0 which is what I have for row r0. What if I keep that for r0 and make the r1 row white? Would that be better?
            Hide
            daniss Daniele Cordella added a comment - - edited

            There are two versions of the Web Content Accessibility Guidelines to verify if two colors are well contrasted or not.
            I googled "html accessibility color contrast analyser" and, for instance, I found: http://www.colorsontheweb.com/colorcontrast.asp
            That site declares that #F0F0F0 and #FAFAFA are not ok.

            Show
            daniss Daniele Cordella added a comment - - edited There are two versions of the Web Content Accessibility Guidelines to verify if two colors are well contrasted or not. I googled "html accessibility color contrast analyser" and, for instance, I found: http://www.colorsontheweb.com/colorcontrast.asp That site declares that #F0F0F0 and #FAFAFA are not ok.
            Hide
            lazydaisy Mary Evans added a comment -

            Un'alternativa sarebbe bene allora

            Show
            lazydaisy Mary Evans added a comment - Un'alternativa sarebbe bene allora
            Hide
            lazydaisy Mary Evans added a comment -

            @Dan

            I've altered this so only r0 uses the background-color, this measn r1 will retain the default color (white).

            Show
            lazydaisy Mary Evans added a comment - @Dan I've altered this so only r0 uses the background-color, this measn r1 will retain the default color (white).
            Hide
            poltawski Dan Poltawski added a comment -

            Sam pointed out that this rule is defined in standard theme:
            .generaltable .cell

            {background-color: #FFFFFF;}
            Show
            poltawski Dan Poltawski added a comment - Sam pointed out that this rule is defined in standard theme: .generaltable .cell {background-color: #FFFFFF;}
            Hide
            poltawski Dan Poltawski added a comment -

            I'm passing this based on other themes, I think we need to deal with the issues from this change in a new issue.

            Show
            poltawski Dan Poltawski added a comment - I'm passing this based on other themes, I think we need to deal with the issues from this change in a new issue.
            Hide
            poltawski Dan Poltawski added a comment -

            Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

            ciao
            Dan

            Show
            poltawski Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  3/Dec/12