Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/1045 paste of attributed strings #1097

Merged
merged 11 commits into from Jun 10, 2019

Conversation

@SergioEstevao
Copy link
Contributor

commented Jun 7, 2019

Fixes #1045

This PR improves the paste code by reading more UTI types when doing paste and processing them to HTML if possible.

Note: Make sure you run yarn ios to get the update of Aztec lib

To test:

  • Start the demo app
  • Copy text with attributes (string, bold) from another app, for example Notes
  • Paste it on the demo app text block
  • Check that the content is pasted with attributes.

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.
@koke
Copy link
Member

left a comment

Code looks good except for the two comments I left. Can you do a WPiOS PR so we can test there and integrate once it's ready to merge?

Since this brings changes to Aztec and we're close to the freeze, I want to make sure we don't merge one and forget the other by mistake.

let html = getHTML(prettify: false)
return html
if let data = pasteboard.data(forPasteboardType: kUTTypePlainText as String) {
return String(data: data, encoding: .ascii)

This comment has been minimized.

Copy link
@koke

koke Jun 7, 2019

Member

Why .ascii and not .utf8?

}

if let data = pasteboard.data(forPasteboardType: kUTTypeText as String) {
return String(data: data, encoding: .ascii)

This comment has been minimized.

Copy link
@koke

koke Jun 7, 2019

Member

Why .ascii and not .utf8?

This comment has been minimized.

Copy link
@SergioEstevao

SergioEstevao Jun 7, 2019

Author Contributor

when the UTI is TypeText you don't have any guarantees of the encoding. So I was trying to put the lowest denominator there. It's a pity the platform doesn't have an auto-discover mode for this.

This comment has been minimized.

Copy link
@koke

koke Jun 7, 2019

Member

I think my slack comment got lost in the threads, so replying here. Are you checking for kUTTypePlainText and kUTTypeText for a reason?

https://developer.apple.com/documentation/uikit/uipasteboard/1622091-strings makes me think that kUTTypeUTF8PlainText is the only one relevant

@etoledom
Copy link
Contributor

left a comment

Hey @SergioEstevao !

I'm testing pasting content from the Note app to a Paragraph block, but it seems to not retain its format:

I have done a bit of debugging, and it seems that the native iOS side is doing what it should, it sends the html property on onPaste, but the final result is a block with not-formatted text.

copy-paste

@SergioEstevao

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@etoledom

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

So you see the HTML field getting sent to JS side?

Yes, it get's to the JS side. But it seems to be ignored.

@SergioEstevao

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@etoledom improve the paste method to better check HTML imports.

It should now be able to import from Notes, Pages and Safari correctly.

Thanks for your tests!


if let data = pasteboard.data(forPasteboardType: kUTTypeHTML as String), let html = String(data: data, encoding: .utf8) {
// Make sure we are not getting a full HTML DOC. We only want inner content
if !html.hasPrefix("<!DOCTYPE html") {

This comment has been minimized.

Copy link
@SergioEstevao

SergioEstevao Jun 9, 2019

Author Contributor

I'm doing this check here to check if we are not importing a full HTML document because on my tests I found that the when copying from the Notes app, it was sending the full HTML document, using span and styles for bold/italic styles instead of using just <strong> and <em> element.

My assumption is that when importing HTML if a full document is there, maybe there is content there is not valid or importable to GB so instead we should try to read RTF.

If we think that this is not a good assumption we can remove this line, but then import from the Notes app will not work.

For the record when pasting from Safari, it provide only the selected HTML and not the full document so the import from HTML works correctly.

This comment has been minimized.

Copy link
@mkevins

mkevins Jun 10, 2019

Contributor

using span and styles for bold/italic styles instead of using just <strong> and <em> element.

My assumption is that when importing HTML if a full document is there, maybe there is content there is not valid or importable to GB so instead we should try to read RTF.

FWIW, code exists on GB side to handle inline styles on span tags, however, the limitations of our DOM implementation (jsdom-jscore) prevent it from working correctly.

To "enable" this code path, we'd need the style property on ELEMENT_NODEs to be implemented, as well as ATTRIBUTE_NODEs.

If we think that this is not a good assumption we can remove this line, but then import from the Notes app will not work.

It's possible that implementing the necessary DOM spec will require significant time / effort. I think your approach is good to mitigate the underlying problem (that our DOM implementation is insufficient). I wonder if wrapping the html in a "full document" is correlated with using inline styles on spans, or if Notes.app happens to behave this way, and other apps have differing behavior regarding these two characteristics.

Is there some way to directly check if the pasteboard html generated by the external app uses inline styles on span tags?

This comment has been minimized.

Copy link
@etoledom

etoledom Jun 10, 2019

Contributor

I think your approach is good to mitigate the underlying problem

I agree with this!

I wonder if wrapping the html in a "full document" is correlated with using inline styles on spans, or if Notes.app happens to behave this way, and other apps have differing behavior regarding these two characteristics.

I just tested the Google docs app, and it happens that it does use inline styles on spans, but without wrapping it up on a full HTML document. So this approach doesn't work for it.

But it does work for the Notes app.

I'm OK with shipping this improvement to support the Apple default's Note app, and check for a more wide alternative later on.

What do you think @SergioEstevao ?

@etoledom etoledom self-requested a review Jun 10, 2019

guard let data = pasteboard.data(forPasteboardType: kUTTypeHTML as String) else {
return nil
private func read(from pasteboard: UIPasteboard, uti: String, documentType: DocumentType) -> String? {
guard let data = pasteboard.data(forPasteboardType: uti as String),

This comment has been minimized.

Copy link
@etoledom

etoledom Jun 10, 2019

Contributor

Small detail:
uti is already String so this extra as String is not needed.

Maybe we can change uti's type to be CFString to make it simpler on the caller side?

@etoledom
Copy link
Contributor

left a comment

Since this solves the main pasting issue with the Notes apps, let's go with this! 🎉

As @mkevins commented, there's probably more work to do on the JS side to fix pasting from any source without the need of native-side cleanups, let's investigate this further in the future 👍

Thank you for this @SergioEstevao !

@SergioEstevao SergioEstevao merged commit e3df0c8 into develop Jun 10, 2019

6 checks passed

Peril All green. Congrats.
Details
ci/circleci: Check Correctness Your tests passed on CircleCI!
Details
ci/circleci: Test Android Your tests passed on CircleCI!
Details
ci/circleci: Test Android on Device Your tests passed on CircleCI!
Details
ci/circleci: Test iOS Your tests passed on CircleCI!
Details
ci/circleci: Test iOS on Device Your tests passed on CircleCI!
Details

@SergioEstevao SergioEstevao deleted the issue/1045_paste_of_attributed_strings branch Jun 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.