Implement srcdoc support for iframes #24576

amj23897 · 2019-10-29T03:33:15Z

This PR contains changes related to adding srcdoc attribute parsing support for iframes in Servo. The following changes have been made:

uncomment the srcdoc WebIDL attribute, and implement the attribute getter.
add a field to LoadData for storing the srcdoc contents when loading a srcdoc iframe.
(partially) implemented a new page_load_about_srcdoc method to script_thread.rs which loads the special about:srcdoc URL per the specification and takes the srcdoc contents as an argument
call this new method from handle_new_layout when it's detected that a srcdoc iframe is being loaded
(partially) in attribute_mutated, ensure that changing the srcdoc attribute of an iframe element follows the specification.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes are a part of fix #4767 (GitHub issue number if applicable)

highfive · 2019-10-29T03:33:22Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @ferjm (or someone else) soon.

highfive · 2019-10-29T03:33:23Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmliframeelement.rs, components/script/script_thread.rs, components/script/dom/webidls/HTMLIFrameElement.webidl
@KiChjang: components/script/dom/htmliframeelement.rs, components/script/script_thread.rs, components/script_traits/lib.rs, components/script/dom/webidls/HTMLIFrameElement.webidl

highfive · 2019-10-29T03:33:25Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2019-10-29T03:51:36Z

r? @jdm
I'll answer your questions tomorrow morning! Looks like a good start.

components/script/dom/htmliframeelement.rs

components/script/script_thread.rs

jdm · 2019-11-04T12:43:28Z

@bors-servo try=wpt

bors-servo · 2019-11-04T12:43:33Z

⌛ Trying commit 616bb1f with merge c9158bb...


        Auto merge of #24576 - jaymodi98:iframe-srcdoc, r=<try>

