Bug 399155 - PlasmaComponents2 Password field gets taller when you type in it using the Noto Sans font with Noto Symbols2 as a fallback font for the U+2B24 (filled in circle) character that's missing in Noto Sans
Summary: PlasmaComponents2 Password field gets taller when you type in it using the No...
Status: RESOLVED FIXED
Alias: None
Product: libplasma
Classification: Frameworks and Libraries
Component: components (show other bugs)
Version: 5.55.0
Platform: Neon Linux
: NOR normal
Target Milestone: ---
Assignee: Marco Martin
URL:
Keywords:
: 405331 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-09-27 18:29 UTC by Neousr
Modified: 2019-04-11 06:08 UTC (History)
11 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.57


Attachments
KDE Dots (3.88 KB, image/png)
2018-09-27 18:29 UTC, Neousr
Details
Video demonstrating the issue (303.15 KB, video/webm)
2019-02-23 04:42 UTC, Nate Graham
Details
noto-1-vs-dejavu-1 (3.03 KB, image/png)
2019-02-25 15:36 UTC, Harald Sitter
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Neousr 2018-09-27 18:29:41 UTC
Created attachment 115271 [details]
KDE Dots

SUMMARY

The dots that appear as the password is typed are misaligned by a missing or extra pixel on the password field on the login screen.

There are 7 pixels on the top and 8 on the bottom as seen on the attached image.

STEPS TO REPRODUCE
1. Power on your system or unlock a already power on system
2. Type on the password field
3. Watch the password field get wider on the first typed character

OBSERVED RESULT

The password field gets wider by typing. Deleting all typed characters will reduce the field to his original size.


EXPECTED RESULT

The password field shouldn't change size by typing on any direction or password character content.


SOFTWARE VERSIONS

KDE Plasma Version: 5.13.5
KDE Frameworks Version: 5.50.0
Qt Version: 5.11.1

ADDITIONAL INFORMATION

Reproducible on neon unstable 5.14.80
Comment 1 Nate Graham 2018-10-01 19:16:35 UTC
Not sure if this is fixable. The number of pixels between the top and the bottom is determined by the font size and type. If we "fixed" this for one font, it would break for other fonts. There's no clean way to ensure this programmatically.
Comment 2 Christoph Feck 2018-10-01 20:15:10 UTC
Right. The dots are actually characters, rendered via libfreetype. The only way to fix it would be in Qt to use actual circles instead of characters. But then people would complain that password substitution characters do no longer follow platform defaults.

There are widget styles where the password substitution character is configurable, at least in Skulpture it is, but I guess QtCurve allows the same.
Comment 3 Nate Graham 2019-02-23 04:33:22 UTC
Re-opening since I think I misunderstood the issue. The problem is that the password field gets taller when you start typing. That's still the case, definitely shouldn't happen, and should be fixable.
Comment 4 Nate Graham 2019-02-23 04:42:29 UTC
Created attachment 118309 [details]
Video demonstrating the issue

This video depicts me typing in the lock screen in Neon Dev Unstable, with default font settings etc. Notice how the password field gets taller when there's any text in it.
Comment 5 Krešimir Čohar 2019-02-23 06:26:45 UTC
Neovar, is neon the only distribution you've tested this on?

I can reproduce this bug on neon, but not on Arch or Fedora.
Comment 6 Neousr 2019-02-23 16:39:45 UTC
Yes it only happens on neon stable, unstable.

Remember trying Kubuntu and the issue was not present it started after 5.12  probably 5.13 when the new blur and redesign on the login and lock screen debuted.
Comment 7 Krešimir Čohar 2019-02-23 17:07:45 UTC
Yeah I can't reproduce it in Kubuntu either
Comment 8 Krešimir Čohar 2019-02-23 17:21:34 UTC
Okay so I managed to track down the cause, it's in /etc/fonts/conf.d/56-neon-noto.conf. If you delete that file, no more bug.
I still don't know why though :D
Comment 9 Krešimir Čohar 2019-02-23 17:32:06 UTC
It's something in the code in lines 80-121 - if I remove lines 85 thru 119, the bug disappears, some kind of weird substitution occurs.
Comment 10 Nate Graham 2019-02-23 17:56:02 UTC
Aha, I was actually just going to suggest that it might be something weird in the fontconfig after having a dream about this! Nice find.
Comment 11 Krešimir Čohar 2019-02-23 18:08:05 UTC
Haha thanks... It really kept tripping me up - why Neon and not Arch, Fedora or Kubuntu even
Comment 12 Harald Sitter 2019-02-25 10:32:23 UTC
When you remove those lines the Noto fonts are no longer used. So, this answers nothing much as this is still either a metrics calculation issue in Qt or the qml components, or a problem with the font itself. When I open NotoSymbols2 and NotoSans and compare U+2B24 (which I think is the particular type of circle) to other characters they seem to in fact have different heights. Now I don't know the next thing about font design but I'm inclined to assume that if they are different heights in fontforge, they'll be different size when rendered.

