Bug 198243 – [iOS] Dropped text, attachments, and images should animate into place

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/35205373>

[Wenson Hsieh]

Created attachment 370645 [details]
Patch

[Wenson Hsieh]

Created attachment 370646 [details]
Patch

[Tim Horton]

Comment on attachment 370646 [details]
Patch

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

> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:200
> +    UITargetedDragPreview *preview = createTargetedDragPreview(textIndicatorImage.get(), contentView, previewContainer, indicator.textBoundingRectInRootViewCoordinates, indicator.textRectsInBoundingRectCoordinates, [UIColor colorWithCGColor:cachedCGColor(indicator.estimatedBackgroundColor)], nil);

A -create method that doesn't return a smart pointer? Weird.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:844
> +        for (TextIterator iterator(range.get()); !iterator.atEnd(); iterator.advance()) {

Why TextIterator? I think just one of the nice DOM child traversal things would be fine, no?

[Wenson Hsieh]

Comment on attachment 370646 [details]
Patch

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

Thanks for taking a look!

It seems this patch breaks the windows build, so I'll fix that as well.

>> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:200
>> +    UITargetedDragPreview *preview = createTargetedDragPreview(textIndicatorImage.get(), contentView, previewContainer, indicator.textBoundingRectInRootViewCoordinates, indicator.textRectsInBoundingRectCoordinates, [UIColor colorWithCGColor:cachedCGColor(indicator.estimatedBackgroundColor)], nil);
> 
> A -create method that doesn't return a smart pointer? Weird.

Weird indeed! Changed to return a RetainPtr.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:844
>> +        for (TextIterator iterator(range.get()); !iterator.atEnd(); iterator.advance()) {
> 
> Why TextIterator? I think just one of the nice DOM child traversal things would be fine, no?

Ah, I chose TextIterator so that it would (1) skip image elements that are hidden/not displayed, and (2) it was easier to extract just elements inside the range, whereas the child traversal helpers (AFAIK) only operate on whole nodes, rather than start/end positions.

[Wenson Hsieh]

Created attachment 370647 [details]
Fix Win build/review comment

[Wenson Hsieh]

Comment on attachment 370647 [details]
Fix Win build/review comment

Oops, this breaks watchOS builds.

[Wenson Hsieh]

Created attachment 370648 [details]
Fix watchOS too

[WebKit Commit Bot]

Comment on attachment 370648 [details]
Fix watchOS too

Clearing flags on attachment: 370648

Committed r245778: <https://trac.webkit.org/changeset/245778>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 370648 [details]
Fix watchOS too

Seems unfortunate to have special casing for the image elements. It’s possible for other elements to be used entirely for the purpose of displaying an image, for example with CSS as a background image or content of an image. Typically we want to have code that waits for images to be loaded to use some more abstract question rather than "is this an HTMLImageElement".

[Wenson Hsieh]

(In reply to Darin Adler from comment #10)
> Comment on attachment 370648 [details]
> Fix watchOS too
> 
> Seems unfortunate to have special casing for the image elements. It’s
> possible for other elements to be used entirely for the purpose of
> displaying an image, for example with CSS as a background image or content
> of an image. Typically we want to have code that waits for images to be
> loaded to use some more abstract question rather than "is this an
> HTMLImageElement".

Yes, that's true.

What we could do instead is make WebPage a CachedImageClient (or have WebPage hold onto a CachedImageClient), and add the CachedImageClient to all cached images in the dropped content range that should load before we try and take a snapshot.

I filed <https://bugs.webkit.org/show_bug.cgi?id=198297> to track this.