Bug 201549 - Incorrect selection rect revealed after pasting images in a contenteditable element
Summary: Incorrect selection rect revealed after pasting images in a contenteditable e...
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: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-06 10:57 PDT by Wenson Hsieh
Modified: 2019-09-06 20:06 PDT (History)
7 users (show)

See Also:


Attachments
Patch (30.92 KB, patch)
2019-09-06 12:27 PDT, Wenson Hsieh
simon.fraser: review+
Details | Formatted Diff | Diff
For EWS (29.91 KB, patch)
2019-09-06 17:20 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2019-09-06 10:57:29 PDT
<rdar://problem/50956429>
Comment 1 Wenson Hsieh 2019-09-06 12:27:52 PDT
Created attachment 378216 [details]
Patch
Comment 2 Simon Fraser (smfr) 2019-09-06 13:15:29 PDT
Comment on attachment 378216 [details]
Patch

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

> Source/WebCore/editing/Editing.cpp:1313
> +    for (TextIterator iterator(&range); !iterator.atEnd(); iterator.advance()) {

Is TextIterator the most efficient way to find image elements?

> Source/WebCore/editing/Editing.cpp:1319
> +        if (cachedImage && cachedImage->image() && cachedImage->image()->isNull())

Does isNull() guarantee that you'll get a load happening later?

> Source/WebCore/editing/Editor.cpp:1593
> +    document().updateLayout();
> +    revealSelectionAfterEditingOperation();

This is the kind of thing that should be queued and happen at "update rendering" time.

> Source/WebCore/page/FrameView.cpp:2452
> +void FrameView::renderLayerDidScroll(const RenderLayer& layer)
> +{
> +    if (!m_frame->isWaitingToRevealSelection())
> +        return;
> +
> +    auto startContainer = makeRefPtr(m_frame->selection().selection().start().containerNode());
> +    if (!startContainer)
> +        return;
> +
> +    auto* startContainerRenderer = startContainer->renderer();
> +    if (!startContainerRenderer)
> +        return;
> +
> +    // FIXME: Ideally, this would also cancel deferred selection revealing if the selection is inside a subframe and a parent frame is scrolled.
> +    for (auto* enclosingLayer = startContainerRenderer->enclosingLayer(); enclosingLayer; enclosingLayer = enclosingLayer->parent()) {
> +        if (enclosingLayer == &layer) {
> +            m_frame->cancelWaitingToRevealSelection();
> +            break;
> +        }
> +    }

Not sure this is the best place for this. Long term, FrameView needs to not know about RenderLayers. The RenderLayer tree walk here seems like the wrong place for the code too. Why not just tell Editor that the scroll happened?

> Source/WebCore/page/Page.cpp:2987
>  void Page::didFinishLoadingImageForElement(HTMLImageElement& element)

Does this get called for failed loads?
Comment 3 Wenson Hsieh 2019-09-06 16:31:34 PDT
Comment on attachment 378216 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/editing/Editing.cpp:1313
>> +    for (TextIterator iterator(&range); !iterator.atEnd(); iterator.advance()) {
> 
> Is TextIterator the most efficient way to find image elements?

It might not be, but it does enable us to:
- only grab image elements that are visible
- iterate only over elements in the given Range (as opposed to all the ancestors of a node)

It’s probably okay for now, since this code is not performance-sensitive.

>> Source/WebCore/editing/Editing.cpp:1319
>> +        if (cachedImage && cachedImage->image() && cachedImage->image()->isNull())
> 
> Does isNull() guarantee that you'll get a load happening later?

Oh, good point — this should’ve been checking cachedImage->isLoading() instead. I’ve changed it to use that instead.

>> Source/WebCore/editing/Editor.cpp:1593
>> +    revealSelectionAfterEditingOperation();
> 
> This is the kind of thing that should be queued and happen at "update rendering" time.

Yeah :/

I think once that mechanism is introduced, we should remove the layout update, and make either revealSelectionAfterEditingOperation (or FrameSelection::revealSelection) schedule the scrolling for the next rendering update. I added a FIXME for this here.

>> Source/WebCore/page/FrameView.cpp:2452
>> +    }
> 
> Not sure this is the best place for this. Long term, FrameView needs to not know about RenderLayers. The RenderLayer tree walk here seems like the wrong place for the code too. Why not just tell Editor that the scroll happened?

Sounds good — I’ll refactor this to tell Editor that the scroll happened.

>> Source/WebCore/page/Page.cpp:2987
>>  void Page::didFinishLoadingImageForElement(HTMLImageElement& element)
> 
> Does this get called for failed loads?

Yes, it does (as long as we began loading, which should be the case if isLoading() is true).
Comment 4 Wenson Hsieh 2019-09-06 17:20:45 PDT
Created attachment 378255 [details]
For EWS
Comment 5 WebKit Commit Bot 2019-09-06 19:58:19 PDT
Comment on attachment 378255 [details]
For EWS

Clearing flags on attachment: 378255

Committed r249605: <https://trac.webkit.org/changeset/249605>