Add warning for low contrast and equal colors #86

ENT8R · 2018-05-19T09:37:24Z

This PR fixes #19
Edit: it also fixes #83
I'm not sure what should be the best contrast ratio (currently it is 4 because the tests I've done with https://dequeuniversity.com/rules/axe/2.2/color-contrast returned that 4 is OK if the text is not too small...)
The amount can be changed here:
https://github.com/ENT8R/offline-qr-code/blob/819d68d403137df93eda018b10553e35e1e010fd/src/options.js#L217

rugk · 2018-05-19T10:28:54Z

Great PR! From a first look at the code it looks good.

And it seems you also fixed #83 (you may add it to the PR description here, so it is automatically closed).

As for the error message: good point to show an error if the color is exactly the same. However, you may show it even earlier when it is not at all readable by a qr code scanner at all even if some text would still be visible to the human eye with that contrast.

So could you maybe do some tests with some qr code scanners to see at which contrast ratio a qr code can still be scanned. (Or maybe you find an official document/spec, which suggests a minimum contrast)

So the levels should maybe be like:

warning: may not be scannable by, say, >50% of most qr code scanners
errors: certainly not scannable even with most good mobile qr code scanners

BTW, if you want, you can make use of the new "action buttons" I've implemented for messages. Maybe reset the last changed color? Or automatically calculate a good color, like the exact contrast color? That would make the error messages even more helpful.

Remember there is also #20, which is very similar and may be accomplished with the same code or just a minor change, or so.

rugk · 2018-05-19T11:46:37Z

src/common/common.js

