Change address / amount error background

w0xlt commented at 9:51 pm on February 18, 2022: contributor

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

qt: change QLineEdit error background fe7c81e34e

DrahtBot commented at 11:53 pm on February 18, 2022: contributor

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.

DrahtBot cross-referenced this on Feb 18, 2022 from issue Point out position of invalid characters in Bech32 addresses by luke-jr

DrahtBot cross-referenced this on Feb 18, 2022 from issue Network Watch tool by luke-jr

luke-jr commented at 1:26 am on February 19, 2022: member

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 commented at 0:01 am on February 22, 2022: contributor

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 commented at 7:13 am on February 23, 2022: member

ACK fe7c81e34e2e16c4a5ec967645ebb49e161d3a25

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 cross-referenced this on Feb 26, 2022 from issue Make error message layout consistent by w0xlt

w0xlt commented at 5:13 am on February 26, 2022: contributor

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 https://github.com/bitcoin-core/gui/pull/560

DrahtBot cross-referenced this on Mar 2, 2022 from issue Warn when sending to already-used Bitcoin addresses by luke-jr

jarolrod added the label UI on Mar 10, 2022

RandyMcMillan commented at 10:21 am on March 16, 2022: contributor

@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… :)

Screen Shot 2022-03-16 at 5 59 59 AM

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.

Screen Shot 2022-03-16 at 6 00 29 AM

RandyMcMillan commented at 10:50 am on March 16, 2022: contributor

When fe7c81e34e2e16c4a5ec967645ebb49e161d3a25 is tested against light/dark theme changes (on Mac) we get…

Screen Shot 2022-03-16 at 6 41 19 AM

Screen Shot 2022-03-16 at 6 42 05 AM

which doesn’t work because the text color is changed based on system theme…

RandyMcMillan commented at 11:32 am on March 16, 2022: contributor

Screen Shot 2022-03-16 at 7 26 55 AM

Screen Shot 2022-03-16 at 7 27 09 AM

Screen Shot 2022-03-16 at 7 27 35 AM

RandyMcMillan commented at 11:36 am on March 16, 2022: contributor

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.

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

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

w0xlt commented at 4:57 am on April 12, 2022: contributor

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

jarolrod approved

jarolrod commented at 2:10 am on June 28, 2022: member

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 approved

shaavan commented at 12:38 pm on July 1, 2022: contributor

ACK fe7c81e34e2e16c4a5ec967645ebb49e161d3a25

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 commented at 12:02 pm on July 14, 2022: contributor

@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 commented at 9:49 pm on August 1, 2022: contributor

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

DrahtBot commented at 1:48 am on July 16, 2023: contributor

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.

DrahtBot added the label CI failed on Jul 28, 2023

DrahtBot removed the label CI failed on Aug 7, 2023

DrahtBot added the label CI failed on Aug 18, 2023

DrahtBot removed the label CI failed on Aug 23, 2023

DrahtBot added the label CI failed on Aug 31, 2023

DrahtBot removed the label CI failed on Sep 4, 2023

DrahtBot added the label CI failed on Sep 13, 2023

DrahtBot removed the label CI failed on Sep 18, 2023

hebasto commented at 2:50 pm on September 22, 2023: member

@GBKS

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

DrahtBot added the label CI failed on Oct 18, 2023

DrahtBot removed the label CI failed on Oct 21, 2023

DrahtBot added the label CI failed on Oct 25, 2023

DrahtBot commented at 2:34 pm on February 5, 2024: contributor

🤔 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 commented at 2:41 pm on February 5, 2024: none

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.

hebasto merged this on Feb 7, 2024

hebasto closed this on Feb 7, 2024

Change address / amount error background #553

Reviews

Conflicts