Implement srcdoc support for iframes  This PR contains changes related to adding srcdoc attribute parsing support for iframes in Servo. The following changes have been made: - uncomment the [srcdoc](https://github.com/servo/servo/blob/f63b404e0cbf30380c4043700861110d06e548bb/components/script/dom/webidls/HTMLIFrameElement.webidl#L10-L11) WebIDL attribute, and implement the attribute getter. - add a field to [LoadData](https://github.com/servo/servo/blob/e6b271d32981ad95442c029bf72bd35efc88f9c5/components/script_traits/lib.rs#L137-L164) for storing the srcdoc contents when loading a srcdoc iframe. - (partially) implemented a new `page_load_about_srcdoc` method to `script_thread.rs` which loads the special `about:srcdoc` URL [per the specification](https://html.spec.whatwg.org/multipage/iframe-embed-object.html#process-the-iframe-attributes) and takes the srcdoc contents as an argument - call this new method from [handle_new_layout](https://github.com/servo/servo/blob/e6b271d32981ad95442c029bf72bd35efc88f9c5/components/script/script_thread.rs#L2409-L2412) when it's detected that a srcdoc iframe is being loaded - (partially) in [attribute_mutated](https://github.com/servo/servo/blob/e6b271d32981ad95442c029bf72bd35efc88f9c5/components/script/dom/htmliframeelement.rs#L560), ensure that changing the `srcdoc` attribute of an iframe element [follows the specification](https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-iframe-element:the-iframe-element-9). ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes are a part of fix #4767 (GitHub issue number if applicable)

bors-servo · 2019-11-04T13:10:31Z

💔 Test failed - linux-rel-css

jdm · 2019-11-04T15:15:11Z

components/script/dom/htmliframeelement.rs

+            load_data.srcdoc = self.get_srcdoc();
+            self.navigate_or_reload_child_browsing_context(
+                load_data,
+                NavigationType::Regular,


Ah, here's the missing piece - in navigate_or_reload_child_browsing_context there's a special branch for NavigationType::InitialAboutBlank which ends up calling ScriptThread::handle_new_layout. If you pass that instead of NavigationType::Regular you should end up with more meaningful test results.

The reason this matters is that Regular causes us to go to the network thread and try to make a request for about:srcdoc, which is then not a recognized URL so we get an error page. To quickly verify if this is working correctly or not, use this test page:

<iframe srcdoc="<div>hi!</div><script>foo=5;</script>" onload="console.log(this.contentWindow.foo)"></iframe>

In the current branch, I see an iframe containing an error message. With the change I suggested, there should be an iframe with "hi!" and the terminal should show 5.

I have done the change. But not tested yet.

Done. I can see the iframe and terminal shows 5. But main tests are failing. The command used:
./mach test-wpt tests/wpt/web-platform-tests/html/semantics/embedded-content/the-iframe-element/srcdoc_process_attributes.html

jdm · 2019-11-04T16:03:22Z

@bors-servo try=wpt

bors-servo · 2019-11-04T16:03:28Z

⌛ Trying commit e157126 with merge 2e52d7f...

bors-servo · 2019-11-12T17:13:37Z

💔 Test failed - linux-rel-css

jdm · 2019-11-12T17:55:15Z

@bors-servo try=wpt

bors-servo · 2019-11-12T17:55:20Z

⌛ Trying commit ddd9cd9 with merge dff4717...


        Auto merge of #24576 - jaymodi98:iframe-srcdoc, r=<try>

Implement srcdoc support for iframes  This PR contains changes related to adding srcdoc attribute parsing support for iframes in Servo. The following changes have been made: - uncomment the [srcdoc](https://github.com/servo/servo/blob/f63b404e0cbf30380c4043700861110d06e548bb/components/script/dom/webidls/HTMLIFrameElement.webidl#L10-L11) WebIDL attribute, and implement the attribute getter. - add a field to [LoadData](https://github.com/servo/servo/blob/e6b271d32981ad95442c029bf72bd35efc88f9c5/components/script_traits/lib.rs#L137-L164) for storing the srcdoc contents when loading a srcdoc iframe. - (partially) implemented a new `page_load_about_srcdoc` method to `script_thread.rs` which loads the special `about:srcdoc` URL [per the specification](https://html.spec.whatwg.org/multipage/iframe-embed-object.html#process-the-iframe-attributes) and takes the srcdoc contents as an argument - call this new method from [handle_new_layout](https://github.com/servo/servo/blob/e6b271d32981ad95442c029bf72bd35efc88f9c5/components/script/script_thread.rs#L2409-L2412) when it's detected that a srcdoc iframe is being loaded - (partially) in [attribute_mutated](https://github.com/servo/servo/blob/e6b271d32981ad95442c029bf72bd35efc88f9c5/components/script/dom/htmliframeelement.rs#L560), ensure that changing the `srcdoc` attribute of an iframe element [follows the specification](https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-iframe-element:the-iframe-element-9). ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes are a part of fix #4767 (GitHub issue number if applicable)

bors-servo · 2019-11-12T18:33:04Z

💔 Test failed - linux-rel-wpt


        Implement srcdoc support for iframes.


        dom: Reduce the scope of iframe's load blocker borrow.

jdm · 2019-11-12T18:58:29Z

@bors-servo r+

bors-servo · 2019-11-12T18:58:30Z

📌 Commit af158de has been approved by jdm

bors-servo · 2019-11-12T19:29:22Z

⌛ Testing commit af158de with merge bd0ae59...


        Auto merge of #24576 - jaymodi98:iframe-srcdoc, r=jdm

Implement srcdoc support for iframes  This PR contains changes related to adding srcdoc attribute parsing support for iframes in Servo. The following changes have been made: - uncomment the [srcdoc](https://github.com/servo/servo/blob/f63b404e0cbf30380c4043700861110d06e548bb/components/script/dom/webidls/HTMLIFrameElement.webidl#L10-L11) WebIDL attribute, and implement the attribute getter. - add a field to [LoadData](https://github.com/servo/servo/blob/e6b271d32981ad95442c029bf72bd35efc88f9c5/components/script_traits/lib.rs#L137-L164) for storing the srcdoc contents when loading a srcdoc iframe. - (partially) implemented a new `page_load_about_srcdoc` method to `script_thread.rs` which loads the special `about:srcdoc` URL [per the specification](https://html.spec.whatwg.org/multipage/iframe-embed-object.html#process-the-iframe-attributes) and takes the srcdoc contents as an argument - call this new method from [handle_new_layout](https://github.com/servo/servo/blob/e6b271d32981ad95442c029bf72bd35efc88f9c5/components/script/script_thread.rs#L2409-L2412) when it's detected that a srcdoc iframe is being loaded - (partially) in [attribute_mutated](https://github.com/servo/servo/blob/e6b271d32981ad95442c029bf72bd35efc88f9c5/components/script/dom/htmliframeelement.rs#L560), ensure that changing the `srcdoc` attribute of an iframe element [follows the specification](https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-iframe-element:the-iframe-element-9). ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes are a part of fix #4767 (GitHub issue number if applicable)

cagandhi · 2019-11-12T21:45:53Z

@jdm Is there some way we can help in solving the synchronous navigation issue?

jdm · 2019-11-12T21:55:29Z

@cagandhi The actual implementation work would be doable, but I would need to do some talking with more knowledgeable people first about what the expected model should be.

bors-servo · 2019-11-12T22:14:23Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing bd0ae59 to master...

cagandhi · 2019-11-13T03:46:12Z

@jdm On behalf of all my team members @jaymodi98 @amj23897, I would like to thank you for helping us immensely with this. This was our first open-source contribution and it feels really great now that the code is merged.

jdm · 2019-11-13T04:29:04Z

Thanks for taking the plunge with us!

highfive assigned ferjm Oct 29, 2019

highfive added the S-awaiting-review label Oct 29, 2019

highfive assigned jdm and unassigned ferjm Oct 29, 2019

jdm requested changes Oct 29, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 29, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 31, 2019

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 1, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 3, 2019

highfive added the S-tests-failed label Nov 4, 2019

jdm reviewed Nov 4, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 4, 2019

highfive added S-awaiting-review and removed S-tests-failed S-needs-code-changes labels Nov 4, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 12, 2019

jdm force-pushed the jaymodi98:iframe-srcdoc branch from 721280e to ddd9cd9 Nov 12, 2019

highfive added S-awaiting-review and removed S-tests-failed labels Nov 12, 2019

highfive added the S-tests-failed label Nov 12, 2019

jaymodi98 and others added 2 commits Oct 28, 2019

Implement srcdoc support for iframes.

bf69b6f

dom: Reduce the scope of iframe's load blocker borrow.

Loading status checks…

af158de

jdm force-pushed the jaymodi98:iframe-srcdoc branch from ddd9cd9 to af158de Nov 12, 2019

highfive removed the S-tests-failed label Nov 12, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 12, 2019

jdm mentioned this pull request Nov 12, 2019

srcdoc iframes navigate synchronously #24718

Closed

bors-servo merged commit af158de into servo:master Nov 12, 2019
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 12, 2019

jdm mentioned this pull request Nov 13, 2019

srcdoc referrer-policy tests are disabled #24731

Open

servo / servo

Implement srcdoc support for iframes #24576

Implement srcdoc support for iframes #24576

amj23897 commented Oct 29, 2019

highfive commented Oct 29, 2019

highfive commented Oct 29, 2019

highfive commented Oct 29, 2019

jdm commented Oct 29, 2019

jdm commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 4, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 12, 2019

jdm commented Nov 12, 2019

bors-servo commented Nov 12, 2019

bors-servo commented Nov 12, 2019

jdm commented Nov 12, 2019

bors-servo commented Nov 12, 2019

bors-servo commented Nov 12, 2019

cagandhi commented Nov 12, 2019

jdm commented Nov 12, 2019

bors-servo commented Nov 12, 2019

cagandhi commented Nov 13, 2019

jdm commented Nov 13, 2019 •

edited

servo / servo

Join GitHub today

Implement srcdoc support for iframes #24576

Implement srcdoc support for iframes #24576

Conversation

amj23897 commented Oct 29, 2019

highfive commented Oct 29, 2019

highfive commented Oct 29, 2019

highfive commented Oct 29, 2019

jdm commented Oct 29, 2019

jdm commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 4, 2019

This comment has been minimized.

jdm Nov 4, 2019

This comment has been minimized.

jdm Nov 4, 2019

This comment has been minimized.

amj23897 Nov 4, 2019

This comment has been minimized.

amj23897 Nov 4, 2019 • edited

jdm commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 12, 2019

jdm commented Nov 12, 2019

bors-servo commented Nov 12, 2019

bors-servo commented Nov 12, 2019

jdm commented Nov 12, 2019

bors-servo commented Nov 12, 2019

bors-servo commented Nov 12, 2019

cagandhi commented Nov 12, 2019

jdm commented Nov 12, 2019

bors-servo commented Nov 12, 2019

cagandhi commented Nov 13, 2019

jdm commented Nov 13, 2019 • edited

amj23897 Nov 4, 2019 •

edited

jdm commented Nov 13, 2019 •

edited