⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 249605 in webkit


Ignore:
Timestamp:
Sep 6, 2019, 7:58:12 PM (7 years ago)
Author:
Wenson Hsieh
Message:

Incorrect selection rect revealed after pasting images in a contenteditable element
​https://bugs.webkit.org/show_bug.cgi?id=201549
<rdar://problem/50956429>

Reviewed by Simon Fraser.

Source/WebCore:

Editor::replaceSelectionWithFragment currently scrolls to reveal the selection after inserting the given
DocumentFragment. However, this scrolling occurs before any inserted images have loaded yet, which causes the
wrong caret rect to be revealed, since all image elements inserted during paste will be empty.

To fix this, we defer revealing the selection after inserting the fragment until after all images that have
been inserted are done loading. While waiting for images to load, if any layers which may be scrolled as a
result of revealing the selection are scrolled, we additionally cancel the deferred selection reveal. See
comments below for more detail.

Tests: editing/pasteboard/do-not-reveal-selection-after-programmatic-scroll.html

editing/pasteboard/reveal-selection-after-pasting-images.html
PasteImage.RevealSelectionAfterPastingImage

  • editing/Editing.cpp:

(WebCore::visibleImageElementsInRangeWithNonLoadedImages):

Add a new helper to iterate through a range and collect all image elements in that range, that contain cached
images that have not finished loading yet.

  • editing/Editing.h:
  • editing/Editor.cpp:

(WebCore::Editor::replaceSelectionWithFragment):

Instead of always immediately revealing the selection after applying the ReplaceSelectionCommand, collect the
image elements that were just inserted, and avoid immediately revealing the selection if any of these images
have non-null cached images, but are not loaded yet. Instead, hold on to these images in a set, remove them once
they finish loading using the new method below, and once all images are removed, reveal the selection.

(WebCore::Editor::revealSelectionIfNeededAfterLoadingImageForElement):
(WebCore::Editor::renderLayerDidScroll):

Called whenever a scrollable RenderLayer is scrolled (or in the case of FrameView, the root layer). In the case
where Editor is waiting to reveal the selection, we check to see if the scrolled layer is an ancestor of the
layer enclosing the start of the selection.

(WebCore::Editor::respondToChangedSelection):

If the selection changes between pasting and waiting for pasted images to load, just cancel waiting to reveal
the selection after pasting.

  • editing/Editor.h:
  • editing/ReplaceSelectionCommand.cpp:

(WebCore::ReplaceSelectionCommand::insertedContentRange const):

Add a helper method to grab the Range of content inserted after applying the command.

  • editing/ReplaceSelectionCommand.h:
  • page/FrameView.cpp:

(WebCore::FrameView::scrollPositionChanged):

  • page/FrameView.h:
  • page/Page.cpp:

(WebCore::Page::didFinishLoadingImageForElement):

Notify Editor after an image finishes loading.

  • rendering/RenderLayer.cpp:

(WebCore::RenderLayer::scrollTo):

Source/WebKit:

Tweak some existing logic to use the new visibleImageElementsInRangeWithNonLoadedImages helper function. See
WebCore for more details.

  • WebProcess/WebPage/ios/WebPageIOS.mm:

(WebKit::WebPage::didConcludeEditDrag):

Tools:

Add an API test to exercise the scenario where we scroll to reveal the selection after pasting an image that was
directly written to the pasteboard.

  • TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:

LayoutTests:

Add a couple of new layout tests.

  • editing/pasteboard/do-not-reveal-selection-after-programmatic-scroll-expected.txt: Added.
  • editing/pasteboard/do-not-reveal-selection-after-programmatic-scroll.html: Added.

This test verifies that we don't try to scroll to reveal the caret after pasting, if the scroll position was
changed before the images finished loading.

  • editing/pasteboard/reveal-selection-after-pasting-images-expected.txt: Added.
  • editing/pasteboard/reveal-selection-after-pasting-images.html: Added.
  • platform/ios/editing/pasteboard/reveal-selection-after-pasting-images-expected.txt: Added.

This test verifies that we reveal the caret after loading multiple pasted images in a selection, and dispatch a
scroll event in the process.

