Fire a load event for network errors in <embed> #4247

domenic · 2018-12-19T22:11:16Z

Fixes #4024.

At least two implementers are interested (and none opposed):
- Chrome implements these semantics
- Firefox and Safari fire a load event for an X-Frame-Options network error but fires no events for non-resolving hosts
- Below Firefox says this sounds reasonable
Tests are written and can be reviewed and commented upon at:
- Add a test for network errors with <embed> web-platform-tests/wpt#24518
Implementation bugs are filed:
- Chrome: N/A
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1653014
- Safari: https://bugs.webkit.org/show_bug.cgi?id=214357

(See WHATWG Working Mode: Changes for more details.)

/iframe-embed-object.html ( diff )

source

Fixes #4024.

Follows whatwg/html#4247.

domenic · 2020-07-08T21:05:50Z

I resurrected this after some time, and wrote tests.

It turns out that only some network errors generate load events in Firefox and Safari. Since I think distinguishing types of network errors is not something we'd want to start doing, I suggest the spec adopt Chrome's semantics. What do you think, @annevk?

Follows whatwg/html#4247.

annevk · 2020-07-09T11:03:57Z

See #1230 and #125. I tend to agree that a navigation that results in a network error (whatever the cause) should use a load event given that Chrome shipped that. There are many tests that could be updated if all user agents aligned on that.

But this is bigger than just embed.

domenic · 2020-07-09T17:00:07Z

Hmm, right. So it seems like there's at least some world where we move frame-ancestors/X-Frame-Options into navigate, and treat it as a "load" of an error page (and fire a load event), whereas other network errors go down a different path, and never fire a load event. In which case it's possible we could end up with the Firefox/Safari behavior. But, we'd kind of like to start firing load events for all network errors everywhere, if possible, so hopefully we won't go that way.

I might want to tackle this larger issue, or at least f-a/X-F-O, as part of cleaning up the foundations so that portals can use them properly. But in the meantime, what should we do with this PR?

I'd like to put in a vote for merging, as it fixes the regression noted in #4024 (and caused by #3900). However, we could hold off on filing bugs and updating the larger swathe of tests until we have a better handle on the bigger picture. What do you think?

annevk · 2020-07-10T15:03:23Z

I was hoping at least one other Mozilla person could chime in with yes/no. I'm personally fine with small steps towards always firing a load event for navigation contexts.

@mattwoodrow what do you think?

mattwoodrow · 2020-07-13T02:12:06Z

This sounds reasonable to me.

I definitely agree that we want to move towards always firing an event for navigation contexts (though no strong preference on load vs error event).

It'd be nice to do the same for <object> and <iframe> as soon as possible, if we can't change them at the same time.

domenic · 2020-07-13T18:20:00Z

It looks like iframe is already specified to fire a load event when any document is created, including a network error document (created via https://html.spec.whatwg.org/#read-ua-inline).

object elements seem to explicitly fire an error event in step 4.7 of the algorithm at https://html.spec.whatwg.org/#the-object-element (no direct link, sorry).

I will expand the tests to see what the result is for those two...

domenic · 2020-07-13T18:33:29Z

It appears that object is significantly worse than embed and iframe.

Firefox:
- new <object>, nonexistent host: error event
- new <object>, X-Frame-Options prevents embedding: load event
- existing <object>, navigated to nonexistent host: no event
- existing <object>, navigating to a page where X-Frame-Options prevents embedding: load event
Chrome: no event in all cases

whereas with <embed> and <iframe>, Chrome always fires the load event, and Firefox always fires it for X-Frame-Options/never fires it for nonexistent hosts.

My recommendation is that we leave <object> alone for now. When Flash is removed from the web platform, we might be able to revisit it and significantly simplify it to be more like <embed> and <iframe> (e.g., ignore all the classid="" and type="" stuff.) But before that point, I'd rather not touch the <object> spec and test suite.

Does that sound reasonable?

annevk · 2020-07-14T11:00:03Z

I think that's fine.

domenic · 2020-07-14T13:58:31Z

Great. Want to review/approve this PR then? :)

annevk · 2020-07-15T14:27:30Z

source

