Bug 209692

Summary: [Cocoa] Password obscuring dots drawn with the system font are too small
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, darin, dmazzoni, esprehn+autocc, ews-watchlist, glenn, jcraig, jdiggs, kondapallykalyan, mifenton, pdr, samuel_white, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
darin: review+
Patch for committing
none
Patch for committing
none
Patch for committing
none
Patch for committing ews-feeder: commit-queue-

Description Myles C. Maxfield 2020-03-27 19:15:35 PDT
[Cocoa] Password obscuring dots drawn with the system font are too small
Comment 1 Myles C. Maxfield 2020-03-27 19:19:12 PDT
Created attachment 394780 [details]
Patch
Comment 2 Myles C. Maxfield 2020-03-27 19:19:15 PDT
<rdar://problem/60788385>
Comment 3 Myles C. Maxfield 2020-03-27 19:20:16 PDT
Created attachment 394781 [details]
Patch
Comment 4 Darin Adler 2020-03-28 19:42:23 PDT
Comment on attachment 394781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394781&action=review

Goal seems great, even critical.

But I think this transformation should be done even closer to the rendering. I would never want to see the U+F79A character coming out of the TextIterator, ever. Not even when TextIteratorEmitsTextsWithoutTranscoding is unset.

The backslash to yen conversion is something different. It’s a sort of semantic conversion, and one that we want to do when copying to a Unicode pasteboard.

But the U+F79A trick is entirely a rendering quirk, not semantic at all. Is there some way we can put it deeper in the style system, closer to the font machinery?

> Source/WebCore/rendering/RenderText.cpp:246
> +static constexpr const UChar textSecurityDiskPUACodePoint = 0xF79A;

No need for the "static" or the "const" when it’s constexpr in a case like this.

This could go into CharacterNames.h but I suppose it’s not truly a character name?

Typically a code point would be a UChar32, not a UChar.

> Source/WebCore/rendering/RenderText.cpp:251
> +    return textSecurity == TextSecurity::Disc && font.platformData().isSystemFont() && font.glyphForCharacter(textSecurityDiskPUACodePoint);

Not obvious to me that a zero glyph means no glyph.

> Source/WebCore/rendering/RenderText.cpp:286
> +        if (oldStyle->fontCascade().useBackslashAsYenSymbol() != newStyle.fontCascade().useBackslashAsYenSymbol()) {
> +            m_useBackslashAsYenSymbol = computeUseBackslashAsYenSymbol();
> +            needsResetText = true;
> +        }
> +        auto oldStyleTextSecurityDiskShouldUsePUACodePoint = computeTextSecurityDiscShouldUsePUACodePoint(oldStyle->fontCascade().primaryFont(), oldStyle->textSecurity());
> +        if (newStyleTextSecurityDiskShouldUsePUACodePoint != oldStyleTextSecurityDiskShouldUsePUACodePoint) {
> +            needsResetText = true;
> +            m_textSecurityDiscShouldUsePUACodePoint = newStyleTextSecurityDiskShouldUsePUACodePoint;
> +        }

Why the different order of setting needsResetText?

Seems costly to compute whether we should use the PUA code point on both typefaces every time style changes. Is this efficient enough?

> Source/WebCore/rendering/RenderText.cpp:1202
> +        secureText(m_textSecurityDiscShouldUsePUACodePoint ? 0xF79A : bullet);

Seems like this should be using the "textSecurityDiskPUACodePoint" constant. We need to put it somewhere it can be used in both places.

> Source/WebCore/rendering/RenderText.cpp:1310
> -String RenderText::textWithoutConvertingBackslashToYenSymbol() const
> +String RenderText::textWithoutTranscoding() const

Not sure the analogy here is perfect. There are places that call this and then convert backslash to yen at the end; I suppose that’s fine with this, though.
Comment 5 Myles C. Maxfield 2020-03-30 21:35:14 PDT
Okay, that makes sense. I'll have to see where a good place to put it is. Inside GraphicsContext or lower is probably not right, since TextRun is constructed with a reference to a preexisting string. Trying to act as-if the string was modified would likely confuse callers.

