Bug 198243

Summary: [iOS] Dropped text, attachments, and images should animate into place
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: HTML EditingAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bdakin, commit-queue, darin, dino, sabouhallawa, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 198242    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Fix Win build/review comment
none
Fix watchOS too none

Description Wenson Hsieh 2019-05-24 22:30:43 PDT
<rdar://problem/35205373>
Comment 1 Wenson Hsieh 2019-05-26 01:13:59 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2019-05-26 01:16:25 PDT
Created attachment 370646 [details]
Patch
Comment 3 Tim Horton 2019-05-26 02:06:21 PDT
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?
Comment 4 Wenson Hsieh 2019-05-26 02:39:09 PDT
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.
Comment 5 Wenson Hsieh 2019-05-26 02:56:59 PDT
Created attachment 370647 [details]
Fix Win build/review comment
Comment 6 Wenson Hsieh 2019-05-26 03:47:59 PDT
Comment on attachment 370647 [details]
Fix Win build/review comment

Oops, this breaks watchOS builds.
Comment 7 Wenson Hsieh 2019-05-26 03:52:17 PDT
Created attachment 370648 [details]
Fix watchOS too
Comment 8 WebKit Commit Bot 2019-05-26 04:31:56 PDT
Comment on attachment 370648 [details]
Fix watchOS too

Clearing flags on attachment: 370648

Committed r245778: <https://trac.webkit.org/changeset/245778>
Comment 9 WebKit Commit Bot 2019-05-26 04:31:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Darin Adler 2019-05-28 09:12:09 PDT
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".
Comment 11 Wenson Hsieh 2019-05-28 09:21:21 PDT
(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.