Skip to content

LibWeb: Prevent a popover crash and implement popovertarget #3285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ADKaster merged 3 commits into from
Jan 30, 2025
Merged

LibWeb: Prevent a popover crash and implement popovertarget #3285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d57de72
LibWeb: Implement popovertarget buttons
Gingeh Jan 14, 2025
3d9dbf2
LibWeb: Ignore DOM state when hiding removed popovers
Gingeh Jan 14, 2025
c3a4ee6
LibWeb: Hide visible popover when type is changed
Gingeh Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions Libraries/LibWeb/DOM/Document.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5814,10 +5814,11 @@ void Document::add_an_element_to_the_top_layer(GC::Ref<Element> element)

// 3. Append el to doc’s top layer.
m_top_layer_elements.set(element);

element->set_in_top_layer(true);

// FIXME: 4. At the UA !important cascade origin, add a rule targeting el containing an overlay: auto declaration.
element->set_rendered_in_top_layer(true);
element->set_needs_style_update(true);
}

// https://drafts.csswg.org/css-position-4/#request-an-element-to-be-removed-from-the-top-layer
Expand All @@ -5830,9 +5831,12 @@ void Document::request_an_element_to_be_remove_from_the_top_layer(GC::Ref<Elemen
return;

// FIXME: 3. Remove the UA !important overlay: auto rule targeting el.
element->set_rendered_in_top_layer(false);
element->set_needs_style_update(true);

// 4. Append el to doc’s pending top layer removals.
m_top_layer_pending_removals.set(element);
element->set_in_top_layer(false);
}

// https://drafts.csswg.org/css-position-4/#remove-an-element-from-the-top-layer-immediately
Expand All @@ -5842,10 +5846,11 @@ void Document::remove_an_element_from_the_top_layer_immediately(GC::Ref<Element>

// 2. Remove el from doc’s top layer and pending top layer removals.
m_top_layer_elements.remove(element);

element->set_in_top_layer(false);

// FIXME: 3. Remove the UA !important overlay: auto rule targeting el, if it exists.
element->set_rendered_in_top_layer(false);
element->set_needs_style_update(true);
}

// https://drafts.csswg.org/css-position-4/#process-top-layer-removals
Expand All @@ -5854,11 +5859,10 @@ void Document::process_top_layer_removals()
// 1. For each element el in doc’s pending top layer removals: if el’s computed value of overlay is none, or el is
// not rendered, remove el from doc’s top layer and pending top layer removals.
for (auto& element : m_top_layer_pending_removals) {
// FIXME: Check overlay property
if (!element->paintable()) {
// FIXME: Implement overlay property
if (true || !element->paintable()) {
m_top_layer_elements.remove(element);
m_top_layer_pending_removals.remove(element);
element->set_in_top_layer(false);
}
}
}
Expand Down
8 changes: 8 additions & 0 deletions Libraries/LibWeb/DOM/Element.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,9 +388,16 @@ class Element
return nullptr;
}

// An element el is in the top layer if el is contained in its node document’s top layer
// but not contained in its node document’s pending top layer removals.
void set_in_top_layer(bool in_top_layer) { m_in_top_layer = in_top_layer; }
bool in_top_layer() const { return m_in_top_layer; }

// An element el is rendered in the top layer if el is contained in its node document’s top layer,
// FIXME: and el has overlay: auto.
void set_rendered_in_top_layer(bool rendered_in_top_layer) { m_rendered_in_top_layer = rendered_in_top_layer; }
bool rendered_in_top_layer() const { return m_rendered_in_top_layer; }

bool has_non_empty_counters_set() const { return m_counters_set; }
Optional<CSS::CountersSet const&> counters_set();
CSS::CountersSet& ensure_counters_set();
Expand Down Expand Up @@ -499,6 +506,7 @@ class Element
Array<CSSPixelPoint, 3> m_scroll_offset;

bool m_in_top_layer { false };
bool m_rendered_in_top_layer { false };
bool m_style_uses_css_custom_properties { false };

