Bug 185289 – [iOS] Multiple links in Mail are dropped in a single line, and are difficult to tell apart

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/35756912>

[Wenson Hsieh]

Created attachment 339512 [details]
Patch

[Tim Horton]

Comment on attachment 339512 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        Additionally remove some extraneous header imports from this implementation file.

Craaaazy. How sure are you that they are extraneous and you’re not just getting lucky because of unified sources?

[Darin Adler]

Comment on attachment 339512 [details]
Patch

r=me as is, but I have some concerns

I also don’t know how I feel about the idea of pasting multiple links as if they were text separated by newlines. I guess someone asked for that behavior. Is there a similar case in the iOS or macOS UI that we are aligning with? It’s particularly strange that it can break a word or sentence into multiple lines.

I am not confident about the choice of a <br> element as a separator between multiple links to achieve having each on a separate line. I think the analogy might be to pasting text with newlines, and that does something different, does not always turn the newlines into <br> elements. See the createFragmentFromText function for details. It uses paragraph elements in some common cases. Inserting a line break by typing does a different thing. See the  InsertLineBreakCommand::doApply function and note that it will use a newline character rather than a <br> in cases like inside a <pre> element. Hitting the Return will do a third different thing. See the InsertParagraphSeparator::doApply function.

This code doesn’t seem to do the right thing when putting multiple links into a non-richly-editable location. Seems like it will put the URLs all next to each other without even spaces separating them?

Not new to this patch: I don’t understand why the macOS and iOS versions of WebContentReader::readURL are so different from each other. They do not seem to be limited to platform specifics. It’s a sort of “bad code smell" that the additional sophistication in the iOS file seems like it comprises all improvements that would apply equally well to macOS and even other platforms. We should factor this code better. I guess I’d like to see most of this logic be entirely platform independent. It’s not good to have the platforms be diverging like this.

Not new to this patch: It seems not so great to have the readURL function do the "append" logic as well as the logic for creating an element for a single link. Perhaps I don’t understand the factoring well enough here, but it seems that we should separate code that knows the platform-specific rules for how to create an anchor for a link from the code that knows how to insert the multiple anchors with appropriate separators.

[Wenson Hsieh]

Comment on attachment 339512 [details]
Patch

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

>> Source/WebCore/ChangeLog:18
>> +        Additionally remove some extraneous header imports from this implementation file.
> 
> Craaaazy. How sure are you that they are extraneous and you’re not just getting lucky because of unified sources?

Good point! I'm sure at least a few of these are not needed. I'll double check to make sure.

[Wenson Hsieh]

Thanks for the feedback! Replies inline.