@@ -30648,6 +30648,10 @@ interface <dfn>HTMLEmbedElement</dfn> : <span>HTMLElement</span> {
       <span>the <code>embed</code> element setup steps</span> for <var>element</var>, then
       return.</p></li>

+       <li><p>If <var>response</var> is a <span>network error</span>, then <span
+       data-x="concept-event-fire">fire an event</span> named <code
+       data-x="event-load">load</code> at <var>element</var>, and return.</p></li>


It seems that the current tests are not testing whether this results in a browsing context/plugin being created or not.

I guess that goes back to the general issue of the embed element being much more like object in implementations than the specification makes it appear.

But given that the note below I suspect this might be inaccurate?

I found this to be inconsistent among Firefox and Chrome :(.

annevk

This would also close #1461 I suppose.

Reluctantly approving as embed is in shambles so it's not clear to me if this is helping or hurting.

domenic · 2020-07-15T16:25:50Z

Thanks :). Web platform tests are merged, and the OP is updated with Gecko and WebKit bugs that I filed.

Follows whatwg/html#4247 for <embed> and existing spec text for <iframe>.

…d>, a=testonly Automatic update from web-platform-tests Add a test for network errors with <embed> and <iframe> Follows whatwg/html#4247 for <embed> and existing spec text for <iframe>. -- wpt-commits: 91b0d35770bcb521c540d08002d8d3700666180c wpt-pr: 24518

…d>, a=testonly Automatic update from web-platform-tests Add a test for network errors with <embed> and <iframe> Follows whatwg/html#4247 for <embed> and existing spec text for <iframe>. -- wpt-commits: 91b0d35770bcb521c540d08002d8d3700666180c wpt-pr: 24518 UltraBlame original commit: 08ab2bb58f49c3dbacd5d3d9c7fa0405f9573f92

domenic added the topic: embed and object label Dec 19, 2018

domfarolino reviewed Dec 20, 2018

View reviewed changes

source Outdated Show resolved Hide resolved

Fire a load event for network errors in <embed>

5dd1288

Fixes #4024.

domenic force-pushed the fix-embed-load-event-regression branch from d3f5326 to 5dd1288 Compare July 8, 2020 20:46

domenic added a commit to web-platform-tests/wpt that referenced this pull request Jul 8, 2020

Add a test for network errors with <embed>

6ee7ade

Follows whatwg/html#4247.

domenic mentioned this pull request Jul 8, 2020

Add a test for network errors with <embed> web-platform-tests/wpt#24518

Merged

domenic requested a review from annevk July 8, 2020 20:56

domenic added a commit to web-platform-tests/wpt that referenced this pull request Jul 8, 2020

Add a test for network errors with <embed>

dd90897

Follows whatwg/html#4247.

domenic mentioned this pull request Jul 10, 2020

activate() should fulfill even if the inner document load failed due to DNS/x-frame-options etc etc WICG/portals#228

Open

jakearchibald mentioned this pull request Jul 10, 2020

Add iframe/window "navigated" event #5725

Open

annevk reviewed Jul 15, 2020

View reviewed changes

annevk approved these changes Jul 15, 2020

View reviewed changes

domenic merged commit 63a0e64 into master Jul 15, 2020

domenic deleted the fix-embed-load-event-regression branch July 15, 2020 16:15

domenic added a commit to web-platform-tests/wpt that referenced this pull request Jul 15, 2020

Add a test for network errors with <embed> and <iframe>

91b0d35

Follows whatwg/html#4247 for <embed> and existing spec text for <iframe>.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fire a load event for network errors in <embed> #4247

Fire a load event for network errors in <embed> #4247

domenic commented Dec 19, 2018 •

edited

Loading

domenic commented Jul 8, 2020

annevk commented Jul 9, 2020

domenic commented Jul 9, 2020

annevk commented Jul 10, 2020

mattwoodrow commented Jul 13, 2020 •

edited by domenic

Loading

domenic commented Jul 13, 2020

domenic commented Jul 13, 2020

annevk commented Jul 14, 2020

domenic commented Jul 14, 2020

annevk Jul 15, 2020

domenic Jul 15, 2020

annevk left a comment

domenic commented Jul 15, 2020

Fire a load event for network errors in <embed> #4247

Fire a load event for network errors in <embed> #4247

Conversation

domenic commented Dec 19, 2018 • edited Loading

domenic commented Jul 8, 2020

annevk commented Jul 9, 2020

domenic commented Jul 9, 2020

annevk commented Jul 10, 2020

mattwoodrow commented Jul 13, 2020 • edited by domenic Loading

domenic commented Jul 13, 2020

domenic commented Jul 13, 2020

annevk commented Jul 14, 2020

domenic commented Jul 14, 2020

annevk Jul 15, 2020

Choose a reason for hiding this comment

domenic Jul 15, 2020

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

domenic commented Jul 15, 2020

domenic commented Dec 19, 2018 •

edited

Loading

mattwoodrow commented Jul 13, 2020 •

edited by domenic

Loading