Bug 209692 – [Cocoa] Password obscuring dots drawn with the system font are too small

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[Cocoa] Password obscuring dots drawn with the system font are too small

[Myles C. Maxfield]

Created attachment 394780 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/60788385>

[Myles C. Maxfield]

Created attachment 394781 [details]
Patch

[Darin Adler]

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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

text() is called 153 times

[Myles C. Maxfield]

TextRuns are created in 51 places

[Myles C. Maxfield]

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()

[Myles C. Maxfield]

WOAH TextRun owns its string contents!!! TextRun::m_text is a String!

😮

[Myles C. Maxfield]

On second thought, SVGInlineTextBox and SVGTextMetrics aren't really relevant if we're interested in password fields.

[Myles C. Maxfield]

RenderText::computeCanUseSimplifiedTextMeasuring() is only used for SimpleLineLayout and LFC, which this new handling can opt ourselves out of.

[Myles C. Maxfield]

Created attachment 395664 [details]
Patch

[Darin Adler]

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?

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 395770 [details]
Patch

[Darin Adler]

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.

[Myles C. Maxfield]

U+F79A is meant to be used on all Apple platforms.

[Darin Adler]

(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?

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

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.

[Darin Adler]

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.

[Darin Adler]

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?

[Myles C. Maxfield]

Created attachment 395874 [details]
Patch

[Darin Adler]

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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Created attachment 395879 [details]
Patch for committing

[Darin Adler]

Looks like tests aren’t working

[Darin Adler]

For example <https://ews-build.webkit.org/results/macOS-Mojave-Release-WK1-Tests-EWS/r395874-6999/platform/mac/fast/text/text-security-disc-bullet-pua-diffs.html>.

[Myles C. Maxfield]

Oh, I see what happened. I named the tests the same, which means it's using the -expected results from platform/mac instead.

[Myles C. Maxfield]

Created attachment 395893 [details]
Patch for committing

[Darin Adler]

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

[Myles C. Maxfield]

Created attachment 396573 [details]
Patch for committing

[Myles C. Maxfield]

Comment on attachment 396573 [details]
Patch for committing

Forgot to add the new test

[Myles C. Maxfield]

Created attachment 396575 [details]
Patch for committing

[EWS]

commit-queue failed to commit attachment 396575 [details] to WebKit repository.

[Myles C. Maxfield]

The failures are all in web animations and look unrelated.

[Darin Adler]

I pushed the retry button to see if that helps.

[Myles C. Maxfield]

Committed r260173

[Darin Adler]

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.

[zalan]

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.

[Darin Adler]

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.

[zalan]

(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.

[Myles C. Maxfield]

Committed r260304: <https://trac.webkit.org/changeset/260304>