Incorrect selection rect revealed after pasting images in a contenteditable element

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/50956429>

[Wenson Hsieh]

Created attachment 378216 [details]
Patch

[Simon Fraser (smfr)]

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?

[Wenson Hsieh]

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).

[Wenson Hsieh]

Created attachment 378255 [details]
For EWS

[WebKit Commit Bot]

Comment on attachment 378255 [details]
For EWS

Clearing flags on attachment: 378255

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