@@ -732,7 +732,7 @@ const MessageHandler = (function () {// eslint-disable-line no-unused-vars
        if (typeof args[0] === "boolean") {
            isDismissable = args.shift();
        }
-        if (typeof args[0] !== undefined && args[0].text !== undefined && args[0].action !== undefined) {
+        if (typeof args[0] !== "undefined" && args[0].text !== "undefined" && args[0].action !== "undefined") {


In ES6 it is no longer needed to compare the typeof value to the string of undefined explicitly. See https://stackoverflow.com/questions/34596489/es2015-2016-way-of-typeof-varname-undefined#34596516

Mmh... But it gave me an error and when I printed typeof args[0] !== undefined it returned true even if the array was empty. And typeof args[0] !== "undefined" returned false...

Firefox 60:

And and yeah… it's wrong. As https://stackoverflow.com/questions/34596489/es2015-2016-way-of-typeof-varname-undefined#34596516 says, we may better not use that "typeof" for it anyway. In ES6 one can just check whether it is undefined directly.
So better not use typeof.

Alright. I will change that.

rugk · 2018-05-19T11:47:56Z

src/common/common.js

@@ -770,7 +770,7 @@ const MessageHandler = (function () {// eslint-disable-line no-unused-vars

            elActionButton.textContent = browser.i18n.getMessage(actionButton.text) || actionButton.text;
            elActionButton.classList.remove("invisible");
-        } else {
+        } else if (elActionButton) {


Oh, good point!

Dismiss icon (https://github.com/rugk/offline-qr-code/pull/86/files#diff-ccdc4da75e7ac1061a8cc0851e3c7122R751) needs the same adjustments. (but I can also do them by myself)

ENT8R · 2018-05-19T12:05:53Z

So could you maybe do some tests with some qr code scanners to see at which contrast ratio a qr code can still be scanned.

I just tested it with Zxing and it scanned even codes with a contrast ratio of below 2 (1 is the lowest possible value). The only problem were inverted codes (which could not be scanned at all below a contrast ratio of 5) but that should be covered with #20... I will do now some more tests with other applications.

rugk · 2018-05-19T12:55:34Z

The only problem were inverted codes (which could not be scanned at all below a contrast ratio of 5) but that should be covered with #20...

Indeed, also Zxing ("Barcode Reader") is one of the few apps, that actually support scanning inverted QR codes. You have to enable it in the options.

rugk · 2018-05-19T13:35:52Z

src/options.js

+     * @function
+     * @private
+     * @param  {string} hex
+     * @returns {Array}


this function can also return null as it seems…

rugk · 2018-05-19T13:35:54Z

src/options.js

+     */
+    function hexToRgb(hex) {
+        const result = /^#?([a-f\d]{2})([a-f\d]{2})([a-f\d]{2})$/i.exec(hex);
+        return result ? [


Uh, splitting the conditional operator on multiple lines? Better use a traditional if here.

rugk

So LGTM from a code review POV.

Other stuff I said before still outstanding.

ENT8R · 2018-05-19T16:10:57Z

Or maybe you find an official document/spec, which suggests a minimum contrast

I found no official document. All I found just said that the contrast should not be too low 😄
But based on my tests with 3 other apps besides Zxing I will show the warning when the contrast ratio is below 6 and the error if the ratio is below 3. But if somebody has other experiences, those values can be changed easily...

Or automatically calculate a good color, like the exact contrast color?

Do you mean the opposite color of the QR code (or the background)?

rugk · 2018-05-19T19:13:03Z

I will show the warning when the contrast ratio is below 6 and the error if the ratio is below 3.

Okay, so I need some practical examples for that…

This is 6:

So for the same color examples it may be okay for the user, but the QR code scanner is what matters, so I also tested some colors…

Below 2 it stopped for most of my QR code scanners:

Around 2 it differed… Funnily particularly ZXing was bad at it. Another scanner took some seconds…

Around 3 (image above) even ZXing got it, but took some seconds…

Around 4 all got it, but actually the perceieved contrast one would imagine is quite good– and it looks great from a design point of view:

However, I had a quite good light conditions (no darker screen) here, this may also come into effect.

So I think 6 is way too much to require. However, it also depends on the light source…
In general we might not want to limit that contrast ratio to integers anyway.

So maybe we need a less scary message. So can you:

make contrast return a float
introduce a simple "info message" (no warning, just a small note), for say – lower than 4.5 (that's WCAG 2.0 level AA for small text, BTW); it should also have a relaxed error message, i.e. "Your QR code may be hard to scan by some QR code readers. Consider changing it."* or so)
a warning for smaller than 3:1 (that's WCAG 2.0 level AA for large text, BTW)
and an error for <2
maybe also add the specs, where I got the numbers from as code comments, so one later sees the rationale behind choosing these

Otherwise please explain the tests you did again, I just did some few I just listed. Of course, if we want a realistic result we need more tests. Please explain the results you had in more detail, maybe we can adjust the number again or my tests were flawed (did not test low screen brightness e.g.).
I want to avoid showing too much errors and prevent some nice-looking QR codes, but of course they should stil be scannable by the majority of QR code scanners in most of the situations.

* When you implement an action button, the term "Consider changing it" must not be mentioned in the message, because it would be redundant – the action button already does it and is more useful than a general statement.

Do you mean the opposite color of the QR code (or the background)?

Yeah.

rugk · 2018-05-19T19:27:33Z

BTW this color stuff get's much if you want to also implement support for opposite color there. So maybe you could split that color stuff (only that color caluclation/etc.) into it's own module. (Could be in commons.js or in options.js – does not really matter, as we currently won't use it anywhere else than in the options, but it will be restructured into import modules of Firefox 60 anyway, soon.)

rugk · 2018-05-20T10:22:44Z

src/common/common.js

@@ -732,7 +732,7 @@ const MessageHandler = (function () {// eslint-disable-line no-unused-vars
        if (typeof args[0] === "boolean") {
            isDismissable = args.shift();
        }
-        if (typeof args[0] !== undefined && args[0].text !== undefined && args[0].action !== undefined) {
+        if (args[0] !== undefined && args[0].text !== "undefined" && args[0].action !== "undefined") {


BTW I cherry-picked this change into the master as it is just a bug fix for a issue I introduced. But BTW the last undefines do not need (or maybe even should not) be quoted. (typeof is not even used there).

Basically I suggest you to merge the master into my branch, that should fix these conflicts.

ENT8R · 2018-05-22T11:42:27Z

It was a little bit more work than I thought but now there is a small button next to each message which (when clicked) calculates the complementary color of the QR code and sets the calculated color as the new background of the code automatically.

Otherwise please explain the tests you did again, I just did some few I just listed.

I just donwloaded 3 different QR code scanner apps and tried some random settings while seeing the contrast ratio value in the console. I think your tests were more extensive...

1. red errors need a white text on the action button 2. if there is few space for the action button, it should not overflow

rugk

So I fixed two display issues.

Apart from that, there is one bigger: If you do it in the wrong way, you can display multiple messages:

rugk · 2018-05-22T11:54:38Z

src/options.js

            } else {
+                MessageHandler.hideInfo();


Hmm, this could get problematic, because we also show a permanent "managed setting" info message when this is used (see OptionHandler.showManagedInfo).

What we could do about this:

check managedInfoIsShown and do not show or hide the info message 8easy, but not really nice)

use a custom element for that managed info thing – this is an information, which stays there, anyway. (the way to go, IMHO)

implement Extend MessageHandler for showing multiple messages #9 (too hard and not needed for this, IMHO)

So I can do the second version after your PR is merged.

rugk · 2018-05-22T11:54:42Z

src/options.js

+                    browser.storage.sync.set({
+                        "qrBackgroundColor": invertedColor
+                    }).catch((error) => {
+                        Logger.logError("could not save option", option, ": ", error);


Great you added this button! As for this line just one comment: As the logger passes this to console anyway, I think you do not need to add a space after the :. All values are separated automatically (with a space as it seems) by the browser.

rugk · 2018-05-22T11:56:07Z

src/common/common.js

@@ -1372,6 +1372,104 @@ const RandomTips = (function () {// eslint-disable-line no-unused-vars
    return me;
 })();

+const Colors = (() => { // eslint-disable-line no-unused-vars


Yeah, this module looks great! So for doing #20 later, everything is set.

rugk · 2018-05-22T12:05:44Z

src/_locales/en/messages.json

    "description": "The message shown when the color for the QR code and the background are equal"
  },
+  "messageAutoSelectColorButton": {
+    "message": "Calculate best contrast",


MAybe you can try to shorten this a bit?

Instead of calculate maybe just "Use". (That the JS has to calculate this is a technical detail the user does not even want to know. 😄)
And instead of contrast maybe contrasting color? Or something else like "best color"? Remember it does not matter to be technically 100% accurate here, the user should just know what happens when they click the button.

Theoretically it could e.g. also be okay for the user to get some color variation (i.e. not 100%ly the contrast color, but a color, which has enough contrast, so the message goes away.)

rugk · 2018-05-22T12:09:17Z

src/options.js

+                }
+            };
+
+            // breakpoints: https://github.com/rugk/offline-qr-code/pull/86#issuecomment-390426286


Oh and once we are at it. You can link to my comment, yes, but please still add comments for the colors, i.e. the accessibility guidelines I mentioned.

Or even better remember the "no magic numbers" rules. So put these numbers into constants. As they are color stuff not (only) related to the options handler, the best would be to use these as constants in the Colors module, i.e. make them "public constants". (add to me in that module)

rugk · 2018-05-22T12:23:39Z

src/_locales/de/messages.json

+    "desription": "The message shown when the contrast ratio is too low"
+  },
+  "lowContrastRatioWarning": {
+    "message": "Das Kontrastverhältnis zwischen der Farbe des QR-Codes und dem Hintergrund ist wahrscheinlich zu niedrig um von den meisten QR-Code-Scannern erkannt zu werden",


shorter:
das Kontrastverhältnis -> der Kontrast
is "wahrscheinlich" neccessary? Maybe just use subjunctive ("Konjunktiv") here, too (as in the English version).

grammar:
dem Hintergrund -> dessen Hintergrund

In general you may reword it. Keep the fact that it may not be scanned by many QR code readers.

rugk · 2018-05-22T12:25:25Z

src/options.js

                MessageHandler.showWarning("lowContrastRatioWarning", false, actionButton);
            } else if (colorContrast <= 4.5) {
+                // show only an info when the contrast is low but most of the scanners can still read it


Okay, I rather thought about "WCAG 2.0 level AA" (so please add that), but this information is also good.

Still prefer them as constants though. 😄

ENT8R · 2018-05-22T12:46:00Z

Apart from that, there is one bigger: If you do it in the wrong way, you can display multiple messages:

Really? How does that work? I can't find the issue in the code currently 😄...

See FirefoxUX/photon#305

…nto contrast

rugk · 2018-05-22T12:58:07Z

src/common/common.js

+    const me = {};
+
+    me.ContrastBreakpoints = {
+        A: 2,


No my numbers were not all the A/AA/AAA things…

But I'll change that by myself.

Small usage change: now uses 3.1 instead 3 for warning, but that should hardly matter.

rugk · 2018-05-22T13:20:31Z

You just change the colors again and again and thus "fix" them. When one message is shown, it does not go away unless you got no message at all… 😄

BTW I changed the constant there. And I could not find any contrast ratio definition for WCAG level A, I guess they start at level AA for contrasts.

rugk · 2018-05-28T19:35:12Z

Any ETA on this?

ENT8R · 2018-05-29T12:15:42Z

Any ETA on this?

I think with the latest commit I fixed the issue where multiple messages where shown. Is there still something to do? I could not find anything...

rugk · 2018-05-29T12:38:33Z

No, all right. As for an explicit message box for only these messages, I'll add this later, as said.

rugk · 2018-05-31T18:27:30Z

Merged in 313a5f6

In 313a5f6 the PR #86 was not really merged in a good way.

In rugk@313a5f6 the PR rugk#86 was not really merged in a good way.

Add warning for low contrast and equal colors

819d68d

rugk requested changes May 19, 2018

View reviewed changes

Add another if-check and remove 'typeof'

10bfa6b

rugk requested changes May 19, 2018

View reviewed changes

Fix suggestions by @rugk

323ad34

rugk approved these changes May 19, 2018

View reviewed changes

Change the breakpoints when the warnings and errors are shown

1b53467

rugk reviewed May 20, 2018

View reviewed changes

ENT8R added 2 commits May 22, 2018 13:38

Added a button to automatically set the right color

9d6e4cb

Merge branch 'master' into contrast

da16700

Fix two display errors

de319c2

1. red errors need a white text on the action button 2. if there is few space for the action button, it should not overflow

rugk requested changes May 22, 2018

View reviewed changes

Add some comments and remove a whitespace

9dae10d

rugk requested changes May 22, 2018

View reviewed changes

rugk reviewed May 22, 2018

View reviewed changes

Reword message, use constants for breakpoints, adjust comments

7627beb

rugk added 2 commits May 22, 2018 14:55

Fix micro button display issues

39cc907

See FirefoxUX/photon#305

Merge branch 'contrast' of https://github.com/ENT8R/offline-qr-code i…

f0009f5

…nto contrast

rugk reviewed May 22, 2018

View reviewed changes

Adjust contrast definition

496edca

Small usage change: now uses 3.1 instead 3 for warning, but that should hardly matter.

Fix an issue where multiple messages where shown

33a661a

rugk approved these changes May 29, 2018

View reviewed changes

rugk closed this May 31, 2018

This was referenced May 31, 2018

Implement (styling of) warning message #83

Closed

Warning when settings are low-contrast for QR codes #19

Closed

Extend MessageHandler for showing multiple messages #9

Closed

ENT8R deleted the contrast branch June 2, 2018 08:49

rugk added a commit that referenced this pull request Jun 2, 2018

Fix problematic merge of PR

a4694bc

In 313a5f6 the PR #86 was not really merged in a good way.

rugk mentioned this pull request Aug 5, 2018

Contrast color algorithm #117

Closed

rugk mentioned this pull request Aug 18, 2018

So does it invert brightness or only hue? onury/invert-color#13

Closed

daaAd1 pushed a commit to daaAd1/offline-qr-code that referenced this pull request Sep 30, 2019

Fix problematic merge of PR

88b7dc4

In rugk@313a5f6 the PR rugk#86 was not really merged in a good way.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning for low contrast and equal colors #86

Add warning for low contrast and equal colors #86

ENT8R commented May 19, 2018 •

edited

Loading

rugk commented May 19, 2018 •

edited

Loading

rugk May 19, 2018

ENT8R May 19, 2018 •

edited

Loading

rugk May 19, 2018

rugk May 19, 2018

ENT8R May 19, 2018

rugk May 19, 2018

ENT8R commented May 19, 2018

rugk commented May 19, 2018

rugk May 19, 2018

rugk May 19, 2018

rugk left a comment •

edited

Loading

ENT8R commented May 19, 2018

rugk commented May 19, 2018 •

edited

Loading

rugk commented May 19, 2018

rugk May 20, 2018

ENT8R commented May 22, 2018

rugk left a comment

rugk May 22, 2018

rugk May 22, 2018

rugk May 22, 2018

rugk May 22, 2018

rugk May 22, 2018

rugk May 22, 2018 •

edited

Loading

rugk May 22, 2018

ENT8R commented May 22, 2018

rugk May 22, 2018

rugk commented May 22, 2018

rugk commented May 28, 2018

ENT8R commented May 29, 2018 •

edited

Loading

rugk commented May 29, 2018

rugk commented May 31, 2018

Add warning for low contrast and equal colors #86

Add warning for low contrast and equal colors #86

Conversation

ENT8R commented May 19, 2018 • edited Loading

rugk commented May 19, 2018 • edited Loading

Choose a reason for hiding this comment

ENT8R May 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ENT8R commented May 19, 2018

rugk commented May 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugk left a comment • edited Loading

Choose a reason for hiding this comment

ENT8R commented May 19, 2018

rugk commented May 19, 2018 • edited Loading

rugk commented May 19, 2018

Choose a reason for hiding this comment

ENT8R commented May 22, 2018

rugk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugk May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ENT8R commented May 22, 2018

Choose a reason for hiding this comment

rugk commented May 22, 2018

rugk commented May 28, 2018

ENT8R commented May 29, 2018 • edited Loading

rugk commented May 29, 2018

rugk commented May 31, 2018

ENT8R commented May 19, 2018 •

edited

Loading

rugk commented May 19, 2018 •

edited

Loading

ENT8R May 19, 2018 •

edited

Loading

rugk left a comment •

edited

Loading

rugk commented May 19, 2018 •

edited

Loading

rugk May 22, 2018 •

edited

Loading

ENT8R commented May 29, 2018 •

edited

Loading