(In reply to Darin Adler from comment #3)
> Comment on attachment 339512 [details]
> Patch
> 
> r=me as is, but I have some concerns
> 
> I also don’t know how I feel about the idea of pasting multiple links as if
> they were text separated by newlines. I guess someone asked for that
> behavior. Is there a similar case in the iOS or macOS UI that we are
> aligning with? It’s particularly strange that it can break a word or
> sentence into multiple lines.
> 

Yeah, the motivation for this comes from Lena in QA, who felt that dropping multiple links in a single line was a bug. The closest platform analogy I can find to this behavior is dropping multiple links into Messages, which puts each link on a different line. I was primarily focused on addressing her feedback than matching the platform since this is already a very different experience in Notes, where we would drop as multiple rich links on different lines. I think the Notes app is also the closest (perhaps the only?) analogy to Mail in terms of editing behaviors on iPad.

Note that the change here does not take effect when one is dropping a selection that contains multiple links, but rather when links are written to the pasteboard as separate items. An example of this is starting a drag on something in News, and then tapping links to add them to the drag session (in a drag flock). Given this, I don't think there's a case where this change would cause us to end up breaking a word or sentence into multiple lines when dropping.

> I am not confident about the choice of a <br> element as a separator between
> multiple links to achieve having each on a separate line. I think the
> analogy might be to pasting text with newlines, and that does something
> different, does not always turn the newlines into <br> elements. See the
> createFragmentFromText function for details. It uses paragraph elements in
> some common cases. Inserting a line break by typing does a different thing.
> See the  InsertLineBreakCommand::doApply function and note that it will use
> a newline character rather than a <br> in cases like inside a <pre> element.
> Hitting the Return will do a third different thing. See the
> InsertParagraphSeparator::doApply function.
> 

Good catch — it looks like createFragmentFromText is really what I should've used here...

> This code doesn’t seem to do the right thing when putting multiple links
> into a non-richly-editable location. Seems like it will put the URLs all
> next to each other without even spaces separating them?
> 

Indeed! I didn't fix this in this patch because it didn't impact the bug reported here, but I think it's relevant enough that I could address this here anyways. I'll do this, and add a new API test where we drop multiple URLs into a plain text area.

> Not new to this patch: I don’t understand why the macOS and iOS versions of
> WebContentReader::readURL are so different from each other. They do not seem
> to be limited to platform specifics. It’s a sort of “bad code smell" that
> the additional sophistication in the iOS file seems like it comprises all
> improvements that would apply equally well to macOS and even other
> platforms. We should factor this code better. I guess I’d like to see most
> of this logic be entirely platform independent. It’s not good to have the
> platforms be diverging like this.
> 

Totally agree — these implementations of readURL (in WebContentReaderMac and WebContentReaderIOS) are so similar, we should just unify them. There's some special handling to bail when transforming file URLs into web content on iOS, but that's about it. I'll add a FIXME for this.

> Not new to this patch: It seems not so great to have the readURL function do
> the "append" logic as well as the logic for creating an element for a single
> link. Perhaps I don’t understand the factoring well enough here, but it
> seems that we should separate code that knows the platform-specific rules
> for how to create an anchor for a link from the code that knows how to
> insert the multiple anchors with appropriate separators.

That's a good point. The responsibility of WebContentReader is to transform all the items on the pasteboard into a single combined fragment to be inserted into the document via editing. As you noted, readURL is a strange place for logic that is concerned with emitting separators between items on the pasteboard. One way to improve this would be to add something like WebContentReader::insertSeparatorBetweenItems(PasteboardItemType firstItemType, PasteboardItemType secondItemType) where PasteboardItemType indicates what kind of content it is (e.g. link, selection, image, etc.). In PasteboardIOS and PasteboardMac, we'd then invoke this before calling WebContentReader::read*.

I think the refactoring above would be a good followup to this, but I'd prefer to keep this patch targeted to address the case posed in <rdar://problem/35756912>.

[Darin Adler]

(In reply to Wenson Hsieh from comment #5)
> > r=me as is, but I have some concerns
>
> I think the refactoring above would be a good followup to this, but I'd
> prefer to keep this patch targeted to address the case posed in
> <rdar://problem/35756912>.

Sounds fine. That’s part of what I meant when I said “r=me as is”.

[Wenson Hsieh]

(In reply to Darin Adler from comment #6)
> (In reply to Wenson Hsieh from comment #5)
> > > r=me as is, but I have some concerns
> >
> > I think the refactoring above would be a good followup to this, but I'd
> > prefer to keep this patch targeted to address the case posed in
> > <rdar://problem/35756912>.
> 
> Sounds fine. That’s part of what I meant when I said “r=me as is”.

Ok!

On a related note, I worked towards getting plain text drops to insert links on separate lines, but discovered that there's quite a bit of refactoring involved in getting that to work. In fact, our current behavior when dropping multiple URLs in a plain text area is to only insert the *first* URL, because Pasteboard::read(PasteboardPlainText&) just reads from item index 0, and then DragData::asPlainText() just calls Pasteboard::read to figure out what text to insert.

To prevent this fix from snowballing, I filed https://bugs.webkit.org/show_bug.cgi?id=185338, and I'll be following up there.

[Wenson Hsieh]

Created attachment 339626 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 339626 [details]
Patch for landing

Clearing flags on attachment: 339626

Committed r231396: <https://trac.webkit.org/changeset/231396>