Bug 234113 – Add test case for execCommand to verify inserting an ordered list and deletion

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Brandon]

Do not immediately execute an event if another event is ongoing

[Brandon]

<rdar://86153015>

[Brandon]

Created attachment 446644 [details]
patch

[Chris Dumez]

Comment on attachment 446644 [details]
patch

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

> LayoutTests/ChangeLog:3
> +        Do not immediately execute an event if another event is ongoing

The title doesn't match the patch.

Should be something like:
The load & error events of an <img> element should be dispatched asynchronously.

[Chris Dumez]

Comment on attachment 446644 [details]
patch

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

> LayoutTests/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=234113

Please add the radar number on the next line:
<rdar://86153015>

[Chris Dumez]

Comment on attachment 446644 [details]
patch

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

> Source/WebCore/html/HTMLImageLoader.cpp:64
> +    element().document().queueTaskToDispatchEvent(TaskSource::DOMManipulation, WTFMove(event));

I was wrong, despite us setting the event target, Document::queueTaskToDispatchEvent() will dispatch the event at the Document (not the image element).
You'll likely need to do something like this instead:
```
element.document().eventLoop().queueTask(TaskSource::DOMManipulation, [element = Ref { element() }, errorOccurred] {
    element.dispatchEvent(Event::create(errorOccurred ? eventNames().errorEvent : eventNames().loadEvent, Event::CanBubble::No, Event::IsCancelable::No));
});
```

[Brandon]

<rdar://85926354>

Radar number should be this one instead.

[Brandon]

Created attachment 446794 [details]
patch

[Chris Dumez]

Comment on attachment 446794 [details]
patch

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

> Source/WebCore/html/HTMLImageLoader.cpp:65
> +    element().document().eventLoop().queueTask(TaskSource::DOMManipulation, [element = Ref { element() }, errorOccurred] {

Many tests are failing. I think the issue is that the page load event can now fire *before* an image's load/error event. We likely need to delay the firing of the page's load event somehow. Or maybe we need to make something asynchronous earlier in the call stack instead.
Do you have a stack trace of this event getting called synchronously in the context of your test? It would help determinate how to best fix this.

[Chris Dumez]

Comment on attachment 446794 [details]
patch

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

>> Source/WebCore/html/HTMLImageLoader.cpp:65
>> +    element().document().eventLoop().queueTask(TaskSource::DOMManipulation, [element = Ref { element() }, errorOccurred] {
> 
> Many tests are failing. I think the issue is that the page load event can now fire *before* an image's load/error event. We likely need to delay the firing of the page's load event somehow. Or maybe we need to make something asynchronous earlier in the call stack instead.
> Do you have a stack trace of this event getting called synchronously in the context of your test? It would help determinate how to best fix this.

Yes, I think it is likely one of the callers need to be fixed instead.

You can see that some callers already call this function asynchronously (e.g. via `loadEventSender().dispatchEventSoon(*this);`) so adding a queueTask() here delays it one run loop iteration further.

[Brandon]

Created attachment 447049 [details]
patch

[Brandon]

Created attachment 447051 [details]
patch

[Chris Dumez]

Comment on attachment 447051 [details]
patch

r=me assuming the bots are happy.

[EWS]

Committed r287029 (245232@main): <https://commits.webkit.org/245232@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447051 [details].