Change address / amount error background

[w0xlt]

This PR proposes a small change in QLineEdit when there is an error in the input.

master

PR

This also shows good results when combined with other open PRs.

#537

#533

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jarolrod, shaavan, GBKS

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

[luke-jr]

This also shows good results when combined with other open PRs.

#537

Well, now the background would be theme-dependent, and #537 will need to figure out a reasonable contrasting colour...

[promag]

IMO this improves text contrast.

However, other improvements could be made as well, some suggestions:

• consistent error message near the invalid fields, for instance, the error message for invalid change address is displayed on the right while the upcoming bech32 error shows on the top
• the amount field only turns red when clicking send and it's zero
• the send button remains enabled while a field is invalid, maybe it should be disabled

[jarolrod]

ACK fe7c81e

I'm ACK on this change specifically, there could be an accessibility improvement to this change (ping @RandyMcMillan)

I disagree that, in the case of #537, this "shows good results when combined with other open PRs" :)

[w0xlt]

consistent error message near the invalid fields, for instance, the error message for invalid change address is displayed on the right while the upcoming bech32 error shows on the top

@promag I addressed this suggestion in #560

[RandyMcMillan]

@jarolrod - thanks for pinging me on this - my apologizes for not seeing it sooner...

@w0xlt,

Just to define the issue with more clarity,

We can see using this color contrast calculator that in the light theme (on Mac) that the black text on the rgb(255,128,128) is "ok" kinda... :)

But when we switch to the dark theme (on Mac) the contrast becomes unsatisfactory from a contrast perspective and is apparent just by looking at it.

[RandyMcMillan]

When fe7c81e is tested against light/dark theme changes (on Mac) we get...

which doesn't work because the text color is changed based on system theme...

[RandyMcMillan]

[RandyMcMillan]

Here are a couple patches to experiment with palleteRole in Qt:
You will have to call the text field to update during the system theme change - other wise it will only update when it receives focus.

diff --git a/src/qt/guiconstants.h b/src/qt/guiconstants.h
index 60e8ee2420..8b37c5dfb4 100644
--- a/src/qt/guiconstants.h
+++ b/src/qt/guiconstants.h
@@ -25,7 +25,7 @@ static const int STATUSBAR_ICONSIZE = 16;
 static const bool DEFAULT_SPLASHSCREEN = true;
 
 /* Invalid field background style */
-#define STYLE_INVALID "border: 3px solid #FF8080"
+#define STYLE_INVALID "color: palette(highlighted-text); background-color: pallet(shadow); border: 3px solid #FF8080"
 
 /* Transaction list -- unconfirmed transaction */
 #define COLOR_UNCONFIRMED QColor(128, 128, 128)

diff --git a/src/qt/guiconstants.h b/src/qt/guiconstants.h
index 60e8ee2420..dca896ca7e 100644
--- a/src/qt/guiconstants.h
+++ b/src/qt/guiconstants.h
@@ -25,7 +25,7 @@ static const int STATUSBAR_ICONSIZE = 16;
 static const bool DEFAULT_SPLASHSCREEN = true;
 
 /* Invalid field background style */
-#define STYLE_INVALID "border: 3px solid #FF8080"
+#define STYLE_INVALID "color: palette(highlighted-text); background-color: pallet(window); border: 3px solid #FF8080"
 
 /* Transaction list -- unconfirmed transaction */
 #define COLOR_UNCONFIRMED QColor(128, 128, 128)

[w0xlt]

@RandyMcMillan Thanks for the detailed review and suggestions.
I will apply and test them.

[jarolrod]

Revisited this, and it is in fact an improvement. Specifically, there's is an improvement for text contrast when in dark mode. Additionally it makes the UI language around a sending error consistent.

improvement with text contrast in dark mode:

master	PR

[shaavan]

ACK fe7c81e

• This change has improved over the current way of handling erroneous values.
• Objectively, this PR shows definite accessibility improvements, as shown by @RandyMcMillan's thorough review.
• On a subjective note, I find the PR's way of handling errors cleaner than the master's way.

Screenshot:

Master	PR

[w0xlt]

@RandyMcMillan I tried your suggestions (palette(window), palette(shadow)) and also palette(base), but none of them worked.
However I didn't test on MacOS, I did it on Ubuntu by setting the theme using Qt5 Settings.

[Rspigler]

I actually prefer the red highlighting. IMO it's more contrasting and more of an alert

[DrahtBot]

There hasn't been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

[hebasto]

@GBKS

Mind sharing your designer's opinion regarding this PR approach.

[DrahtBot]

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

[GBKS]

ACK fe7c81e

Just got the DrahtBot notification and realized I was asked for my input. Sorry, for the delay on this. I agree with this change. The text remains more legible. Also visually less jarring with the outline.

If one wanted to keep a red background, it could also be a red with transparency (15-25% might be enough). That would avoid the thick border, still draw enough attention, and not impact contrast so much.