Fonts KCM: Fix text readability regression
ClosedPublic

Authored by rkflx on Jun 6 2018, 9:49 PM.

Details

Summary

Porting the Fonts KCM to QML in 24b960f92284 set the state of the font
preview labels to disabled, resulting in a light gray text with too
little contrast to enable a usable preview of the chosen font.

In the QWidgets based KCM the following code was used:

QLabel::setFrameStyle(QFrame::StyledPanel | QFrame::Sunken)

As that property is not available from QML, we use an alternative
approach by setting the state to readOnly. To also indicate
visually that nothing can be typed into the textedit, we add
Kirigami.Theme.inherit: true which results in changing the
background from white to grey.

Test Plan

Font previews for each category in kcmshell5 kcm_fonts should be
readable again.

Before:


After:

How it looked like in Plasma 5.12:

Diff Detail

Repository
R119 Plasma Desktop
Branch
fonts-kcm-readability (branched from Plasma/5.13)
Lint
No Linters Available
Unit
No Unit Test Coverage
rkflx created this revision.Jun 6 2018, 9:49 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 6 2018, 9:49 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
rkflx requested review of this revision.Jun 6 2018, 9:49 PM
Zren added a subscriber: Zren.Jun 6 2018, 11:48 PM

Would the TextField.readOnly property be useful here?

http://doc.qt.io/qt-5/qml-qtquick-controls-textfield.html#readOnly-prop

rkflx added a comment.Jun 7 2018, 6:23 AM
In D13390#275034, @Zren wrote:

Would the TextField.readOnly property be useful here?

http://doc.qt.io/qt-5/qml-qtquick-controls-textfield.html#readOnly-prop

I already tried that, but as you can see in your screenshot, visually readonly still indicates to the user that the field can be edited. Unfortunately that's not what I think is needed here.

broulik added a subscriber: broulik.Jun 7 2018, 6:27 AM

Unfortunately that's not what I think is needed here.

Why not? You make the text field look as though it is enabled, which is what readOnly does. Still being able to select text isn't too bad, is it?

rkflx added a comment.Jun 7 2018, 6:30 AM

Unfortunately that's not what I think is needed here.

Why not? You make the text field look as though it is enabled, which is what readOnly does. Still being able to select text isn't too bad, is it?

It will lead users to click into the field, expecting to be able to edit the name directly. If that was the case, we could show that visually, but it's not, so we shouldn't either.

It will lead users to click into the field, expecting to be able to edit the name directly

Oh, you mean because the background turns white when enabled? I see. Please use

color: Kirigami.Theme.textColor

then this is good to go imho

rkflx added a comment.EditedJun 7 2018, 6:35 AM

It will lead users to click into the field, expecting to be able to edit the name directly

Oh, you mean because the background turns white when enabled? I see.

Yup, that was the point.

Please use

color: Kirigami.Theme.textColor

Is this working for you? For me this resolves to the disabled palette (as it should).

(I could use colorGroup: Kirigami.theme, but there should be no difference to colorGroup: SystemPalette.Active, unless I'm missing something.)

broulik added a subscriber: mart.Jun 7 2018, 6:43 AM

Is this working for you? For me this resolves to the disabled palette (as it should).

Interesting. I just looked at TextField's code and saw that it explicit used a different color for !enabled so I thought removing that check worked.

What works for me is doing root.Kirigami.Theme.textColor which arguably isn't a lot better since it won't use button context but view context but I don't like having a random SystemPalette item in there. Perhaps @mart knows a proper solution

mart added a comment.Jun 7 2018, 8:49 AM

It will lead users to click into the field, expecting to be able to edit the name directly

Oh, you mean because the background turns white when enabled? I see.

Yup, that was the point.

Please use

color: Kirigami.Theme.textColor

Is this working for you? For me this resolves to the disabled palette (as it should).

(I could use colorGroup: Kirigami.theme, but there should be no difference to colorGroup: SystemPalette.Active, unless I'm missing something.)

none of those are valid values for colorGroup.

what you should do is:
TextField {

readOnly: true
Kirigami.Theme.inherit: false   //always to be set when you are writing anything to the Theme attached proeprty
Kirigami.Theme.colorGroup: Kirigami.Theme.Window

}

changing the textfield colorgroup to window, will make its background to become gray, so a look somewhat less "clickable"

rkflx added a comment.Jun 7 2018, 11:34 AM
In D13390#275153, @mart wrote:

what you should do is:
TextField {

readOnly: true
Kirigami.Theme.inherit: false   //always to be set when you are writing anything to the Theme attached proeprty
Kirigami.Theme.colorGroup: Kirigami.Theme.Window

}

changing the textfield colorgroup to window, will make its background to become gray, so a look somewhat less "clickable"

Now it looks as if you can edit the text, and the previews are placeholders for what you can type in:

IMO that's worse.

What works for me is doing root.Kirigami.Theme.textColor which arguably isn't a lot better since it won't use button context but view context but I don't like having a random SystemPalette item in there.

Let me know if I should go in that direction. It's a bit ugly as it assumes root is in enabled state, but at least it works.

(I miss good old KFontRequester.)

mart added a comment.Jun 7 2018, 12:32 PM
In D13390#275153, @mart wrote:

what you should do is:
TextField {

readOnly: true
Kirigami.Theme.inherit: false   //always to be set when you are writing anything to the Theme attached proeprty
Kirigami.Theme.colorGroup: Kirigami.Theme.Window

}

changing the textfield colorgroup to window, will make its background to become gray, so a look somewhat less "clickable"

Now it looks as if you can edit the text, and the previews are placeholders for what you can type in:

yeah, that's a bug i can reproduce, it's not supposed to look like that, i'll fix it

in the meantime, this form is also correct but it works with current kirigami:

QtControls.TextField {
     readOnly: true
     Kirigami.Theme.inherit: true
mart added a comment.Jun 7 2018, 12:32 PM

on which it looks like that:

mart added a comment.Jun 7 2018, 12:49 PM

edit: Kirigami.Theme.colorGroup: was wrong,should have been
Kirigami.Theme.colorSet: Kirigami.Theme.Window

tough i prefer the latter form of the patch

rkflx updated this revision to Diff 35808.Jun 7 2018, 10:50 PM
rkflx edited the summary of this revision. (Show Details)
rkflx edited the test plan for this revision. (Show Details)

Okay, great. Thanks for investigating! I now changed to what you proposed, so we have something for 5.13.1.

mart accepted this revision.Jun 8 2018, 10:53 AM

awesome, thanks :)

This revision is now accepted and ready to land.Jun 8 2018, 10:53 AM
This revision was automatically updated to reflect the committed changes.
abetts added a subscriber: abetts.Jun 8 2018, 2:19 PM

Maybe this is for another ticket, but... can we make the field where the font is named the actual field where you select the fonts from?

I mean, why have so many controls when the field can do it?

Also, would it be helpful that the field where the font is named to be able to type your font into? Without having to get a drop down?