Location:
trunk
Files:
5 added
16 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r249601 r249605  
     12019-09-06  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        Incorrect selection rect revealed after pasting images in a contenteditable element
     4        https://bugs.webkit.org/show_bug.cgi?id=201549
     5        <rdar://problem/50956429>
     6
     7        Reviewed by Simon Fraser.
     8
     9        Add a couple of new layout tests.
     10
     11        * editing/pasteboard/do-not-reveal-selection-after-programmatic-scroll-expected.txt: Added.
     12        * editing/pasteboard/do-not-reveal-selection-after-programmatic-scroll.html: Added.
     13
     14        This test verifies that we don't try to scroll to reveal the caret after pasting, if the scroll position was
     15        changed before the images finished loading.
     16
     17        * editing/pasteboard/reveal-selection-after-pasting-images-expected.txt: Added.
     18        * editing/pasteboard/reveal-selection-after-pasting-images.html: Added.
     19        * platform/ios/editing/pasteboard/reveal-selection-after-pasting-images-expected.txt: Added.
     20
     21        This test verifies that we reveal the caret after loading multiple pasted images in a selection, and dispatch a
     22        scroll event in the process.
     23
    1242019-09-06  Justin Fan  <justin_fan@apple.com>
    225

  • trunk/Source/WebCore/ChangeLog

    r249604 r249605  
     12019-09-06  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        Incorrect selection rect revealed after pasting images in a contenteditable element
     4        https://bugs.webkit.org/show_bug.cgi?id=201549
     5        <rdar://problem/50956429>
     6
     7        Reviewed by Simon Fraser.
     8
     9        Editor::replaceSelectionWithFragment currently scrolls to reveal the selection after inserting the given
     10        DocumentFragment. However, this scrolling occurs before any inserted images have loaded yet, which causes the
     11        wrong caret rect to be revealed, since all image elements inserted during paste will be empty.
     12
     13        To fix this, we defer revealing the selection after inserting the fragment until after all images that have
     14        been inserted are done loading. While waiting for images to load, if any layers which may be scrolled as a
     15        result of revealing the selection are scrolled, we additionally cancel the deferred selection reveal. See
     16        comments below for more detail.
     17
     18        Tests: editing/pasteboard/do-not-reveal-selection-after-programmatic-scroll.html
     19               editing/pasteboard/reveal-selection-after-pasting-images.html
     20               PasteImage.RevealSelectionAfterPastingImage
     21
     22        * editing/Editing.cpp:
     23        (WebCore::visibleImageElementsInRangeWithNonLoadedImages):
     24
     25        Add a new helper to iterate through a range and collect all image elements in that range, that contain cached
     26        images that have not finished loading yet.
     27
     28        * editing/Editing.h:
     29        * editing/Editor.cpp:
     30        (WebCore::Editor::replaceSelectionWithFragment):
     31
     32        Instead of always immediately revealing the selection after applying the ReplaceSelectionCommand, collect the
     33        image elements that were just inserted, and avoid immediately revealing the selection if any of these images
     34        have non-null cached images, but are not loaded yet. Instead, hold on to these images in a set, remove them once
     35        they finish loading using the new method below, and once all images are removed, reveal the selection.
     36
     37        (WebCore::Editor::revealSelectionIfNeededAfterLoadingImageForElement):
     38        (WebCore::Editor::renderLayerDidScroll):
     39
     40        Called whenever a scrollable RenderLayer is scrolled (or in the case of FrameView, the root layer). In the case
     41        where Editor is waiting to reveal the selection, we check to see if the scrolled layer is an ancestor of the
     42        layer enclosing the start of the selection.
     43
     44        (WebCore::Editor::respondToChangedSelection):
     45
     46        If the selection changes between pasting and waiting for pasted images to load, just cancel waiting to reveal
     47        the selection after pasting.
     48
     49        * editing/Editor.h:
     50        * editing/ReplaceSelectionCommand.cpp:
     51        (WebCore::ReplaceSelectionCommand::insertedContentRange const):
     52
     53        Add a helper method to grab the Range of content inserted after applying the command.
     54
     55        * editing/ReplaceSelectionCommand.h:
     56        * page/FrameView.cpp:
     57        (WebCore::FrameView::scrollPositionChanged):
     58        * page/FrameView.h:
     59        * page/Page.cpp:
     60        (WebCore::Page::didFinishLoadingImageForElement):
     61
     62        Notify Editor after an image finishes loading.
     63
     64        * rendering/RenderLayer.cpp:
     65        (WebCore::RenderLayer::scrollTo):
     66
    1672019-09-06  Brent Fulgham  <bfulgham@apple.com>
    268

  • trunk/Source/WebCore/editing/Editing.cpp

    r249565 r249605  
    2828
    2929#include "AXObjectCache.h"
     30#include "CachedImage.h"
    3031#include "Document.h"
    3132#include "Editor.h"
    … …  
    3536#include "HTMLDivElement.h"
    3637#include "HTMLElementFactory.h"
     38#include "HTMLImageElement.h"
    3739#include "HTMLInterchange.h"
    3840#include "HTMLLIElement.h"
    … …  
    13031305}
    13041306
     1307HashSet<RefPtr<HTMLImageElement>> visibleImageElementsInRangeWithNonLoadedImages(const Range& range)
     1308{
     1309    HashSet<RefPtr<HTMLImageElement>> result;
     1310    for (TextIterator iterator(&range); !iterator.atEnd(); iterator.advance()) {
     1311        if (!is<HTMLImageElement>(iterator.node()))
     1312            continue;
     1313
     1314        auto& imageElement = downcast<HTMLImageElement>(*iterator.node());
     1315        auto* cachedImage = imageElement.cachedImage();
     1316        if (cachedImage && cachedImage->isLoading())
     1317            result.add(&imageElement);
     1318    }
     1319    return result;
     1320}
     1321
    13051322} // namespace WebCore

  • trunk/Source/WebCore/editing/Editing.h

    r246490 r249605  
    2828#include "Position.h"
    2929#include <wtf/Forward.h>
     30#include <wtf/HashSet.h>
    3031#include <wtf/unicode/CharacterNames.h>
    3132
    … …  
    3435class Document;
    3536class HTMLElement;
     37class HTMLImageElement;
    3638class HTMLSpanElement;
    3739class HTMLTextFormControlElement;
    … …  
    103105bool positionBeforeOrAfterNodeIsCandidate(Node&);
    104106
     107WEBCORE_EXPORT HashSet<RefPtr<HTMLImageElement>> visibleImageElementsInRangeWithNonLoadedImages(const Range&);
     108
    105109// -------------------------------------------------------------------------
    106110// Position

  • trunk/Source/WebCore/editing/Editor.cpp

    r248846 r249605  
    8787#include "RemoveFormatCommand.h"
    8888#include "RenderBlock.h"
     89#include "RenderLayer.h"
    8990#include "RenderTextControl.h"
    9091#include "RenderedDocumentMarker.h"
    … …  
    675676    auto command = ReplaceSelectionCommand::create(document(), &fragment, options, editingAction);
    676677    command->apply();
    677     revealSelectionAfterEditingOperation();
     678
     679    m_imageElementsToLoadBeforeRevealingSelection.clear();
     680    if (auto insertionRange = command->insertedContentRange())
     681        m_imageElementsToLoadBeforeRevealingSelection = visibleImageElementsInRangeWithNonLoadedImages(*insertionRange);
     682
     683    if (m_imageElementsToLoadBeforeRevealingSelection.isEmpty())
     684        revealSelectionAfterEditingOperation();
    678685
    679686    selection = m_frame.selection().selection();
    … …  
    15711578
    15721579#endif
     1580
     1581void Editor::revealSelectionIfNeededAfterLoadingImageForElement(HTMLImageElement& element)
     1582{
     1583    if (m_imageElementsToLoadBeforeRevealingSelection.isEmpty())
     1584        return;
     1585
     1586    if (!m_imageElementsToLoadBeforeRevealingSelection.remove(&element))
     1587        return;
     1588
     1589    if (!m_imageElementsToLoadBeforeRevealingSelection.isEmpty())
     1590        return;
     1591
     1592    // FIXME: This should be queued as a task for the next rendering update.
     1593    document().updateLayout();
     1594    revealSelectionAfterEditingOperation();
     1595}
     1596
     1597void Editor::renderLayerDidScroll(const RenderLayer& layer)
     1598{
     1599    if (m_imageElementsToLoadBeforeRevealingSelection.isEmpty())
     1600        return;
     1601
     1602    auto startContainer = makeRefPtr(m_frame.selection().selection().start().containerNode());
     1603    if (!startContainer)
     1604        return;
     1605
     1606    auto* startContainerRenderer = startContainer->renderer();
     1607    if (!startContainerRenderer)
     1608        return;
     1609
     1610    // FIXME: Ideally, this would also cancel deferred selection revealing if the selection is inside a subframe and a parent frame is scrolled.
     1611    for (auto* enclosingLayer = startContainerRenderer->enclosingLayer(); enclosingLayer; enclosingLayer = enclosingLayer->parent()) {
     1612        if (enclosingLayer == &layer) {
     1613            m_imageElementsToLoadBeforeRevealingSelection.clear();
     1614            break;
     1615        }
     1616    }
     1617}
    15731618
    15741619bool Editor::isContinuousSpellCheckingEnabled() const
    … …  
    35763621
    35773622    setStartNewKillRingSequence(true);
     3623    m_imageElementsToLoadBeforeRevealingSelection.clear();
    35783624
    35793625    if (m_editorUIUpdateTimer.isActive())

  • trunk/Source/WebCore/editing/Editor.h

    r243195 r249605  
    7373class Pasteboard;
    7474class PasteboardWriterData;
     75class RenderLayer;
    7576class SharedBuffer;
    7677class Font;
    … …  
    183184    WEBCORE_EXPORT void copyImage(const HitTestResult&);
    184185#endif
     186
     187    void renderLayerDidScroll(const RenderLayer&);
     188    void revealSelectionIfNeededAfterLoadingImageForElement(HTMLImageElement&);
    185189
    186190    String readPlainTextFromPasteboard(Pasteboard&);
    … …  
    629633
    630634    bool m_isGettingDictionaryPopupInfo { false };
     635    HashSet<RefPtr<HTMLImageElement>> m_imageElementsToLoadBeforeRevealingSelection;
    631636};
    632637

  • trunk/Source/WebCore/editing/ReplaceSelectionCommand.cpp

    r249517 r249605  
    17721772}
    17731773
     1774RefPtr<Range> ReplaceSelectionCommand::insertedContentRange() const
     1775{
     1776    if (auto document = makeRefPtr(m_startOfInsertedContent.document()))
     1777        return Range::create(*document, m_startOfInsertedContent, m_endOfInsertedContent);
     1778
     1779    return nullptr;
     1780}
     1781
    17741782} // namespace WebCore

  • trunk/Source/WebCore/editing/ReplaceSelectionCommand.h

    r249517 r249605  
    3232
    3333class DocumentFragment;
     34class Range;
    3435class ReplacementFragment;
    3536
    … …  
    5253
    5354    VisibleSelection visibleSelectionForInsertedText() const { return m_visibleSelectionForInsertedText; }
     55
     56    RefPtr<Range> insertedContentRange() const;
    5457
    5558private:

  • trunk/Source/WebCore/page/FrameView.cpp

    r249080 r249605  
    4141#include "DocumentLoader.h"
    4242#include "DocumentMarkerController.h"
     43#include "Editor.h"
    4344#include "EventHandler.h"
    4445#include "EventNames.h"
    … …  
    24242425    updateLayoutViewport();
    24252426    viewportContentsChanged();
     2427
     2428    if (auto* renderView = this->renderView()) {
     2429        if (auto* layer = renderView->layer())
     2430            frame().editor().renderLayerDidScroll(*layer);
     2431    }
    24262432}
    24272433

  • trunk/Source/WebCore/page/FrameView.h

    r247839 r249605  
    660660    GraphicsLayer* layerForVerticalScrollbar() const final;
    661661
     662    void renderLayerDidScroll(const RenderLayer&);
     663
    662664protected:
    663665    bool scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect) final;

  • trunk/Source/WebCore/page/Page.cpp

    r249575 r249605  
    29792979void Page::didFinishLoadingImageForElement(HTMLImageElement& element)
    29802980{
     2981    auto protectedElement = makeRef(element);
     2982    if (auto frame = makeRefPtr(element.document().frame()))
     2983        frame->editor().revealSelectionIfNeededAfterLoadingImageForElement(element);
    29812984    chrome().client().didFinishLoadingImageForElement(element);
    29822985}

  • trunk/Source/WebCore/rendering/RenderLayer.cpp

    r249434 r249605  
    5656#include "DocumentMarkerController.h"
    5757#include "DocumentTimeline.h"
     58#include "Editor.h"
    5859#include "Element.h"
    5960#include "EventHandler.h"
    … …  
    26102611
    26112612    view.frameView().viewportContentsChanged();
     2613    frame.editor().renderLayerDidScroll(*this);
    26122614}
    26132615

  • trunk/Source/WebKit/ChangeLog

    r249603 r249605  
     12019-09-06  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        Incorrect selection rect revealed after pasting images in a contenteditable element
     4        https://bugs.webkit.org/show_bug.cgi?id=201549
     5        <rdar://problem/50956429>
     6
     7        Reviewed by Simon Fraser.
     8
     9        Tweak some existing logic to use the new visibleImageElementsInRangeWithNonLoadedImages helper function. See
     10        WebCore for more details.
     11
     12        * WebProcess/WebPage/ios/WebPageIOS.mm:
     13        (WebKit::WebPage::didConcludeEditDrag):
     14
    1152019-09-06  Chris Dumez  <cdumez@apple.com>
    216

  • trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm

    r249508 r249605  
    909909    m_pendingImageElementsForDropSnapshot.clear();
    910910
    911     bool waitingForAnyImageToLoad = false;
    912911    auto frame = makeRef(m_page->focusController().focusedOrMainFrame());
    913912    if (auto selectionRange = frame->selection().selection().toNormalizedRange()) {
    914         for (TextIterator iterator(selectionRange.get()); !iterator.atEnd(); iterator.advance()) {
    915             auto* node = iterator.node();
    916             if (!is<HTMLImageElement>(node))
    917                 continue;
    918 
    919             auto& imageElement = downcast<HTMLImageElement>(*node);
    920             auto* cachedImage = imageElement.cachedImage();
    921             if (cachedImage && cachedImage->image() && cachedImage->image()->isNull()) {
    922                 m_pendingImageElementsForDropSnapshot.add(&imageElement);
    923                 waitingForAnyImageToLoad = true;
    924             }
    925         }
     913        m_pendingImageElementsForDropSnapshot = visibleImageElementsInRangeWithNonLoadedImages(*selectionRange);
    926914        auto collapsedRange = Range::create(selectionRange->ownerDocument(), selectionRange->endPosition(), selectionRange->endPosition());
    927915        frame->selection().setSelectedRange(collapsedRange.ptr(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
    … …  
    930918    }
    931919
    932     if (!waitingForAnyImageToLoad)
     920    if (m_pendingImageElementsForDropSnapshot.isEmpty())
    933921        computeAndSendEditDragSnapshot();
    934922}

  • trunk/Tools/ChangeLog

    r249602 r249605  
     12019-09-06  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        Incorrect selection rect revealed after pasting images in a contenteditable element
     4        https://bugs.webkit.org/show_bug.cgi?id=201549
     5        <rdar://problem/50956429>
     6
     7        Reviewed by Simon Fraser.
     8
     9        Add an API test to exercise the scenario where we scroll to reveal the selection after pasting an image that was
     10        directly written to the pasteboard.
     11
     12        * TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:
     13
    1142019-09-06  Matt Lewis  <jlewis3@apple.com>
    215

  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm

    r242339 r249605  
    3434
    3535#if PLATFORM(IOS_FAMILY)
    36 #include <MobileCoreServices/MobileCoreServices.h>
     36#import <MobileCoreServices/MobileCoreServices.h>
    3737#endif
    3838
    … …  
    138138}
    139139
     140TEST(PasteImage, RevealSelectionAfterPastingImage)
     141{
     142    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);
     143    [webView synchronouslyLoadHTMLString:@"<meta name='viewport' content='width=device-width, initial-scale=1'><body contenteditable>Hello world</body>"];
     144    [webView stringByEvaluatingJavaScript:@"document.body.focus()"];
     145    [webView _synchronouslyExecuteEditCommand:@"InsertText" argument:@"Hello world"];
     146    [webView _synchronouslyExecuteEditCommand:@"InsertParagraph" argument:nil];
     147
     148    writeImageDataToPasteboard((__bridge NSString *)kUTTypeJPEG, [NSData dataWithContentsOfFile:[[NSBundle mainBundle] pathForResource:@"sunset-in-cupertino-600px" ofType:@"jpg" inDirectory:@"TestWebKitAPI.resources"]]);
     149    [webView paste:nil];
     150
     151    while ([[webView stringByEvaluatingJavaScript:@"document.scrollingElement.scrollTop"] doubleValue] <= 0)
     152        [NSRunLoop.currentRunLoop runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.05]];
     153}
     154
    140155#if PLATFORM(MAC)
    141156void writeBundleFileToPasteboard(id object)
Note: See TracChangeset for help on using the changeset viewer.