OwnPtr<CSS::CountersSet> m_counters_set;
Expand Down
5 changes: 4 additions & 1 deletion Libraries/LibWeb/HTML/HTMLButtonElement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <LibWeb/Bindings/HTMLButtonElementPrototype.h>
#include <LibWeb/DOM/Document.h>
#include <LibWeb/DOM/Event.h>
#include <LibWeb/HTML/HTMLButtonElement.h>
#include <LibWeb/HTML/HTMLFormElement.h>

Expand Down Expand Up @@ -115,7 +116,9 @@ void HTMLButtonElement::activation_behavior(DOM::Event const& event)
}
}

// 4. FIXME: Run the popover target attribute activation behavior given element.
// 4. Run the popover target attribute activation behavior given element and event's target.
if (event.target() && event.target()->is_dom_node())
PopoverInvokerElement::popover_target_activation_behaviour(*this, as<DOM::Node>(*event.target()));
}

bool HTMLButtonElement::is_focusable() const
Expand Down
99 changes: 67 additions & 32 deletions Libraries/LibWeb/HTML/HTMLElement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,27 @@ void HTMLElement::attribute_changed(FlyString const& name, Optional<String> cons
}
ENUMERATE_GLOBAL_EVENT_HANDLERS(__ENUMERATE)
#undef __ENUMERATE

[&]() {
// https://html.spec.whatwg.org/multipage/popover.html#the-popover-attribute:concept-element-attributes-change-ext
// https://whatpr.org/html/9457/popover.html#the-popover-attribute:concept-element-attributes-change-ext
// The following attribute change steps, given element, localName, oldValue, value, and namespace, are used for all HTML elements:

// 1. If namespace is not null, then return.
if (namespace_.has_value())
return;

// 2. If localName is not popover, then return.
if (name != HTML::AttributeNames::popover)
return;

// 3. If element's popover visibility state is in the showing state
// and oldValue and value are in different states,
// then run the hide popover algorithm given element, true, true, false, and true.
if (m_popover_visibility_state == PopoverVisibilityState::Showing
&& popover_value_to_state(old_value) != popover_value_to_state(value))
MUST(hide_popover(FocusPreviousElement::Yes, FireEvents::Yes, ThrowExceptions::No, IgnoreDomState::Yes));
}();
}

WebIDL::ExceptionOr<void> HTMLElement::cloned(Web::DOM::Node& copy, bool clone_children) const
Expand Down Expand Up @@ -957,13 +978,8 @@ WebIDL::ExceptionOr<GC::Ref<ElementInternals>> HTMLElement::attach_internals()
return { internals };
}

// https://html.spec.whatwg.org/multipage/popover.html#dom-popover
Optional<String> HTMLElement::popover() const
Optional<String> HTMLElement::popover_value_to_state(Optional<String> value)
{
// FIXME: This should probably be `Reflect` in the IDL.
// The popover IDL attribute must reflect the popover attribute, limited to only known values.
auto value = get_attribute(HTML::AttributeNames::popover);

if (!value.has_value())
return {};

Expand All @@ -975,6 +991,15 @@ Optional<String> HTMLElement::popover() const
return "manual"_string;
}

// https://html.spec.whatwg.org/multipage/popover.html#dom-popover
Optional<String> HTMLElement::popover() const
{
// FIXME: This should probably be `Reflect` in the IDL.
// The popover IDL attribute must reflect the popover attribute, limited to only known values.
auto value = get_attribute(HTML::AttributeNames::popover);
return popover_value_to_state(value);
}

// https://html.spec.whatwg.org/multipage/popover.html#dom-popover
WebIDL::ExceptionOr<void> HTMLElement::set_popover(Optional<String> value)
{
Expand All @@ -997,10 +1022,11 @@ void HTMLElement::adjust_computed_style(CSS::ComputedProperties& style)
}