We'll have to modify BreakingContext::handleText() directly since that calls directly into renderText.text(). Same thing with RenderText::computePreferredLogicalWidths(). And any other place which constructs a TextRun.
Comment 6 Myles C. Maxfield 2020-04-06 22:09:33 PDT
text() is called 153 times
Comment 7 Myles C. Maxfield 2020-04-06 22:30:49 PDT
TextRuns are created in 51 places
Comment 8 Myles C. Maxfield 2020-04-06 23:22:42 PDT
After auditing all the sites, I think these are the ones which need to be updated:

RenderBlock::constructTextRun()
InlineTextBox::text()
SVGInlineTextBox::constructTextRun()
SVGTextMetrics::constructTextRun()
RenderText::computeCanUseSimplifiedTextMeasuring()
Comment 9 Myles C. Maxfield 2020-04-06 23:37:25 PDT
WOAH TextRun owns its string contents!!! TextRun::m_text is a String!

😮
Comment 10 Myles C. Maxfield 2020-04-07 00:10:31 PDT
On second thought, SVGInlineTextBox and SVGTextMetrics aren't really relevant if we're interested in password fields.
Comment 11 Myles C. Maxfield 2020-04-07 00:14:13 PDT
RenderText::computeCanUseSimplifiedTextMeasuring() is only used for SimpleLineLayout and LFC, which this new handling can opt ourselves out of.
Comment 12 Myles C. Maxfield 2020-04-07 01:59:32 PDT
Created attachment 395664 [details]
Patch
Comment 13 Darin Adler 2020-04-07 11:08:21 PDT
Comment on attachment 395664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395664&action=review

> Source/WebCore/rendering/SimpleLineLayout.cpp:244
> +    if (style.textSecurity() != TextSecurity::None)
> +        SET_REASON_AND_RETURN_IF_NEEDED(FlowHasTextSecurity, reasons, includeReasons);

Add a "why" comment somewhere?

> Source/WebCore/rendering/style/RenderStyle.h:738
> +    static constexpr UChar textSecurityDiskPUACodePoint = 0xF79A; // The PUA character in the system font which is used to render password field dots on Cocoa platforms

Great idea to have a constant. But this seems like a strange place for the constant.

> Source/WebCore/rendering/style/RenderStyle.h:739
> +    bool computeTextSecurityDiscShouldUsePUACodePoint() const;

Normally functions are named "compute" when there is a slow algorithm (compute) and a memoized version (no verb). I am not sure that the word "compute" should be used here.