What puzzles me though is when you copy ⬤⬤⬤⬤⬤ into kickoff's lineedit it doesn't switch size. What it means or if it means anything I don't know.

In conclusion I think the problem is the NotoSymbols2 font itself. So you'll probably want to file the bug report with them on github.

Furthermore I think the problem likely would go away if we simply used U+25CF which is a regular sized black circle (U+2B24 is big black circle, so in a way it's rendering the kind of circle that was requested ;)).
Moving this bug to plasma-integration for consideration. AFAIK the platformtheme sets the password masking character.
Comment 13 Nate Graham 2019-02-25 15:06:48 UTC
Sure, but U+2B24 doesn't seem to result in any problems in any other distros that don't have Neon's fontconfig settings. Does this mean that other distros are actually not using the noto font for the SDDM user?

I guess we could just use the alternative U+25CF character.
Comment 14 Harald Sitter 2019-02-25 15:36:50 UTC
Created attachment 118356 [details]
noto-1-vs-dejavu-1

(In reply to Nate Graham from comment #13)
> Does this mean that other distros are actually not using the noto font for the SDDM user?

That is correct. Well, sddm + lockscreen, I think the same font configs (Qt's) apply to both and as such they would by default be 'sans-serif' and what that means is controlled through fontconfig and that is usually not Noto (unless you are on Neon or ChromeOS at least).

An easy way to tell is looking at the date and time: Noto doesn't have a serif "1" character, while many other default font choices (DejaVu and Bitstream Vera come to mind) tend to have a serif 1.

For illustration I am attaching a screenshot of Noto 1 vs. DejaVu 1.
Comment 15 Harald Sitter 2019-02-25 15:38:27 UTC
Oh actually, having written that, I am not sure plasma-integration can actually do anything about the password character. At least on sddm I think the platformtheme for Plasma isn't actually loaded, what with not being a plasma session and all.
Comment 16 Nate Graham 2019-02-25 15:52:46 UTC
All right, let's use a different character. Do you know where that gets set?
Comment 17 Harald Sitter 2019-02-25 16:09:18 UTC
Not specifically, I don't know which QML components are involved here unfortunately. From the actual UI control the call chain ultimately goes deep into Qt (qcommonstyle I think?) and from there into the QPlatformTheme (i.e. plasma-integration for plasma sessions; but as I say that has no impact on SDDM most likely).
Chances are there is a way to influence the character from QML though.  I know that via qstylesheets you could change the character for qwidget QLabel.
Comment 18 Harald Sitter 2019-02-25 16:28:49 UTC
I had a quick grep in qtbase and I fear this needs some investigating.

There is this (but I think that isn't it because that's actually a tiny bullet •••)
platformsupport/themes/genericunix/qgenericunixthemes.cpp-    case QPlatformTheme::PasswordMaskCharacter:
platformsupport/themes/genericunix/qgenericunixthemes.cpp:        return QVariant(QChar(0x2022));

Then there is also this, but that's already the character I was suggesting ●●●●
gui/kernel/qplatformtheme.cpp-    case QPlatformTheme::PasswordMaskCharacter:
gui/kernel/qplatformtheme.cpp:        return QVariant(QChar(0x25CF));

Beyond those two I see no relevant code inside qtbase. There could be in qtquick controls, but I don't have a clone of that at hand.

Or I am wrong and we are already using U+25CF and I am reading the fontforge window incorrectly and the glyph is in fact larger than other characters. In which case we'd have to move to 2022 (the bullet) ^^
Comment 19 Krešimir Čohar 2019-02-25 19:57:45 UTC
This happens with other fonts too (Ubuntu, Inconsolata etc.) which might be because they're defaulting to Noto?

"
Or I am wrong and we are already using U+25CF and I am reading the fontforge window incorrectly and the glyph is in fact larger than other characters. In which case we'd have to move to 2022 (the bullet)"

Maybe Kickoff and other widgets just cut it off and the sddm-theme adapts to its size (makes the field bigger). In either case, a lot of other fonts (including whatever neon defaults to that isn't noto) don't do this, so I propose that we either modify Noto (can we do that?) or use a different glyph.
Comment 20 Harald Sitter 2019-02-26 09:54:42 UTC
Yes. If a font doesn't provide a character the next higher preferred font in the same family will provide it.
Comment 21 Eike Hein 2019-02-26 14:05:23 UTC
tightBoundingRect() on a FontMetrics with $text should be glyph substitution aware. If not, a hack would be a hidden Text {} with the same font doing the symbol and then looking at painted/implicitHeight.
Comment 22 Harald Sitter 2019-02-26 14:39:51 UTC
That made me think that this actually may be a problem with how the height is calculated when in password echo mode. The current input is a plasmacomponents 2 TextField, when I switch it for a qt controls TextField the height is large enough to accommodate the password character. So perhaps the problem is actually in plasmacomponents/kirigami where it should get the `max(placeholderText, passwordCharacterHeight)`?
Comment 23 Harald Sitter 2019-02-26 16:27:39 UTC
For the record: the font metrics are indeed different

Noto Sans
Ascent: 12
Descent: 4
Height: 16

Noto Sans Symbols2
Ascent: 12
Descent: 7
Height: 19

(all values from FontMetrics component)
Comment 24 Krešimir Čohar 2019-03-04 00:52:42 UTC
Shouldn't noto sans symbols2 be fixed (too) then?
Comment 25 Harald Sitter 2019-03-04 16:58:55 UTC
It's probably worth filing a bug with Noto. The size difference seems intentional though, so there may well be a rationale for why Symbols2 has larger metrics.
Comment 26 David Edmundson 2019-03-04 17:34:29 UTC
You might find PlasmaComponents3.TextField doesn't jump about. 

The height code is much more sensible.
Comment 27 Krešimir Čohar 2019-03-05 12:01:45 UTC
(In reply to David Edmundson from comment #26)
> You might find PlasmaComponents3.TextField doesn't jump about. 
> 
> The height code is much more sensible.

Good call - it doesn't (it even looks leaner), but this happens
https://phabricator.kde.org/file/data/x75du5yanhince3tbuy4/PHID-FILE-q7ao2tbeicsq6s3q62k7/image.png

Which I'm assuming is because of noto itself?
Comment 28 Krešimir Čohar 2019-03-05 12:13:20 UTC
(the issue doesn't occur with the plasmacomponents 3 textfield without the 56-neon-noto.conf file / when using a different font / different glyphs)
Comment 29 Krešimir Čohar 2019-03-07 13:56:46 UTC
If you set the font.family to "Noto Sans Symbols2" the password is permanently tall (deformed)

I strongly maintain that this is an issue with Noto Sans Symbols2 and that removing the relevant lines of code from 56-neon-noto.conf is a perfectly sound temporary workaround for this issue.
Comment 30 Harald Sitter 2019-03-07 14:06:17 UTC
We need a solution not a workaround. Noto Sans is the default font for Plasma, its symbols causing display glitches is a problem that needs solving.
Comment 31 Krešimir Čohar 2019-03-07 14:47:20 UTC
(In reply to Harald Sitter from comment #30)
> We need a solution not a workaround. Noto Sans is the default font for
> Plasma, its symbols causing display glitches is a problem that needs solving.

Noto Sans is indeed the default font in Plasma. Noto Sans Symbols2 is not.
And there are only so many ways to render a circle.

I never said we didn't need a solution. I agree that we do but given Noto Sans' other failings, the fact that no font is ever perfect (and that we shouldn't cling to Noto Sans so fervently), and the fact that the circles as rendered by a different font whose height matches that of Noto Sans (and not Symbols2) look the same as those of Noto Sans Symbols2, this workaround seems like an acceptable provisional solution.

We can put back the config file after the height issue is resolved.
Comment 32 trmdi 2019-03-07 15:15:03 UTC
Here is a minimal code to reproduce the bug:

###########

import QtQuick 2.0
import QtQuick.Layouts 1.2
import org.kde.plasma.components 2.0 as PlasmaComponents

Rectangle {
    width: passwordBox.width + 4
    height: passwordBox.height + 4
    color: "grey"

    PlasmaComponents.TextField {
        id: passwordBox
        revealPasswordButtonShown: true
        echoMode: TextInput.Password
        Layout.fillWidth: true
        font.family: "Noto Color Emoji"
    }

}

###########
When run it with qmlscene:

qmlscene test.qml
file:///usr/lib64/qt5/qml/QtQuick/Controls/Styles/Plasma/TextFieldStyle.qml:53:17: QML QQuickItem: Binding loop detected for property "implicitHeight"
file:///usr/lib64/qt5/qml/QtQuick/Controls/Styles/Plasma/TextFieldStyle.qml:53:17: QML QQuickItem: Binding loop detected for property "implicitHeight"
file:///usr/lib64/qt5/qml/QtQuick/Controls/Styles/Plasma/TextFieldStyle.qml:53:17: QML QQuickItem: Binding loop detected for property "implicitHeight"
file:///usr/lib64/qt5/qml/QtQuick/Controls/Styles/Plasma/TextFieldStyle.qml:53:17: QML QQuickItem: Binding loop detected for property "implicitHeight"
Comment 33 Krešimir Čohar 2019-03-07 16:00:43 UTC
(In reply to trmdi from comment #32)
> Here is a minimal code to reproduce the bug:
> 
> ###########
> 
> import QtQuick 2.0
> import QtQuick.Layouts 1.2
> import org.kde.plasma.components 2.0 as PlasmaComponents
> 
> Rectangle {
>     width: passwordBox.width + 4
>     height: passwordBox.height + 4
>     color: "grey"
> 
>     PlasmaComponents.TextField {
>         id: passwordBox
>         revealPasswordButtonShown: true
>         echoMode: TextInput.Password
>         Layout.fillWidth: true
>         font.family: "Noto Color Emoji"
>     }
> 
> }
> 
> ###########
> When run it with qmlscene:
> 
> qmlscene test.qml
> file:///usr/lib64/qt5/qml/QtQuick/Controls/Styles/Plasma/TextFieldStyle.qml:
> 53:17: QML QQuickItem: Binding loop detected for property "implicitHeight"
> file:///usr/lib64/qt5/qml/QtQuick/Controls/Styles/Plasma/TextFieldStyle.qml:
> 53:17: QML QQuickItem: Binding loop detected for property "implicitHeight"
> file:///usr/lib64/qt5/qml/QtQuick/Controls/Styles/Plasma/TextFieldStyle.qml:
> 53:17: QML QQuickItem: Binding loop detected for property "implicitHeight"
> file:///usr/lib64/qt5/qml/QtQuick/Controls/Styles/Plasma/TextFieldStyle.qml:
> 53:17: QML QQuickItem: Binding loop detected for property "implicitHeight"

Yes that works. You could also use just "Noto Sans" instead of the emoji font.

If I set the height value to accommodate Noto Sans (I haven't figured out how to resolve the binding loop yet so I used a ballpark estimate), the circles get cut off again (not unlike the PlasmaComponents 3 screenshot: https://phabricator.kde.org/file/data/x75du5yanhince3tbuy4/PHID-FILE-q7ao2tbeicsq6s3q62k7/image.png).

While this issue needs to be resolved too, it's not going to be enough.
Comment 34 Krešimir Čohar 2019-03-07 16:09:33 UTC
P.S. @trmdi you could also try the PlasmaComponents 3 text field (no binding loop) for reference
Comment 35 trmdi 2019-03-07 16:10:11 UTC
(In reply to Krešimir Čohar from comment #33)
I haven't figured out
> how to resolve the binding loop yet...

Maybe by using FontMetrics.height instead of control.cursorRectangle.height in /usr/lib64/qt5/qml/QtQuick/Controls/Styles/Plasma/TextFieldStyle.qml line 63.
But we also have to use QtQuick 2.12 instead.
Comment 36 Krešimir Čohar 2019-03-07 16:19:22 UTC
(In reply to trmdi from comment #35)
> (In reply to Krešimir Čohar from comment #33)
> I haven't figured out
> > how to resolve the binding loop yet...
> 
> Maybe by using FontMetrics.height instead of control.cursorRectangle.height
> in /usr/lib64/qt5/qml/QtQuick/Controls/Styles/Plasma/TextFieldStyle.qml line
> 63.
> But we also have to use QtQuick 2.12 instead.

Good idea. That eliminates the loop.
But it also leaves the field kind of short (Noto Sans gets cut off at the bottom and Noto Sans Symbols2 gets cut off at the top).

And yeah I had to bump QtQuick to make it work.

But I think you're on the right track.
Comment 37 trmdi 2019-03-07 17:50:31 UTC
(In reply to Krešimir Čohar from comment #36)
> But it also leaves the field kind of short (Noto Sans gets cut off at the
> bottom and Noto Sans Symbols2 gets cut off at the top).

Try: 

FontMetrics {
    ...
    font.pixelSize: control.font.pixelSize
    ...
}
Comment 38 Nate Graham 2019-03-07 17:52:50 UTC
After much investigation by Marco and Harald (thanks co much guys!), this turned out to have been caused by two distinct issues that interact poorly with one another:

- PlasmaComponents2 Text field inappropriately gets taller to accommodate the text, rather than making the text fit in the text field. This is fixed with https://phabricator.kde.org/D19599

- The Noto Symbols2 font has an incorrect Descent value (7 va 4 in Noto Sans). This is tracked with https://github.com/googlei18n/noto-fonts/issues/1468
Comment 39 trmdi 2019-03-07 17:55:30 UTC
(In reply to trmdi from comment #37)
> (In reply to Krešimir Čohar from comment #36)
> > But it also leaves the field kind of short (Noto Sans gets cut off at the
> > bottom and Noto Sans Symbols2 gets cut off at the top).
> 
> Try: 
> 
> FontMetrics {
>     ...
>     font.pixelSize: control.font.pixelSize
>     ...
> }

Sorry, just ignore this.
Comment 40 Marco Martin 2019-03-08 14:40:56 UTC
Git commit a2749dc9c71809c9ce3566d741cf133b5bf7e046 by Marco Martin.
Committed on 08/03/2019 at 14:40.
Pushed by mart into branch 'master'.

textfield height based only on clear text

Summary:
as the implicit height could change when switching to password mode,
use a textmetrics which always computes it from the visible text

Test Plan: height doesn't change

Reviewers: #plasma, ngraham, rooty

Reviewed By: ngraham, rooty

Subscribers: sitter, rooty, ngraham, kde-frameworks-devel

Tags: #frameworks

Differential Revision: https://phabricator.kde.org/D19599

M  +9    -2    src/declarativeimports/plasmastyle/TextFieldStyle.qml

https://commits.kde.org/plasma-framework/a2749dc9c71809c9ce3566d741cf133b5bf7e046
Comment 41 Christoph Feck 2019-03-31 10:18:00 UTC
*** Bug 405331 has been marked as a duplicate of this bug. ***
Comment 42 Jan Grulich 2019-04-10 14:11:37 UTC
*** Bug 406402 has been marked as a duplicate of this bug. ***
Comment 43 Jan Grulich 2019-04-11 06:08:03 UTC
Git commit 5014b50b719f7ab9837520799b5d9cde2a8b5c46 by Jan Grulich.
Committed on 11/04/2019 at 06:07.
Pushed by grulich into branch 'master'.

No need to set height to implicitHeight

Summary:
There seem to bug a bug in PlasmaComponents.TextField causing to use different
height depending on missing symbol for echo mode when the symbol is used from
a fallback font. While this was fixed in plasma-framework, we seem to be hitting
same issue when we set height to implicitHeight. It shouldn't be necessary to
set the height.

Reviewers: #plasma, davidedmundson

Reviewed By: #plasma, davidedmundson

Subscribers: plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D20436

M  +1    -1    applet/contents/ui/PasswordField.qml

https://commits.kde.org/plasma-nm/5014b50b719f7ab9837520799b5d9cde2a8b5c46