Bug 165515 - [Cocoa] NSAttributedString representation of text copied from -webkit-nbsp-mode:space element contains non-breaking space characters, but shouldn’t
Summary: [Cocoa] NSAttributedString representation of text copied from -webkit-nbsp-mo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: mitz
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-12-06 21:55 PST by mitz
Modified: 2016-12-11 10:10 PST (History)
2 users (show)

See Also:


Attachments
Possible fix (no tests and no change log) (1.65 KB, patch)
2016-12-06 21:56 PST, mitz
no flags Details | Formatted Diff | Diff
Replace non-breaking spaces with spaces as needed (10.17 KB, patch)
2016-12-10 18:20 PST, mitz
no flags Details | Formatted Diff | Diff
Replace non-breaking spaces with spaces as needed (4.56 KB, patch)
2016-12-10 19:07 PST, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2016-12-06 21:55:12 PST
<rdar://problem/4108460>

In content-editable elements, we use non-breaking space characters to prevent runs of spaces from collapsing. But when the text is copied, we should turn those back into spaces in the NSAttributedString pasteboard representation (like we already do in the plain-text representation).
Comment 1 mitz 2016-12-06 21:56:39 PST
Created attachment 296374 [details]
Possible fix (no tests and no change log)
Comment 2 mitz 2016-12-06 21:59:08 PST
Comment on attachment 296374 [details]
Possible fix (no tests and no change log)

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

> Source/WebCore/editing/cocoa/HTMLConverter.mm:2252
> +                        nbspMode = characterData.parentElement() ? static_cast<ENBSPMode>(downcast<CSSPrimitiveValue>(*_caches->computedStylePropertyForElement(*characterData.parentElement(), CSSPropertyWebkitNbspMode).get())) : NBNORMAL;

Perhaps the chance of encountering an non-breaking space is low enough that we can use afford to use propertyValueForNode() and do a string compare instead of using computedStylePropertyForElement().
Comment 3 Darin Adler 2016-12-07 09:01:19 PST
Comment on attachment 296374 [details]
Possible fix (no tests and no change log)

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

>> Source/WebCore/editing/cocoa/HTMLConverter.mm:2252
>> +                        nbspMode = characterData.parentElement() ? static_cast<ENBSPMode>(downcast<CSSPrimitiveValue>(*_caches->computedStylePropertyForElement(*characterData.parentElement(), CSSPropertyWebkitNbspMode).get())) : NBNORMAL;
> 
> Perhaps the chance of encountering an non-breaking space is low enough that we can use afford to use propertyValueForNode() and do a string compare instead of using computedStylePropertyForElement().

Seems clear we *could* do that even if non-breaking spaces are relatively common, because the textTransform logic below calls propertyValueForNode once per string already. So calling it a second time couldn’t make things more than twice as slow.

I have to admit, I am not really an expert on the philosophy behind how the HTMLConverter works, but I see no harm in making this change.
Comment 4 Darin Adler 2016-12-08 09:07:58 PST
Change looks fine to me.
Comment 5 mitz 2016-12-10 18:20:58 PST
Created attachment 296837 [details]
Replace non-breaking spaces with spaces as needed
Comment 6 mitz 2016-12-10 18:59:13 PST
Comment on attachment 296837 [details]
Replace non-breaking spaces with spaces as needed

Pleased to see that there’s a layout test that can tell the difference!
Comment 7 mitz 2016-12-10 19:07:33 PST
Created attachment 296839 [details]
Replace non-breaking spaces with spaces as needed
Comment 8 mitz 2016-12-11 10:10:50 PST
Fixed in <https://trac.webkit.org/r209682>.