// https://html.spec.whatwg.org/multipage/popover.html#check-popover-validity
WebIDL::ExceptionOr<bool> HTMLElement::check_popover_validity(ExpectedToBeShowing expected_to_be_showing, ThrowExceptions throw_exceptions, GC::Ptr<DOM::Document> expected_document)
// https://whatpr.org/html/9457/popover.html#check-popover-validity
WebIDL::ExceptionOr<bool> HTMLElement::check_popover_validity(ExpectedToBeShowing expected_to_be_showing, ThrowExceptions throw_exceptions, GC::Ptr<DOM::Document> expected_document, IgnoreDomState ignore_dom_state)
Comment thread
Gingeh marked this conversation as resolved.
{
// 1. If element's popover attribute is in the no popover state, then:
if (!popover().has_value()) {
// 1. If ignoreDomState is false and element's popover attribute is in the no popover state, then:
if (ignore_dom_state == IgnoreDomState::No && !popover().has_value()) {
// 1.1. If throwExceptions is true, then throw a "NotSupportedError" DOMException.
if (throw_exceptions == ThrowExceptions::Yes)
return WebIDL::NotSupportedError::create(realm(), "Element is not a popover"_string);
Expand All @@ -1017,15 +1043,19 @@ WebIDL::ExceptionOr<bool> HTMLElement::check_popover_validity(ExpectedToBeShowin
}

// 3. If any of the following are true:
// - element is not connected;
// - ignoreDomState is false and element is not connected;
// - element's node document is not fully active;
// - expectedDocument is not null and element's node document is not expectedDocument;
// - ignoreDomState is false and expectedDocument is not null and element's node document is not expectedDocument;
// - element is a dialog element and its is modal flage is set to true; or
// - FIXME: element's fullscreen flag is set,
// then:
// 3.1 If throwExceptions is true, then throw an "InvalidStateError" DOMException.
// 3.2 Return false.
if (!is_connected() || !document().is_fully_active() || (expected_document && &document() != expected_document) || (is<HTMLDialogElement>(*this) && as<HTMLDialogElement>(*this).is_modal())) {

if ((ignore_dom_state == IgnoreDomState::No && !is_connected())
|| !document().is_fully_active()
|| (ignore_dom_state == IgnoreDomState::No && expected_document && &document() != expected_document)
|| (is<HTMLDialogElement>(*this) && as<HTMLDialogElement>(*this).is_modal())) {
if (throw_exceptions == ThrowExceptions::Yes)
return WebIDL::InvalidStateError::create(realm(), "Element is not in a valid state to show a popover"_string);
return false;
Expand All @@ -1045,10 +1075,11 @@ WebIDL::ExceptionOr<void> HTMLElement::show_popover_for_bindings(ShowPopoverOpti
}

// https://html.spec.whatwg.org/multipage/popover.html#show-popover
// https://whatpr.org/html/9457/popover.html#show-popover
WebIDL::ExceptionOr<void> HTMLElement::show_popover(ThrowExceptions throw_exceptions, GC::Ptr<HTMLElement> invoker)
{
// 1. If the result of running check popover validity given element, false, throwExceptions, and null is false, then return.
if (!TRY(check_popover_validity(ExpectedToBeShowing::No, throw_exceptions, nullptr)))
// 1. If the result of running check popover validity given element, false, throwExceptions, null and false is false, then return.
if (!TRY(check_popover_validity(ExpectedToBeShowing::No, throw_exceptions, nullptr, IgnoreDomState::No)))
return {};

// 2. Let document be element's node document.
Expand Down Expand Up @@ -1085,8 +1116,8 @@ WebIDL::ExceptionOr<void> HTMLElement::show_popover(ThrowExceptions throw_except
return {};
}

// 10. If the result of running check popover validity given element, false, throwExceptions, and document is false, then run cleanupShowingFlag and return.
if (!TRY(check_popover_validity(ExpectedToBeShowing::No, throw_exceptions, nullptr))) {
// 10. If the result of running check popover validity given element, false, throwExceptions, document, and false is false, then run cleanupShowingFlag and return.
if (!TRY(check_popover_validity(ExpectedToBeShowing::No, throw_exceptions, document, IgnoreDomState::No))) {
cleanup_showing_flag();
return {};
}
Expand Down Expand Up @@ -1123,7 +1154,7 @@ WebIDL::ExceptionOr<void> HTMLElement::show_popover(ThrowExceptions throw_except
// FIXME: 18.2. If originalType is not equal to the value of element's popover attribute, then:
// FIXME: 18.2.1. If throwExceptions is true, then throw a "InvalidStateError" DOMException.
// FIXME: 18.2.2. Return.
// FIXME: 18.3. If the result of running check popover validity given element, false, throwExceptions, and document is false, then run cleanupShowingFlag and return.
// FIXME: 18.3. If the result of running check popover validity given element, false, throwExceptions, document, and false is false, then run cleanupShowingFlag and return.
// FIXME: 18.4. If the result of running topmost auto or hint popover on document is null, then set shouldRestoreFocus to true.
// FIXME: 18.5. If stackToAppendTo is "auto":
// FIXME: 18.5.1. Assert: document's showing auto popover list does not contain element.
Expand All @@ -1139,7 +1170,7 @@ WebIDL::ExceptionOr<void> HTMLElement::show_popover(ThrowExceptions throw_except
// - closeAction being to hide a popover given element, true, true, and false.
auto close_callback_function = JS::NativeFunction::create(
realm(), [this](JS::VM&) {
MUST(hide_popover(FocusPreviousElement::Yes, FireEvents::Yes, ThrowExceptions::No));
MUST(hide_popover(FocusPreviousElement::Yes, FireEvents::Yes, ThrowExceptions::No, IgnoreDomState::No));

return JS::js_undefined();
},
Expand Down Expand Up @@ -1168,17 +1199,19 @@ WebIDL::ExceptionOr<void> HTMLElement::show_popover(ThrowExceptions throw_except
}

// https://html.spec.whatwg.org/multipage/popover.html#dom-hidepopover
// https://whatpr.org/html/9457/popover.html#dom-hidepopover
WebIDL::ExceptionOr<void> HTMLElement::hide_popover_for_bindings()
{
// The hidePopover() method steps are to run the hide popover algorithm given this, true, true, and true.
return hide_popover(FocusPreviousElement::Yes, FireEvents::Yes, ThrowExceptions::Yes);
// The hidePopover() method steps are to run the hide popover algorithm given this, true, true, true, and false.
return hide_popover(FocusPreviousElement::Yes, FireEvents::Yes, ThrowExceptions::Yes, IgnoreDomState::No);
}

// https://html.spec.whatwg.org/multipage/popover.html#hide-popover-algorithm
WebIDL::ExceptionOr<void> HTMLElement::hide_popover(FocusPreviousElement, FireEvents fire_events, ThrowExceptions throw_exceptions)
// https://whatpr.org/html/9457/popover.html#hide-popover-algorithm
WebIDL::ExceptionOr<void> HTMLElement::hide_popover(FocusPreviousElement, FireEvents fire_events, ThrowExceptions throw_exceptions, IgnoreDomState ignore_dom_state)
{
// 1. If the result of running check popover validity given element, true, throwExceptions, and null is false, then return.
if (!TRY(check_popover_validity(ExpectedToBeShowing::Yes, throw_exceptions, nullptr)))
// 1. If the result of running check popover validity given element, true, throwExceptions, null and ignoreDomState is false, then return.
if (!TRY(check_popover_validity(ExpectedToBeShowing::Yes, throw_exceptions, nullptr, ignore_dom_state)))
return {};

// 2. Let document be element's node document.
Expand Down Expand Up @@ -1211,7 +1244,7 @@ WebIDL::ExceptionOr<void> HTMLElement::hide_popover(FocusPreviousElement, FireEv
// 7. If element's popover attribute is in the auto state FIXME: or the hint state, then:
if (popover().has_value() && popover().value() == "auto"sv) {
// FIXME: 7.1. Run hide all popovers until given element, focusPreviousElement, and fireEvents.
// FIXME: 7.2. If the result of running check popover validity given element, true, and throwExceptions is false, then run cleanupSteps and return.
// FIXME: 7.2. If the result of running check popover validity given element, true, throwExceptions, and ignoreDomState is false, then run cleanupSteps and return.
}
// FIXME: 8. Let autoPopoverListContainsElement be true if document's showing auto popover list's last item is element, otherwise false.

Expand All @@ -1228,8 +1261,8 @@ WebIDL::ExceptionOr<void> HTMLElement::hide_popover(FocusPreviousElement, FireEv

// FIXME: 10.2. If autoPopoverListContainsElement is true and document's showing auto popover list's last item is not element, then run hide all popovers until given element, focusPreviousElement, and false.

// 10.3. If the result of running check popover validity given element, true, throwExceptions, and null is false, then run cleanupSteps and return.
if (!TRY(check_popover_validity(ExpectedToBeShowing::Yes, throw_exceptions, nullptr))) {
// 10.3. If the result of running check popover validity given element, true, throwExceptions, null, and ignoreDomState is false, then run cleanupSteps and return.
if (!TRY(check_popover_validity(ExpectedToBeShowing::Yes, throw_exceptions, nullptr, ignore_dom_state))) {
cleanup_steps();
return {};
}
Expand Down Expand Up @@ -1262,6 +1295,7 @@ WebIDL::ExceptionOr<void> HTMLElement::hide_popover(FocusPreviousElement, FireEv
}

// https://html.spec.whatwg.org/multipage/popover.html#dom-togglepopover
// https://whatpr.org/html/9457/popover.html#dom-togglepopover
WebIDL::ExceptionOr<bool> HTMLElement::toggle_popover(TogglePopoverOptionsOrForceBoolean const& options)
{
// 1. Let force be null.
Expand All @@ -1280,18 +1314,18 @@ WebIDL::ExceptionOr<bool> HTMLElement::toggle_popover(TogglePopoverOptionsOrForc
invoker = options.source;
});

// 5. If this's popover visibility state is showing, and force is null or false, then run the hide popover algorithm given this, true, true, and true.
// 5. If this's popover visibility state is showing, and force is null or false, then run the hide popover algorithm given this, true, true, true, and false.
if (popover_visibility_state() == PopoverVisibilityState::Showing && (!force.has_value() || !force.value()))
TRY(hide_popover(FocusPreviousElement::Yes, FireEvents::Yes, ThrowExceptions::Yes));
TRY(hide_popover(FocusPreviousElement::Yes, FireEvents::Yes, ThrowExceptions::Yes, IgnoreDomState::No));
// 6. Otherwise, if force is not present or true, then run show popover given this true, and invoker.
else if (!force.has_value() || force.value())
TRY(show_popover(ThrowExceptions::Yes, invoker));
// 7. Otherwise:
else {
// 7.1 Let expectedToBeShowing be true if this's popover visibility state is showing; otherwise false.
ExpectedToBeShowing expected_to_be_showing = popover_visibility_state() == PopoverVisibilityState::Showing ? ExpectedToBeShowing::Yes : ExpectedToBeShowing::No;
// 7.2 Run check popover validity given expectedToBeShowing, true, and null.
TRY(check_popover_validity(expected_to_be_showing, ThrowExceptions::Yes, nullptr));
// 7.2 Run check popover validity given expectedToBeShowing, true, null, and false.
TRY(check_popover_validity(expected_to_be_showing, ThrowExceptions::Yes, nullptr, IgnoreDomState::No));
}
// 8. Return true if this's popover visibility state is showing; otherwise false.
return popover_visibility_state() == PopoverVisibilityState::Showing;
Expand Down Expand Up @@ -1368,9 +1402,10 @@ void HTMLElement::removed_from(Node* old_parent, Node& old_root)
{
Element::removed_from(old_parent, old_root);

// If removedNode's popover attribute is not in the no popover state, then run the hide popover algorithm given removedNode, false, false, and false.
// https://whatpr.org/html/9457/infrastructure.html#dom-trees:concept-node-remove-ext
// If removedNode's popover attribute is not in the no popover state, then run the hide popover algorithm given removedNode, false, false, false, and true.
if (popover().has_value())
MUST(hide_popover(FocusPreviousElement::No, FireEvents::No, ThrowExceptions::No));
MUST(hide_popover(FocusPreviousElement::No, FireEvents::No, ThrowExceptions::No, IgnoreDomState::Yes));
}

// https://html.spec.whatwg.org/multipage/interaction.html#dom-accesskeylabel
Expand Down
Loading