From cd58eaf8399a64d35ca2c3292841afb885184bed Mon Sep 17 00:00:00 2001 From: Mason Freed Date: Fri, 26 Aug 2022 17:01:10 +0000 Subject: [PATCH] Change the `popup` invalid value default to "manual" Per the resolution [1], invalid values should now be treated as if they were `popup=manual`. This lines up with the related change [2] to the UA stylesheet for pop-up. I also took the opportunity to expand the "basic" test to include adding the `popup` attribute on all (visible) element types. [1] https://github.com/openui/open-ui/issues/533#issuecomment-1227619710 [2] https://chromium-review.googlesource.com/c/chromium/src/+/3840811 Bug: 1307772 Change-Id: I6d5bf956e344f9256ab9cc1bf3d30655c8dee5bf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3858445 Commit-Queue: Mason Freed Reviewed-by: David Baron Auto-Submit: Mason Freed Cr-Commit-Position: refs/heads/main@{#1039838} NOKEYCHECK=True GitOrigin-RevId: 323e813eb8163ac256bbc9a36e097bf55f805f6c --- .../core/css/css_default_style_sheets.cc | 2 +- blink/renderer/core/css/selector_checker.cc | 4 +- blink/renderer/core/dom/document.cc | 2 +- blink/renderer/core/dom/element.cc | 77 ++++++++++--------- blink/renderer/core/dom/element.h | 2 +- blink/renderer/core/dom/element.idl | 2 +- ...popup_animation_finished_event_listener.cc | 2 +- blink/renderer/core/fullscreen/fullscreen.cc | 2 +- .../html/forms/html_form_control_element.cc | 2 +- .../html/forms/html_select_menu_element.cc | 6 +- .../popup-appearance-ref.tentative.html | 15 ++-- .../popups/popup-appearance.tentative.html | 24 +++--- .../popup-attribute-basic.tentative.html | 74 ++++++++++++------ 13 files changed, 118 insertions(+), 96 deletions(-) diff --git a/blink/renderer/core/css/css_default_style_sheets.cc b/blink/renderer/core/css/css_default_style_sheets.cc index 5a461bbd0fd..4ac35cbfbe6 100644 --- a/blink/renderer/core/css/css_default_style_sheets.cc +++ b/blink/renderer/core/css/css_default_style_sheets.cc @@ -323,7 +323,7 @@ bool CSSDefaultStyleSheets::EnsureDefaultStyleSheetsForElement( } } - if (!popup_style_sheet_ && element.HasValidPopupAttribute()) { + if (!popup_style_sheet_ && element.HasPopupAttribute()) { // TODO: We should assert that this sheet only contains rules for popups. DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled( element.GetDocument().GetExecutionContext())); diff --git a/blink/renderer/core/css/selector_checker.cc b/blink/renderer/core/css/selector_checker.cc index 44b322c3b99..a4074fa03f0 100644 --- a/blink/renderer/core/css/selector_checker.cc +++ b/blink/renderer/core/css/selector_checker.cc @@ -1500,7 +1500,7 @@ bool SelectorChecker::CheckPseudoClass(const SelectorCheckingContext& context, break; } case CSSSelector::kPseudoOpen: - if (element.HasValidPopupAttribute()) { + if (element.HasPopupAttribute()) { return element.popupOpen(); } return false; @@ -1509,7 +1509,7 @@ bool SelectorChecker::CheckPseudoClass(const SelectorCheckingContext& context, element.GetDocument().GetExecutionContext())) { return false; } - if (element.HasValidPopupAttribute()) { + if (element.HasPopupAttribute()) { return element.GetPopupData()->visibilityState() == PopupVisibilityState::kHidden; } diff --git a/blink/renderer/core/dom/document.cc b/blink/renderer/core/dom/document.cc index 082f3468a9e..e7724dbe961 100644 --- a/blink/renderer/core/dom/document.cc +++ b/blink/renderer/core/dom/document.cc @@ -7497,7 +7497,7 @@ Element* Document::TopmostPopupAutoOrHint() const { void Document::SetPopUpMousedownTarget(const Element* pop_up) { DCHECK( RuntimeEnabledFeatures::HTMLPopupAttributeEnabled(GetExecutionContext())); - DCHECK(!pop_up || pop_up->HasValidPopupAttribute()); + DCHECK(!pop_up || pop_up->HasPopupAttribute()); pop_up_mousedown_target_ = pop_up; } diff --git a/blink/renderer/core/dom/element.cc b/blink/renderer/core/dom/element.cc index 71458f79f40..cbdec22e538 100644 --- a/blink/renderer/core/dom/element.cc +++ b/blink/renderer/core/dom/element.cc @@ -2394,7 +2394,7 @@ void Element::AttributeChanged(const AttributeModificationParams& params) { } if (params.reason == AttributeModificationReason::kByParser && - name == html_names::kDefaultopenAttr && HasValidPopupAttribute()) { + name == html_names::kDefaultopenAttr && HasPopupAttribute()) { DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled( GetDocument().GetExecutionContext())); DCHECK(!isConnected()); @@ -2445,10 +2445,19 @@ void Element::UpdatePopupAttribute(String value) { type = PopupValueType::kHint; } else if (EqualIgnoringASCIICase(value, kPopupTypeValueManual)) { type = PopupValueType::kManual; - } else { - type = PopupValueType::kNone; + } else if (!value.IsNull()) { + // Invalid values default to popup=manual. + type = PopupValueType::kManual; + // TODO(masonf) This console message might be too much log spam. Though + // in case there's a namespace collision with something the developer is + // doing with e.g. a function called 'popup', this will be helpful to + // troubleshoot that. + GetDocument().AddConsoleMessage(MakeGarbageCollected( + mojom::blink::ConsoleMessageSource::kOther, + mojom::blink::ConsoleMessageLevel::kWarning, + "Found a 'popup' attribute with an invalid value.")); } - if (HasValidPopupAttribute()) { + if (HasPopupAttribute()) { if (PopupType() == type) return; // If the popup type is changing, hide it. @@ -2458,25 +2467,17 @@ void Element::UpdatePopupAttribute(String value) { } } if (type == PopupValueType::kNone) { - if (HasValidPopupAttribute()) { - // If the popup is changing from valid to invalid, remove the PopupData. + if (HasPopupAttribute()) { + // If the popup attribute is being removed, remove the PopupData. GetElementRareData()->RemovePopupData(); } - // TODO(masonf) This console message might be too much log spam. Though - // in case there's a namespace collision with something the developer is - // doing with e.g. a function called 'popup', this will be helpful to - // troubleshoot that. - GetDocument().AddConsoleMessage(MakeGarbageCollected( - mojom::blink::ConsoleMessageSource::kOther, - mojom::blink::ConsoleMessageLevel::kInfo, - "Found a 'popup' attribute with an invalid value.")); return; } UseCounter::Count(GetDocument(), WebFeature::kValidPopupAttribute); EnsureElementRareData().EnsurePopupData().setType(type); } -bool Element::HasValidPopupAttribute() const { +bool Element::HasPopupAttribute() const { return GetPopupData(); } @@ -2507,7 +2508,7 @@ bool Element::popupOpen() const { void Element::showPopUp(ExceptionState& exception_state) { DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled( GetDocument().GetExecutionContext())); - if (!HasValidPopupAttribute()) { + if (!HasPopupAttribute()) { return exception_state.ThrowDOMException( DOMExceptionCode::kNotSupportedError, "Not supported on elements that do not have a valid value for the " @@ -2526,7 +2527,7 @@ void Element::showPopUp(ExceptionState& exception_state) { // The 'show' event handler could have changed this pop-up, e.g. by changing // its type, removing it from the document, or calling showPopUp(). - if (!HasValidPopupAttribute() || !isConnected() || popupOpen()) + if (!HasPopupAttribute() || !isConnected() || popupOpen()) return; bool should_restore_focus = false; @@ -2560,7 +2561,7 @@ void Element::showPopUp(ExceptionState& exception_state) { // The 'hide' event handlers could have changed this popup, e.g. by changing // its type, removing it from the document, or calling showPopUp(). - if (!HasValidPopupAttribute() || !isConnected() || popupOpen()) + if (!HasPopupAttribute() || !isConnected() || popupOpen()) return; // We only restore focus for popup/hint, and only for the first popup in @@ -2598,7 +2599,7 @@ void Element::showPopUp(ExceptionState& exception_state) { // Only restore focus (later) if focus changed as a result of showing the // pop-up. - if (should_restore_focus && HasValidPopupAttribute() && + if (should_restore_focus && HasPopupAttribute() && originally_focused_element != document.FocusedElement()) { GetPopupData()->setPreviouslyFocusedElement(originally_focused_element); } @@ -2616,7 +2617,7 @@ void Element::HideAllPopupsUntil(const Element* endpoint, HidePopupIndependence popup_independence) { DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled( document.GetExecutionContext())); - DCHECK(!endpoint || endpoint->HasValidPopupAttribute()); + DCHECK(!endpoint || endpoint->HasPopupAttribute()); // If we're forcing a popup to hide immediately, first hide any other popups // that have already started the hide process. @@ -2670,7 +2671,7 @@ void Element::HideAllPopupsUntil(const Element* endpoint, void Element::hidePopUp(ExceptionState& exception_state) { DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled( GetDocument().GetExecutionContext())); - if (!HasValidPopupAttribute()) { + if (!HasPopupAttribute()) { return exception_state.ThrowDOMException( DOMExceptionCode::kNotSupportedError, "Not supported on elements that do not have a valid value for the " @@ -2706,7 +2707,7 @@ void Element::HidePopUpInternal(HidePopupFocusBehavior focus_behavior, HidePopupForcingLevel forcing_level) { DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled( GetDocument().GetExecutionContext())); - DCHECK(HasValidPopupAttribute()); + DCHECK(HasPopupAttribute()); auto& document = GetDocument(); if (PopupType() == PopupValueType::kAuto || PopupType() == PopupValueType::kHint) { @@ -2716,7 +2717,7 @@ void Element::HidePopUpInternal(HidePopupFocusBehavior focus_behavior, // The 'hide' event handlers could have changed this popup, e.g. by changing // its type, removing it from the document, or calling hidePopUp(). - if (!HasValidPopupAttribute() || !isConnected() || + if (!HasPopupAttribute() || !isConnected() || GetPopupData()->visibilityState() != PopupVisibilityState::kShowing) { DCHECK(!GetDocument().PopupStack().Contains(this)); return; @@ -2764,7 +2765,7 @@ void Element::HidePopUpInternal(HidePopupFocusBehavior focus_behavior, // The 'hide' event handler could have changed this popup, e.g. by changing // its type, removing it from the document, or calling showPopUp(). - if (!isConnected() || !HasValidPopupAttribute() || + if (!isConnected() || !HasPopupAttribute() || GetPopupData()->visibilityState() != PopupVisibilityState::kTransitioning) { return; @@ -2870,7 +2871,7 @@ Element* Element::GetPopupFocusableArea() const { auto* element = DynamicTo(node); if (!element) continue; - if (element->HasValidPopupAttribute() || IsA(*element)) { + if (element->HasPopupAttribute() || IsA(*element)) { next = FlatTreeTraversal::NextSkippingChildren(*element, this); continue; } @@ -2981,7 +2982,7 @@ const Element* Element::NearestOpenAncestralPopup(const Node& node, } auto* element = DynamicTo(node); bool new_element = - element && element->HasValidPopupAttribute() && !element->popupOpen(); + element && element->HasPopupAttribute() && !element->popupOpen(); if (new_element) { DCHECK(!inclusive); popup_positions.Set(element, indx++); @@ -3000,7 +3001,7 @@ const Element* Element::NearestOpenAncestralPopup(const Node& node, for (const Node* current_node = &node; current_node; current_node = FlatTreeTraversal::Parent(*current_node)) { if (auto* current_element = DynamicTo(current_node); - current_element && current_element->HasValidPopupAttribute() && + current_element && current_element->HasPopupAttribute() && current_element->popupOpen() && current_element->PopupType() != PopupValueType::kManual) { upper_bound = @@ -3076,7 +3077,7 @@ void Element::InvokePopup(Element* invoker) { DCHECK(invoker); DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled( GetDocument().GetExecutionContext())); - DCHECK(HasValidPopupAttribute()); + DCHECK(HasPopupAttribute()); GetPopupData()->setInvoker(invoker); showPopUp(ASSERT_NO_EXCEPTION); } @@ -3085,7 +3086,7 @@ Element* Element::anchorElement() const { if (!RuntimeEnabledFeatures::HTMLPopupAttributeEnabled( GetDocument().GetExecutionContext())) return nullptr; - if (!HasValidPopupAttribute()) + if (!HasPopupAttribute()) return nullptr; const AtomicString& anchor_id = FastGetAttribute(html_names::kAnchorAttr); if (anchor_id.IsNull()) @@ -3099,11 +3100,11 @@ void Element::SetNeedsRepositioningForSelectMenu(bool flag) { DCHECK(RuntimeEnabledFeatures::HTMLSelectMenuElementEnabled()); DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled( GetDocument().GetExecutionContext())); - DCHECK(HasValidPopupAttribute()); - auto& popup_data = EnsureElementRareData().EnsurePopupData(); - if (popup_data.needsRepositioningForSelectMenu() == flag) + DCHECK(HasPopupAttribute()); + auto* popup_data = GetPopupData(); + if (popup_data->needsRepositioningForSelectMenu() == flag) return; - popup_data.setNeedsRepositioningForSelectMenu(flag); + popup_data->setNeedsRepositioningForSelectMenu(flag); if (flag) { SetHasCustomStyleCallbacks(); SetNeedsStyleRecalc(kLocalStyleChange, @@ -3116,15 +3117,15 @@ void Element::SetOwnerSelectMenuElement(HTMLSelectMenuElement* element) { DCHECK(RuntimeEnabledFeatures::HTMLSelectMenuElementEnabled()); DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled( GetDocument().GetExecutionContext())); - DCHECK(HasValidPopupAttribute()); - EnsureElementRareData().EnsurePopupData().setOwnerSelectMenuElement(element); + DCHECK(HasPopupAttribute()); + GetPopupData()->setOwnerSelectMenuElement(element); } // TODO(crbug.com/1197720): The popup position should be provided by the new // anchored positioning scheme. void Element::AdjustPopupPositionForSelectMenu(ComputedStyle& style) { DCHECK(RuntimeEnabledFeatures::HTMLSelectMenuElementEnabled()); - DCHECK(HasValidPopupAttribute()); + DCHECK(HasPopupAttribute()); DCHECK(GetPopupData()->needsRepositioningForSelectMenu()); auto* owner_select = GetPopupData()->ownerSelectMenuElement(); DCHECK(owner_select); @@ -3492,7 +3493,7 @@ void Element::RemovedFrom(ContainerNode& insertion_point) { // If a popup is removed from the document, make sure it gets // removed from the popup element stack and the top layer. - if (was_in_document && HasValidPopupAttribute()) { + if (was_in_document && HasPopupAttribute()) { // We can't run focus event handlers while removing elements. HidePopUpInternal(HidePopupFocusBehavior::kNone, HidePopupForcingLevel::kHideImmediately); @@ -7903,7 +7904,7 @@ scoped_refptr Element::CustomStyleForLayoutObject( OriginalStyleForLayoutObject(style_recalc_context); // TODO(crbug.com/1197720): This logic is for positioning the selectmenu // popup. This should be replaced by the new anchored positioning scheme. - if (HasValidPopupAttribute() && + if (HasPopupAttribute() && GetPopupData()->needsRepositioningForSelectMenu()) { DCHECK(RuntimeEnabledFeatures::HTMLSelectMenuElementEnabled()); DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled( diff --git a/blink/renderer/core/dom/element.h b/blink/renderer/core/dom/element.h index 09eedcf9284..174f67434c2 100644 --- a/blink/renderer/core/dom/element.h +++ b/blink/renderer/core/dom/element.h @@ -588,7 +588,7 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable { // Popup API related functions. void UpdatePopupAttribute(String); - bool HasValidPopupAttribute() const; + bool HasPopupAttribute() const; PopupData* GetPopupData() const; PopupValueType PopupType() const; bool popupOpen() const; diff --git a/blink/renderer/core/dom/element.idl b/blink/renderer/core/dom/element.idl index f83f8278ce5..53ad021c994 100644 --- a/blink/renderer/core/dom/element.idl +++ b/blink/renderer/core/dom/element.idl @@ -144,7 +144,7 @@ dictionary CheckVisibilityOptions { // The Pop-up API [MeasureAs=ElementShowPopup,RuntimeEnabled=HTMLPopupAttribute,RaisesException] void showPopUp(); [MeasureAs=ElementHidePopup,RuntimeEnabled=HTMLPopupAttribute,RaisesException] void hidePopUp(); - [Unscopable,CEReactions,RuntimeEnabled=HTMLPopupAttribute,MeasureAs=AnyPopupAttribute,Reflect,ReflectOnly=("auto","hint","manual"),ReflectEmpty="auto",ReflectInvalid] attribute DOMString? popUp; + [Unscopable,CEReactions,RuntimeEnabled=HTMLPopupAttribute,MeasureAs=AnyPopupAttribute,Reflect,ReflectOnly=("auto","hint","manual"),ReflectEmpty="auto",ReflectInvalid="manual"] attribute DOMString? popUp; [CEReactions,RuntimeEnabled=HTMLPopupAttribute,Reflect] attribute boolean defaultOpen; [CEReactions,RuntimeEnabled=HTMLPopupAttribute,Reflect] attribute DOMString? popUpToggleTarget; [CEReactions,RuntimeEnabled=HTMLPopupAttribute,Reflect] attribute DOMString? popUpHideTarget; diff --git a/blink/renderer/core/dom/popup_animation_finished_event_listener.cc b/blink/renderer/core/dom/popup_animation_finished_event_listener.cc index 74e8bbbe51c..7c83a609ef9 100644 --- a/blink/renderer/core/dom/popup_animation_finished_event_listener.cc +++ b/blink/renderer/core/dom/popup_animation_finished_event_listener.cc @@ -17,7 +17,7 @@ PopupAnimationFinishedEventListener::PopupAnimationFinishedEventListener( Member popup_element, HeapHashSet>&& animations) : popup_element_(popup_element), animations_(std::move(animations)) { - DCHECK(popup_element->HasValidPopupAttribute()); + DCHECK(popup_element->HasPopupAttribute()); DCHECK(!animations_.IsEmpty()); for (auto animation : animations_) { animation->addEventListener(event_type_names::kFinish, this, diff --git a/blink/renderer/core/fullscreen/fullscreen.cc b/blink/renderer/core/fullscreen/fullscreen.cc index b7f75d4f292..5792bdaa774 100644 --- a/blink/renderer/core/fullscreen/fullscreen.cc +++ b/blink/renderer/core/fullscreen/fullscreen.cc @@ -404,7 +404,7 @@ bool RequestFullscreenConditionsMet(Element& pending, Document& document) { return false; // |pending| is not a dialog or popup element. - if (IsA(pending) || pending.HasValidPopupAttribute()) + if (IsA(pending) || pending.HasPopupAttribute()) return false; // The fullscreen element ready check for |pending| returns false. diff --git a/blink/renderer/core/html/forms/html_form_control_element.cc b/blink/renderer/core/html/forms/html_form_control_element.cc index bf6416ebf55..a88706e12d9 100644 --- a/blink/renderer/core/html/forms/html_form_control_element.cc +++ b/blink/renderer/core/html/forms/html_form_control_element.cc @@ -365,7 +365,7 @@ HTMLFormControlElement::popupTargetElement() const { if (idref.IsNull()) return no_element; Element* popup_element = GetTreeScope().getElementById(idref); - if (!popup_element || !popup_element->HasValidPopupAttribute()) + if (!popup_element || !popup_element->HasPopupAttribute()) return no_element; return PopupTargetElement{.element = popup_element, .action = action, diff --git a/blink/renderer/core/html/forms/html_select_menu_element.cc b/blink/renderer/core/html/forms/html_select_menu_element.cc index 12ae22bb3de..758c160f05f 100644 --- a/blink/renderer/core/html/forms/html_select_menu_element.cc +++ b/blink/renderer/core/html/forms/html_select_menu_element.cc @@ -338,7 +338,7 @@ bool HTMLSelectMenuElement::open() const { // either of the key parts (button or listbox) are missing. if (!listbox_part_) return false; - return listbox_part_->HasValidPopupAttribute() && listbox_part_->popupOpen(); + return listbox_part_->HasPopupAttribute() && listbox_part_->popupOpen(); } void HTMLSelectMenuElement::OpenListbox() { @@ -354,7 +354,7 @@ void HTMLSelectMenuElement::OpenListbox() { void HTMLSelectMenuElement::CloseListbox() { if (listbox_part_ && open()) { - if (listbox_part_->HasValidPopupAttribute()) { + if (listbox_part_->HasPopupAttribute()) { // We will handle focus directly. listbox_part_->HidePopUpInternal( HidePopupFocusBehavior::kNone, @@ -418,7 +418,7 @@ bool HTMLSelectMenuElement::IsValidListboxPart(const Node* node, return false; } - if (!element->HasValidPopupAttribute()) { + if (!element->HasPopupAttribute()) { if (show_warning) { GetDocument().AddConsoleMessage(MakeGarbageCollected( mojom::blink::ConsoleMessageSource::kRendering, diff --git a/blink/web_tests/external/wpt/html/semantics/popups/popup-appearance-ref.tentative.html b/blink/web_tests/external/wpt/html/semantics/popups/popup-appearance-ref.tentative.html index b8ebba794d2..43e3fd4ab7a 100644 --- a/blink/web_tests/external/wpt/html/semantics/popups/popup-appearance-ref.tentative.html +++ b/blink/web_tests/external/wpt/html/semantics/popups/popup-appearance-ref.tentative.html @@ -5,15 +5,16 @@ -

There should be four pop-ups with similar appearance, and - the word "Unknown" should not be visible on the page.

+

There should be five pop-ups with similar appearance.

Blank
Auto
-
Hint
+
Hint
Manual
+
Invalid
diff --git a/blink/web_tests/external/wpt/html/semantics/popups/popup-appearance.tentative.html b/blink/web_tests/external/wpt/html/semantics/popups/popup-appearance.tentative.html index 4acc276c16c..942734e203e 100644 --- a/blink/web_tests/external/wpt/html/semantics/popups/popup-appearance.tentative.html +++ b/blink/web_tests/external/wpt/html/semantics/popups/popup-appearance.tentative.html @@ -7,27 +7,21 @@ -

There should be four pop-ups with similar appearance, and - the word "Unknown" should not be visible on the page.

+

There should be five pop-ups with similar appearance.

Blank
Auto
Hint
Manual
- -
Unknown
+ +
Invalid
diff --git a/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html b/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html index 4554162c3cc..4f508ee5bae 100644 --- a/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html +++ b/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html @@ -5,6 +5,7 @@ +
Pop up
@@ -12,12 +13,17 @@
Pop up
Pop up
Pop up
+
Different element type
+
Different element type
+ + +
Invalid popup value - defaults to popup=manual
+
Invalid popup value - defaults to popup=manual
+
Invalid popup value - defaults to popup=manual
Not a pop-up
-
Not a pop-up
-
Not a pop-up
Animated pop-up
@@ -32,24 +38,25 @@