Given the only use of this is to do replacement in a String, and always with a String that is going to be created, I suggest we consider an approach where there is a function that maps the string as needed. I wouldn’t put it as a RenderStyle member, though. I would name it like this:

    String updateSecurityDiscCharacters(RenderStyle& style, String&& string)
    {
#if !PLATFORM(COCOA)
        return WTFMove(string);
#else
        if (style.textSecurity() != TextSecurity::Disc)
            return WTFMove(string);
         // This PUA character in the system font is used to render password field dots on Cocoa platforms.
         constexpr UChar textSecurityDiscPUACodePoint = 0xF79A;
        auto& font = style.fontCascade().primaryFont();
        if (!(font.platformData().isSystemFont() && font.glyphForCharacter(textSecurityDiscPUACodePoint))
            return WTFMove(string);
        return string.replace(bullet, textSecurityDiscPUACodePoint);
#endif
    }

The thing I like about this is that this function is the only place that needs the textSecurityDiscPUACodePoint constant. Not sure where the best place to put this is. Could make it an inline since it’s only called in two places.

Separate from this patch: Should consider adding a third constructor to TextRun that takes a String&&. The String and StringView ones can both call the String&& one. Also, maybe we should make the toStringWithoutCopying() explicit at call sites instead of implicit?
Comment 14 Myles C. Maxfield 2020-04-07 22:19:33 PDT
The iOS test is failing because:

#if !PLATFORM(IOS_FAMILY)
    // We use the same characters here as for list markers.
    // See the listMarkerText function in RenderListMarker.cpp.
    case TextSecurity::Circle:
        secureText(whiteBullet);
        break;
    case TextSecurity::Disc:
        secureText(bullet);
        break;
    case TextSecurity::Square:
        secureText(blackSquare);
        break;
#else
    // FIXME: Why this quirk on iOS?
    case TextSecurity::Circle:
    case TextSecurity::Disc:
    case TextSecurity::Square:
        secureText(blackCircle);
        break;
#endif
Comment 15 Myles C. Maxfield 2020-04-07 22:31:12 PDT
Created attachment 395770 [details]
Patch
Comment 16 Darin Adler 2020-04-08 09:39:55 PDT
Comment on attachment 395770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395770&action=review

> Source/WebCore/rendering/RenderBlock.cpp:3517
> +#if !PLATFORM(COCOA)

Maybe this should be #if !PLATFORM(MAC) if it’s not for iOS yet?

> Source/WebCore/rendering/RenderBlock.cpp:3527
> +    return string.replace(bullet, textSecurityDiscPUACodePoint);

Do we want to replace "blackCircle" on iOS? Further, do we want to do it for types other than "disc"?

> Source/WebCore/rendering/SimpleLineLayout.cpp:244
> +    // -webkit-text-security: disc has special handling. It needs to use a special PUA character when rendering with the system font, but can't modify the actual text of the renderer to do this.
> +    // Therefore, it's handled at layout/drawing time, rather than in the renderer itself.

Thank you for adding a comment. I think it could be a shorter one. More like this:

    // Special handling of text-security:disc is not yet implemented in the simple line layout code path.
    // See RenderBlock::updateSecurityDiscCharacters.

> LayoutTests/ChangeLog:14
> +        * fast/text/text-security-disc-bullet-pua-expected.html: Added.
> +        * fast/text/text-security-disc-bullet-pua-old-expected.html: Added.
> +        * fast/text/text-security-disc-bullet-pua-old.html: Added.
> +        * fast/text/text-security-disc-bullet-pua.html: Added.
> +        * fast/text/text-security-disc-bullet-pua-ios-expected.html: Added.
> +        * fast/text/text-security-disc-bullet-pua-ios.html: Added.

If this is Mac-only behavior, then the test should be in platform/mac.

Ideally if this is Cocoa-only behavior, the test should be in platform/cocoa.

It’s not as good to put the test in the main test directories, and then disable or skip it on other platforms.
Comment 17 Myles C. Maxfield 2020-04-08 12:34:30 PDT
U+F79A is meant to be used on all Apple platforms.
Comment 18 Darin Adler 2020-04-08 12:50:12 PDT
(In reply to Myles C. Maxfield from comment #17)
> U+F79A is meant to be used on all Apple platforms.

Then can we revise this so it’s used on IOS_FAMILY?
Comment 19 Myles C. Maxfield 2020-04-08 14:06:03 PDT
(In reply to Darin Adler from comment #18)
> (In reply to Myles C. Maxfield from comment #17)
> > U+F79A is meant to be used on all Apple platforms.
> 
> Then can we revise this so it’s used on IOS_FAMILY?

Yes, of course. I was just making a note here just in case.
Comment 20 Myles C. Maxfield 2020-04-08 14:34:49 PDT
Comment on attachment 395770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395770&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:3527
>> +    return string.replace(bullet, textSecurityDiscPUACodePoint);
> 
> Do we want to replace "blackCircle" on iOS? Further, do we want to do it for types other than "disc"?

Yes to replacing blackCircle, no to doing it for types other than "disc." The other types are things like "square" which shouldn't use this character. "Circle" is supposed to be hollow, so it shouldn't use this character either.
Comment 21 Darin Adler 2020-04-08 15:02:25 PDT
Comment on attachment 395770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395770&action=review

>>> Source/WebCore/rendering/RenderBlock.cpp:3527
>>> +    return string.replace(bullet, textSecurityDiscPUACodePoint);
>> 
>> Do we want to replace "blackCircle" on iOS? Further, do we want to do it for types other than "disc"?
> 
> Yes to replacing blackCircle, no to doing it for types other than "disc." The other types are things like "square" which shouldn't use this character. "Circle" is supposed to be hollow, so it shouldn't use this character either.

On iOS, when a page specifies text-securiity:square or text-security:circle, WebKit use blackCircle, effectively treating it like text-security:disc. Given that behavior, on iOS, I suggest WebKit use textSecurityDiscPUACodePoint for all three of those cases. I agree that if we change WebKit to support square or circle on iOS, then we would *not* want to use textSecurityDiscPUACodePoint, transforming it into a disc. My point is that if we continue to check for TextSecurity::Disc specifically in this function, then on iOS this means we will still get the glyph for Unicode character blackCircle in cases where we’d instead want the glyph for textSecurityDiscPUACodePoint.
Comment 22 Darin Adler 2020-04-08 15:03:06 PDT
Comment on attachment 395770 [details]
Patch

Do you want me to review+ this, or are you planning to post a version with better handling for the iOS-family platforms?
Comment 23 Myles C. Maxfield 2020-04-08 15:07:53 PDT
Created attachment 395874 [details]
Patch
Comment 24 Darin Adler 2020-04-08 15:16:07 PDT
Comment on attachment 395874 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395874&action=review

> Source/WebCore/rendering/RenderBlock.cpp:3520
> +    if (style.textSecurity() != TextSecurity::Disc)

For iOS family, should run the code below for TextSecurity::Square and TextSecurity::Circle.

It would be safe, because it will never transform any character other than blackCircle. So it won’t ever change a square or hollow circle.

If would be helpful and effective, because otherwise the blackCircle character will be used for TextSecurity::Square and TextSecurity::Circle.

So I think we should consider changing this.

> Source/WebCore/rendering/RenderBlock.cpp:3533
> +#if PLATFORM(IOS_FAMILY)
> +    constexpr UChar discCharacterToReplace = blackCircle;
> +#else
> +    constexpr UChar discCharacterToReplace = bullet;
> +#endif

It’s irritating to have the connection between the code here and the code in RenderText::setRenderedText, with no comment there pointing here.

I feel like it’s my fault since I encouraged you to put the code here instead of there. Can live with it for now, I guess.
Comment 25 Myles C. Maxfield 2020-04-08 15:53:41 PDT
Comment on attachment 395874 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395874&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:3520
>> +    if (style.textSecurity() != TextSecurity::Disc)
> 
> For iOS family, should run the code below for TextSecurity::Square and TextSecurity::Circle.
> 
> It would be safe, because it will never transform any character other than blackCircle. So it won’t ever change a square or hollow circle.
> 
> If would be helpful and effective, because otherwise the blackCircle character will be used for TextSecurity::Square and TextSecurity::Circle.
> 
> So I think we should consider changing this.

Right, good catch.

>> Source/WebCore/rendering/RenderBlock.cpp:3533
>> +#endif
> 
> It’s irritating to have the connection between the code here and the code in RenderText::setRenderedText, with no comment there pointing here.
> 
> I feel like it’s my fault since I encouraged you to put the code here instead of there. Can live with it for now, I guess.

There is a comment there pointing here. See Source/WebCore/rendering/RenderText.cpp line 1169.
Comment 26 Myles C. Maxfield 2020-04-08 16:05:07 PDT
Created attachment 395879 [details]
Patch for committing
Comment 27 Darin Adler 2020-04-08 16:30:00 PDT
Looks like tests aren’t working
Comment 29 Myles C. Maxfield 2020-04-08 17:52:50 PDT
Oh, I see what happened. I named the tests the same, which means it's using the -expected results from platform/mac instead.
Comment 30 Myles C. Maxfield 2020-04-08 17:59:43 PDT
Created attachment 395893 [details]
Patch for committing
Comment 31 Darin Adler 2020-04-09 11:45:12 PDT
Comment on attachment 395893 [details]
Patch for committing

View in context: https://bugs.webkit.org/attachment.cgi?id=395893&action=review

> LayoutTests/platform/ios/fast/text/text-security-disc-bullet-pua-ios.html:8
> +<div style="font: 48px system-ui; -webkit-text-security: disc;">abcdefg</div>
> +<div style="font: 48px serif; -webkit-text-security: disc;">abcdefg</div>

This test is failing: https://ews-build.webkit.org/results/iOS-13-Simulator-WK2-Tests-EWS/r395893-14998/fast/text/text-security-disc-bullet-pua-diffs.html
Comment 32 Myles C. Maxfield 2020-04-15 14:14:43 PDT
Created attachment 396573 [details]
Patch for committing
Comment 33 Myles C. Maxfield 2020-04-15 14:17:48 PDT
Comment on attachment 396573 [details]
Patch for committing

Forgot to add the new test
Comment 34 Myles C. Maxfield 2020-04-15 14:18:50 PDT
Created attachment 396575 [details]
Patch for committing
Comment 35 EWS 2020-04-15 14:57:36 PDT
commit-queue failed to commit attachment 396575 [details] to WebKit repository.
Comment 36 Myles C. Maxfield 2020-04-15 18:24:21 PDT
The failures are all in web animations and look unrelated.
Comment 37 Darin Adler 2020-04-15 18:50:05 PDT
I pushed the retry button to see if that helps.
Comment 38 Myles C. Maxfield 2020-04-15 22:57:56 PDT
Committed r260173
Comment 39 Darin Adler 2020-04-16 09:58:29 PDT
Comment on attachment 396575 [details]
Patch for committing

View in context: https://bugs.webkit.org/attachment.cgi?id=396575&action=review

> LayoutTests/platform/mac/TestExpectations:1982
> +webkit.org/b/209692 [ Sierra HighSierra Mojave Catalina ] platform/mac/fast/text/text-security-disc-bullet-pua-mac.html [ ImageOnlyFailure ]

For tidiness should take out Sierra and HighSierra since Mojave is the oldest version of macOS supported in TOT WebKit.
Comment 40 zalan 2020-04-17 13:47:15 PDT
It's very unfortunate that RenderText has these many hats. It makes seemingly easy tasks very difficult to implement in a robust way. For example in this case, if the text iterator/editing code consulted the render tree through some iterator interface and this iterator layer implemented all the necessary conversions (in an efficient manner), this would be a very simple change and RenderText could be solely about the _measured_ content, input to the layout process (and in turn InlineTextBox could always be about the painted content, in a memory efficient way ofc).
We measure text in way too many different ways and places and the fact that now RenderText::text() needs to be measured through RenderBlock::constructTextRun() if you want matching geometry, makes it a tiny bit more error prone. I don't really have a solution, I just wish RenderText had fewer hats.
Comment 41 Darin Adler 2020-04-17 13:50:32 PDT
Alan, we should probably talk about this in person at some point.

The original patch did all the work in the secureText function, which is far earlier in the pipeline from DOM to rendering. What I didn't like about that was that the non-bullet characters could be seen by TextIterator and since it's entirely a glyph/font-rendering issue we would never want that to happen, even if we do want to see the bullets used for text security.

Finding the perfect place for this isn’t 100% obvious and it would be fine to reimplement it another way later if we find one.
Comment 42 zalan 2020-04-17 14:13:02 PDT
(In reply to Darin Adler from comment #41)
> Alan, we should probably talk about this in person at some point.
> 
> The original patch did all the work in the secureText function, which is far
> earlier in the pipeline from DOM to rendering. What I didn't like about that
> was that the non-bullet characters could be seen by TextIterator and since
> it's entirely a glyph/font-rendering issue we would never want that to
> happen, even if we do want to see the bullets used for text security.
I agree, glyph transforms should not be visible to TextIterator. Unfortunately this is a very special case where the transform is not a paint time operation only, but it changes geometry as well -hence the complexity.

> Finding the perfect place for this isn’t 100% obvious and it would be fine
> to reimplement it another way later if we find one.
It's not obvious to me either given the current architecture.
Comment 43 Myles C. Maxfield 2020-04-17 18:26:41 PDT
Committed r260304: <https://trac.webkit.org/changeset/260304>