Skip to content

LibWeb: Pass a null sourceDocument to Browser UI initiated navigations#4515

Closed
ADKaster wants to merge 3 commits intoLadybirdBrowser:masterfrom
ADKaster:html-pr-11250
Closed

LibWeb: Pass a null sourceDocument to Browser UI initiated navigations#4515
ADKaster wants to merge 3 commits intoLadybirdBrowser:masterfrom
ADKaster:html-pr-11250

Conversation

@ADKaster
Copy link
Copy Markdown
Member

Buildup and implementation for whatwg/html#11250

This does some .. interesting things to the m_current_entry_index of window.navigation. But as far as I could tell, it doesn't actually regress our UI-initiated fwd/back buttons, so I guess we'll have to untangle that later :)

Upcoming HTML spec changes require us to create these without a document
Upcoming HTML changes will actually pass parameters requiring an empty
TaskDestination to fetch from navigate(), so we need to be able to
queue fetch tasks using the implicit parallel queue TaskDestination.

Of course, we still have a FIXME for the parallel part, and just queue
a task with the implicit document now instead..
|| (request.url().host().has_value() && (request.url().host()->has<URL::IPv4Address>() || request.url().host()->has<URL::IPv6Address>()))

// 3. §4.3 Does settings prohibit mixed security contexts? returns "Does Not Restrict Mixed Security Contents" when applied to request’s client.
// 3. § 4.3 Does settings prohibit mixed security contexts? returns "Does Not Restrict Mixed Security Contents" when applied to request’s client.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently this spec has some non-breaking spaces in it when you copy from it, caught my eye in the IDE.

@@ -11,6 +11,15 @@

println(`transition is null: ${n.transition == null}`);
println(`canGoBack: ${n.canGoBack}`);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea why our test runner causes this to become false due to this change, but I can't say I think it should have been true to begin with.

Copy link
Copy Markdown
Contributor

@trflynn89 trflynn89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a full review, just a couple things I noticed while waiting for dinner to cook

request->set_policy_container(source_snapshot_params.source_policy_container);

// 4. If documentResource is a POST resource, then:
// 4. If request's client is null, then:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra space here, and a couple more below you can find by searching for .

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

artifact from copying steps from the whatpr url it seems. I'll do a scan.

// AD-HOC: If we are not able to continue in this process, request a new process from the UI.
if (is_top_level_traversable() && !page_client.is_url_suitable_for_same_process_navigation(active_document.url(), params.url)) {
page_client.request_new_process_for_navigation(params.url);
if (!page_client.is_url_suitable_for_same_process_navigation(active_document.url(), url)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge conflict? We don't want to lose the is_top_level_traversable check. See 00aa620.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops yep, I had to do some gymnastics to rebase this, given your change and Shannon's change to make two of the types affected no longer take a realm when allocating

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that came up when I was looking at that static function on Document function as well 😅 I also have this commit sitting around if helpful by the way: shannonbooth@c09aa7a

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At yeah that's the fetch spec algo that Domenic was updating due to my finding about the "request's window" needing updated for this.

Copy link
Copy Markdown
Member

@shannonbooth shannonbooth Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right just a catchup commit to make the actual spec changes easier to see/apply in that fetch MR

@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@github-actions github-actions Bot added the stale label May 20, 2025
@github-actions
Copy link
Copy Markdown

This pull request has been closed because it has not had recent activity. Feel free to open a new pull request if you wish to still contribute these changes. Thank you for your contributions!

@github-actions github-actions Bot closed this May 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants