Skip to content

Commit

Permalink
HTML: BrowsingContext: Remove m_parent and fix is_ancestor_of
Browse files Browse the repository at this point in the history
`BrowsingContext::m_parent` has been removed from the spec,
and previously `m_parent` was always null.

`BrowsingContext::is_top_level` was already always returning
true before because of that, and the updated spec algorithm
causes assertions to fail.

This fixes the following example:
```html
<a href="about:blank" target="test">a
<iframe name="test">
```
clicking the link twice no longer causes it to open in a new tab.
  • Loading branch information
bbb651 authored and awesomekling committed Aug 20, 2024
1 parent f82e334 commit e6a668a
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 12 deletions.
4 changes: 3 additions & 1 deletion Userland/Libraries/LibWeb/DOM/Document.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3672,7 +3672,9 @@ void Document::make_active()
m_visibility_state = navigable()->traversable_navigable()->system_visibility_state();
}

// 4. Set window's relevant settings object's execution ready flag.
// TODO: 4. Queue a new VisibilityStateEntry whose visibility state is document's visibility state and whose timestamp is zero.

// 5. Set window's relevant settings object's execution ready flag.
HTML::relevant_settings_object(window).execution_ready = true;

if (m_needs_to_call_page_did_load) {
Expand Down
36 changes: 29 additions & 7 deletions Userland/Libraries/LibWeb/HTML/BrowsingContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,6 @@ void BrowsingContext::visit_edges(Cell::Visitor& visitor)
visitor.visit(m_page);
visitor.visit(m_window_proxy);
visitor.visit(m_group);
visitor.visit(m_parent);
visitor.visit(m_first_child);
visitor.visit(m_last_child);
visitor.visit(m_next_sibling);
Expand All @@ -321,8 +320,10 @@ JS::NonnullGCPtr<HTML::TraversableNavigable> BrowsingContext::top_level_traversa
// https://html.spec.whatwg.org/multipage/browsers.html#top-level-browsing-context
bool BrowsingContext::is_top_level() const
{
// A browsing context that has no parent browsing context is the top-level browsing context for itself and all of the browsing contexts for which it is an ancestor browsing context.
return !parent();
// FIXME: Remove this. The active document's navigable is sometimes null when it shouldn't be, failing assertions.
return true;
// A top-level browsing context is a browsing context whose active document's node navigable is a traversable navigable.
return active_document() != nullptr && active_document()->navigable() != nullptr && active_document()->navigable()->is_traversable();
}

JS::GCPtr<BrowsingContext> BrowsingContext::top_level_browsing_context() const
Expand Down Expand Up @@ -434,12 +435,28 @@ JS::GCPtr<BrowsingContext> BrowsingContext::next_sibling() const
return m_next_sibling;
}

bool BrowsingContext::is_ancestor_of(BrowsingContext const& other) const
// https://html.spec.whatwg.org/multipage/document-sequences.html#ancestor-browsing-context
bool BrowsingContext::is_ancestor_of(BrowsingContext const& potential_descendant) const
{
for (auto ancestor = other.parent(); ancestor; ancestor = ancestor->parent()) {
if (ancestor == this)
// A browsing context potentialDescendant is said to be an ancestor of a browsing context potentialAncestor if the following algorithm returns true:

// 1. Let potentialDescendantDocument be potentialDescendant's active document.
auto const* potential_descendant_document = potential_descendant.active_document();

// 2. If potentialDescendantDocument is not fully active, then return false.
if (!potential_descendant_document->is_fully_active())
return false;

// 3. Let ancestorBCs be the list obtained by taking the browsing context of the active document of each member of potentialDescendantDocument's ancestor navigables.
for (auto const& ancestor : potential_descendant_document->ancestor_navigables()) {
auto ancestor_browsing_context = ancestor->active_browsing_context();

// 4. If ancestorBCs contains potentialAncestor, then return true.
if (ancestor_browsing_context == this)
return true;
}

// 5. Return false.
return false;
}

Expand All @@ -464,7 +481,12 @@ bool BrowsingContext::is_familiar_with(BrowsingContext const& other) const

// 4. If there exists an ancestor browsing context of B whose active document has the same origin as the active document of A, then return true.
// NOTE: This includes the case where A is an ancestor browsing context of B.
for (auto ancestor = B.parent(); ancestor; ancestor = ancestor->parent()) {

// If B's active document is not fully active then it cannot have ancestor browsing context
if (!B.active_document()->is_fully_active())
return false;

for (auto const& ancestor : B.active_document()->ancestor_navigables()) {
if (ancestor->active_document()->origin().is_same_origin(A.active_document()->origin()))
return true;
}
Expand Down
4 changes: 2 additions & 2 deletions Userland/Libraries/LibWeb/HTML/BrowsingContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ class BrowsingContext final : public JS::Cell {

JS::NonnullGCPtr<HTML::TraversableNavigable> top_level_traversable() const;

JS::GCPtr<BrowsingContext> parent() const { return m_parent; }
JS::GCPtr<BrowsingContext> first_child() const;
JS::GCPtr<BrowsingContext> next_sibling() const;

Expand Down Expand Up @@ -112,6 +111,8 @@ class BrowsingContext final : public JS::Cell {
Page& page() { return m_page; }
Page const& page() const { return m_page; }

u64 virtual_browsing_context_group_id() const { return m_virtual_browsing_context_group_id; }

JS::GCPtr<BrowsingContext> top_level_browsing_context() const;

BrowsingContextGroup* group();
Expand Down Expand Up @@ -161,7 +162,6 @@ class BrowsingContext final : public JS::Cell {
// https://html.spec.whatwg.org/multipage/browsers.html#tlbc-group
JS::GCPtr<BrowsingContextGroup> m_group;

JS::GCPtr<BrowsingContext> m_parent;
JS::GCPtr<BrowsingContext> m_first_child;
JS::GCPtr<BrowsingContext> m_last_child;
JS::GCPtr<BrowsingContext> m_next_sibling;
Expand Down
6 changes: 4 additions & 2 deletions Userland/Libraries/LibWeb/MixedContent/AbstractOperations.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ Fetch::Infrastructure::RequestOrResponseBlocking should_fetching_request_be_bloc
|| false

// 4. request’s destination is "document", and request’s target browsing context has no parent browsing context.
|| (request.destination() == Fetch::Infrastructure::Request::Destination::Document && !(request.client()->target_browsing_context && request.client()->target_browsing_context->parent()))) {
// TODO: "parent browsing context" doesn't exist anymore and is a spec bug, seems like it should be `is_top_level`
|| (request.destination() == Fetch::Infrastructure::Request::Destination::Document && request.client()->target_browsing_context && request.client()->target_browsing_context->is_top_level())) {
return Fetch::Infrastructure::RequestOrResponseBlocking::Allowed;
}

Expand All @@ -104,7 +105,8 @@ Web::Fetch::Infrastructure::RequestOrResponseBlocking should_response_to_request
|| false

// 4. request’s destination is "document", and request’s target browsing context has no parent browsing context.
|| (request.destination() == Fetch::Infrastructure::Request::Destination::Document && !(request.client()->target_browsing_context && request.client()->target_browsing_context->parent()))) {
// TODO: "parent browsing context" doesn't exist anymore and is a spec bug, seems like it should be `is_top_level`
|| (request.destination() == Fetch::Infrastructure::Request::Destination::Document && request.client()->target_browsing_context && request.client()->target_browsing_context->is_top_level())) {
return Fetch::Infrastructure::RequestOrResponseBlocking::Allowed;
}

Expand Down

0 comments on commit e6a668a

Please sign in to comment.