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] openui/open-ui#533 (comment)
[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 <[email protected]>
Reviewed-by: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1039838}
NOKEYCHECK=True
GitOrigin-RevId: 323e813eb8163ac256bbc9a36e097bf55f805f6c

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()));

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;
       }

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;
 }
 

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<ConsoleMessage>(
+        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<ConsoleMessage>(
-        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<Element>(node);
     if (!element)
       continue;
-    if (element->HasValidPopupAttribute() || IsA<HTMLDialogElement>(*element)) {
+    if (element->HasPopupAttribute() || IsA<HTMLDialogElement>(*element)) {
       next = FlatTreeTraversal::NextSkippingChildren(*element, this);
       continue;
     }
@@ -2981,7 +2982,7 @@ const Element* Element::NearestOpenAncestralPopup(const Node& node,
   }
   auto* element = DynamicTo<Element>(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<Element>(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<ComputedStyle> 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(

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;

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;

blink/renderer/core/dom/popup_animation_finished_event_listener.cc

@@ -17,7 +17,7 @@ PopupAnimationFinishedEventListener::PopupAnimationFinishedEventListener(
     Member<Element> popup_element,
     HeapHashSet<Member<EventTarget>>&& 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,

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<HTMLDialogElement>(pending) || pending.HasValidPopupAttribute())
+  if (IsA<HTMLDialogElement>(pending) || pending.HasPopupAttribute())
     return false;
 
   // The fullscreen element ready check for |pending| returns false.

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,

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<ConsoleMessage>(
           mojom::blink::ConsoleMessageSource::kRendering,

blink/web_tests/external/wpt/html/semantics/popups/popup-appearance-ref.tentative.html

@@ -5,15 +5,16 @@
 
 <style>
 .fake-pop-up {top: 100px; bottom: auto;}
-#blank {left: -300px;}
-#auto {left: -100px;}
-#hint {left: 100px;}
-#manual {left: 300px;}
+#blank {left: -400px;}
+#auto {left: -200px;}
+#hint {left: 0;}
+#manual {left: 200px;}
+#invalid {left: 400px;}
 </style>
 
-<p>There should be four pop-ups with similar appearance, and
-   the word "Unknown" should not be visible on the page.</p>
+<p>There should be five pop-ups with similar appearance.</p>
 <div class="fake-pop-up" id=blank>Blank</div>
 <div class="fake-pop-up" id=auto>Auto</div>
-<div class="fake-pop-up" id=hint><span>Hint</span></div>
+<div class="fake-pop-up" id=hint>Hint</div>
 <div class="fake-pop-up" id=manual>Manual</div>
+<div class="fake-pop-up" id=invalid>Invalid</div>