Bug 201490 – REGRESSION (iOS 13): Bulleted list copied from Notes to Mail results in Times New Roman

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Reproduction steps:
* Make a bullet list item on Notes
* Copy & paste it into Mail compose

Result:
The pasted text is in Times New Roman.

<rdar://problem/54394059>

[Ryosuke Niwa]

Created attachment 378044 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 378045 [details]
Added a forgotten resource file

[Daniel Bates]

Comment on attachment 378045 [details]
Added a forgotten resource file

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

This patch looks good.

> Source/WebCore/editing/EditingStyle.cpp:1285
> +static String loneFontFamilyName(StyleProperties::PropertyReference& property)

Can the parameter be made const lvalue reference? Or can param be taken by value since sizeof(StyleProperties::PropertyReference) == sizeof(StringView) and we pass StringView by value?

> Source/WebCore/editing/EditingStyle.cpp:1290
> +    CSSValue& value = *property.value();

Why is it safe to dereference this? Are there any id types where it would be unsafe to dereference this? If it is always safe then can it be explained why in the ChangeLog?

Can this local be a const lvalue reference?

> Source/WebCore/editing/EditingStyle.cpp:1295
> +        auto& primitiveValue = downcast<CSSPrimitiveValue>(value);
> +        if (!primitiveValue.isFontFamily())
> +            return { };
> +        return primitiveValue.fontFamily().familyName;

This code is a duplicate of the code below (lines 1305-1309). Can the code be shared? The benefit of sharing code is that if a bug is found in the code the fix can be made in one place instead of two.

> Source/WebCore/editing/EditingStyle.h:64
> +enum class StandardFontFamilySerializationMode { Keep, Strip };

Can a width be specified?

> LayoutTests/editing/pasteboard/paste-cocoa-writer-markup-with-webkit-standard-font-family-expected.txt:12
> +Start

Can this be removed from the result? The benefit would be to reduce the noise in the output to focus on the actual test content.

[Daniel Bates]

Comment on attachment 378045 [details]
Added a forgotten resource file

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

> LayoutTests/editing/pasteboard/paste-cocoa-writer-markup-with-webkit-standard-font-family.html:48
> +editor.focus();
> +start.addEventListener('click', () => {
> +    editor.focus();
> +    document.execCommand('selectAll');
> +    document.execCommand('copy');
> +    if (window.testRunner)
> +        document.execCommand('paste');
> +});
> +editor.addEventListener('copy', (event) => {
> +    event.clipboardData.setData('text/html', markup);
> +    event.preventDefault();
> +});
> +editor.addEventListener('paste', (event) => {
> +    shouldBe(`event.clipboardData.getData('text/html')`, `markup`);
> +    document.execCommand('insertHTML', false, event.clipboardData.getData('text/html'));
> +    shouldBeTrue(`editor.innerHTML.includes('-webkit-standard')`);
> +    editor.innerHTML = '';
> +    event.preventDefault();
> +    finishJSTest();
> +});
> +
> +if (window.testRunner)
> +    start.click();

This code is almost identical to the code in LayoutTests/editing/pasteboard/paste-cocoa-writer-markup-with-system-fonts.html. Can the common part be extracted and shared?

[Ryosuke Niwa]

Comment on attachment 378045 [details]
Added a forgotten resource file

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

>> Source/WebCore/editing/EditingStyle.cpp:1285
>> +static String loneFontFamilyName(StyleProperties::PropertyReference& property)
> 
> Can the parameter be made const lvalue reference? Or can param be taken by value since sizeof(StyleProperties::PropertyReference) == sizeof(StringView) and we pass StringView by value?

The point of taking PropertyReference is to have CSSPropertyFontFamily check
so that we can avoid having one more nested if in mergeStyleFromRulesForSerialization.

>> Source/WebCore/editing/EditingStyle.cpp:1290
>> +    CSSValue& value = *property.value();
> 
> Why is it safe to dereference this? Are there any id types where it would be unsafe to dereference this? If it is always safe then can it be explained why in the ChangeLog?
> 
> Can this local be a const lvalue reference?

Because we're iterating over properties that exist in mergeStyleFromRulesForSerialization.

>> Source/WebCore/editing/EditingStyle.cpp:1295
>> +        return primitiveValue.fontFamily().familyName;
> 
> This code is a duplicate of the code below (lines 1305-1309). Can the code be shared? The benefit of sharing code is that if a bug is found in the code the fix can be made in one place instead of two.

Maybe. We can probably add a static helper.

>> Source/WebCore/editing/EditingStyle.h:64
>> +enum class StandardFontFamilySerializationMode { Keep, Strip };
> 
> Can a width be specified?

Sure.

>> LayoutTests/editing/pasteboard/paste-cocoa-writer-markup-with-webkit-standard-font-family.html:48
>> +    start.click();
> 
> This code is almost identical to the code in LayoutTests/editing/pasteboard/paste-cocoa-writer-markup-with-system-fonts.html. Can the common part be extracted and shared?

I don't think we should do that because it would make the test harder to understand & debug.

[Ryosuke Niwa]

Created attachment 378057 [details]
Addressed review comments

[Daniel Bates]

Comment on attachment 378057 [details]
Addressed review comments

LGTM

[Ryosuke Niwa]

Comment on attachment 378057 [details]
Addressed review comments

Thanks for the review!

[WebKit Commit Bot]

Comment on attachment 378057 [details]
Addressed review comments

Clearing flags on attachment: 378057

Committed r249535: <https://trac.webkit.org/changeset/249535>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.