Modify the UA stylesheet to allow <dialog popup>

This CL does two main things: 1. Convert the :-internal-popup-hidden to :-internal-popup-opening-or-open 2. Add/modify rules for both dialog and [popup] so that they don't add display:none when a <dialog popup> is either open as a dialog or pop-up. See this issue for details: openui/open-ui#520 Bug: 1307772 Change-Id: I751f6991a988a58032cebd52f289e5468140ce79 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3856639 Commit-Queue: Mason Freed <[email protected]> Auto-Submit: Mason Freed <[email protected]> Reviewed-by: David Baron <[email protected]> Cr-Commit-Position: refs/heads/main@{#1040576} NOKEYCHECK=True GitOrigin-RevId: 1f5127c0f61573d6fe4f7a3a2294d363f21bcade
mjfroman · Aug 29, 2022 · d8040fe · d8040fe
1 parent c8a6d46
commit d8040fe
Show file tree

Hide file tree

Showing 10 changed files with 25 additions and 19 deletions.
diff --git a/blink/renderer/core/css/css_selector.cc b/blink/renderer/core/css/css_selector.cc
@@ -329,7 +329,7 @@ PseudoId CSSSelector::GetPseudoId(PseudoType type) {
     case kPseudoPlaceholder:
     case kPseudoPlaceholderShown:
     case kPseudoPlaying:
-    case kPseudoPopupHidden:
+    case kPseudoPopupOpeningOrOpen:
     case kPseudoReadOnly:
     case kPseudoReadWrite:
     case kPseudoRelativeAnchor:
@@ -383,7 +383,7 @@ const static NameToPseudoStruct kPseudoTypeWithoutArgumentsMap[] = {
     {"-internal-media-controls-overlay-cast-button",
      CSSSelector::kPseudoWebKitCustomElement},
     {"-internal-multi-select-focus", CSSSelector::kPseudoMultiSelectFocus},
-    {"-internal-popup-hidden", CSSSelector::kPseudoPopupHidden},
+    {"-internal-popup-opening-or-open", CSSSelector::kPseudoPopupOpeningOrOpen},
     {"-internal-relative-anchor", CSSSelector::kPseudoRelativeAnchor},
     {"-internal-selector-fragment-anchor",
      CSSSelector::kPseudoSelectorFragmentAnchor},
@@ -576,7 +576,7 @@ CSSSelector::PseudoType CSSSelector::NameToPseudoType(
   if (match->type == CSSSelector::kPseudoOpen && !popup_attribute_enabled)
     return CSSSelector::kPseudoUnknown;
 
-  if (match->type == CSSSelector::kPseudoPopupHidden &&
+  if (match->type == CSSSelector::kPseudoPopupOpeningOrOpen &&
       !popup_attribute_enabled)
     return CSSSelector::kPseudoUnknown;
 
@@ -779,7 +779,7 @@ void CSSSelector::UpdatePseudoType(const AtomicString& value,
     case kPseudoPictureInPicture:
     case kPseudoPlaceholderShown:
     case kPseudoPlaying:
-    case kPseudoPopupHidden:
+    case kPseudoPopupOpeningOrOpen:
     case kPseudoReadOnly:
     case kPseudoReadWrite:
     case kPseudoRelativeAnchor:

diff --git a/blink/renderer/core/css/css_selector.h b/blink/renderer/core/css/css_selector.h
@@ -285,7 +285,7 @@ class CORE_EXPORT CSSSelector {
     kPseudoMultiSelectFocus,
     kPseudoHostHasAppearance,
     kPseudoOpen,
-    kPseudoPopupHidden,
+    kPseudoPopupOpeningOrOpen,
     kPseudoSlotted,
     kPseudoVideoPersistent,
     kPseudoVideoPersistentAncestor,

diff --git a/blink/renderer/core/css/parser/css_proto_converter.cc b/blink/renderer/core/css/parser/css_proto_converter.cc
@@ -43,7 +43,7 @@ const std::string Converter::kPseudoLookupTable[] = {
     "-internal-list-box",
     "-internal-media-controls-overlay-cast-button",
     "-internal-multi-select-focus",
-    "-internal-popup-hidden",
+    "-internal-popup-opening-or-open",
     "-internal-shadow-host-has-appearance",
     "-internal-spatial-navigation-focus",
     "-internal-video-persistent",

diff --git a/blink/renderer/core/css/popup.css b/blink/renderer/core/css/popup.css
@@ -9,8 +9,8 @@
 
 @namespace "http://www.w3.org/1999/xhtml";
 
-[popup]:-internal-popup-hidden {
-  display: none;
+[popup]:not(:-internal-popup-opening-or-open,dialog[open]) {
+  display:none;
 }
 
 [popup] {

diff --git a/blink/renderer/core/css/rule_feature_set.cc b/blink/renderer/core/css/rule_feature_set.cc
@@ -177,7 +177,7 @@ bool SupportsInvalidation(CSSSelector::PseudoType type) {
     case CSSSelector::kPseudoMultiSelectFocus:
     case CSSSelector::kPseudoHostHasAppearance:
     case CSSSelector::kPseudoOpen:
-    case CSSSelector::kPseudoPopupHidden:
+    case CSSSelector::kPseudoPopupOpeningOrOpen:
     case CSSSelector::kPseudoSlotted:
     case CSSSelector::kPseudoVideoPersistent:
     case CSSSelector::kPseudoVideoPersistentAncestor:
@@ -653,7 +653,7 @@ InvalidationSet* RuleFeatureSet::InvalidationSetForSimpleSelector(
       case CSSSelector::kPseudoOutOfRange:
       case CSSSelector::kPseudoDefined:
       case CSSSelector::kPseudoOpen:
-      case CSSSelector::kPseudoPopupHidden:
+      case CSSSelector::kPseudoPopupOpeningOrOpen:
       case CSSSelector::kPseudoVideoPersistent:
       case CSSSelector::kPseudoVideoPersistentAncestor:
       case CSSSelector::kPseudoXrOverlay:

diff --git a/blink/renderer/core/css/selector_checker.cc b/blink/renderer/core/css/selector_checker.cc
@@ -1504,18 +1504,19 @@ bool SelectorChecker::CheckPseudoClass(const SelectorCheckingContext& context,
         return element.popupOpen();
       }
       return false;
-    case CSSSelector::kPseudoPopupHidden:
+    case CSSSelector::kPseudoPopupOpeningOrOpen:
       if (!RuntimeEnabledFeatures::HTMLPopupAttributeEnabled(
               element.GetDocument().GetExecutionContext())) {
+        // The html.css UA stylesheet contains a rule for <dialog> elements
+        // that uses this pseudo, with `dialog:not(this_pseudo)`, so it's
+        // important to *not* match when the feature is *disabled*.
         return false;
       }
       if (element.HasPopupAttribute()) {
-        return element.GetPopupData()->visibilityState() ==
+        return element.GetPopupData()->visibilityState() !=
                PopupVisibilityState::kHidden;
       }
-      // Invalid values of the `popup` attribute should match the
-      // :-internal-popup-hidden pseudo selector.
-      return element.FastHasAttribute(html_names::kPopupAttr);
+      return false;
     case CSSSelector::kPseudoFullscreen:
     // fall through
     case CSSSelector::kPseudoFullScreen:

diff --git a/blink/renderer/core/dom/element.cc b/blink/renderer/core/dom/element.cc
@@ -2588,7 +2588,7 @@ void Element::showPopUp(ExceptionState& exception_state) {
   document.AddToTopLayer(this);
   // Remove display:none styling:
   GetPopupData()->setVisibilityState(PopupVisibilityState::kTransitioning);
-  PseudoStateChanged(CSSSelector::kPseudoPopupHidden);
+  PseudoStateChanged(CSSSelector::kPseudoPopupOpeningOrOpen);
 
   // Force a style update. This ensures that base property values are set prior
   // to `:open` matching, so that transitions can start on the change to
@@ -2816,7 +2816,7 @@ void Element::PopupHideFinishIfNeeded() {
   if (GetPopupData()) {
     GetPopupData()->setVisibilityState(PopupVisibilityState::kHidden);
     GetPopupData()->setAnimationFinishedListener(nullptr);
-    PseudoStateChanged(CSSSelector::kPseudoPopupHidden);
+    PseudoStateChanged(CSSSelector::kPseudoPopupOpeningOrOpen);
   }
 }
 

diff --git a/blink/renderer/core/html/resources/html.css b/blink/renderer/core/html/resources/html.css
@@ -1367,7 +1367,9 @@ textarea[dir=auto i] {
     unicode-bidi: plaintext;
 }
 
-dialog:not([open]) {
+/* The `,[popup]:-internal-popup-opening-or-open` is to support the pop-up API, and
+   has no effect if the HTMLPopupAttribute feature is disabled. */
+dialog:not([open],[popup]:-internal-popup-opening-or-open) {
     /* https://html.spec.whatwg.org/multipage/rendering.html#flow-content-3 */
     display: none;
 }

diff --git a/blink/renderer/core/inspector/inspector_trace_events.cc b/blink/renderer/core/inspector/inspector_trace_events.cc
@@ -382,7 +382,7 @@ const char* PseudoTypeToString(CSSSelector::PseudoType pseudo_type) {
     DEFINE_STRING_MAPPING(PseudoListBox)
     DEFINE_STRING_MAPPING(PseudoMultiSelectFocus)
     DEFINE_STRING_MAPPING(PseudoOpen)
-    DEFINE_STRING_MAPPING(PseudoPopupHidden)
+    DEFINE_STRING_MAPPING(PseudoPopupOpeningOrOpen)
     DEFINE_STRING_MAPPING(PseudoHostHasAppearance)
     DEFINE_STRING_MAPPING(PseudoVideoPersistent)
     DEFINE_STRING_MAPPING(PseudoVideoPersistentAncestor)

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
@@ -17,13 +17,16 @@
   <header popup>Different element type</header>
   <nav popup>Different element type</nav>
   <input type=text popup value="Different element type">
+  <dialog popup>Dialog with popup attribute</dialog>
+  <dialog popup="manual">Dialog with popup=manual</dialog>
   <div popup=true>Invalid popup value - defaults to popup=manual</div>
   <div popup=popup>Invalid popup value - defaults to popup=manual</div>
   <div popup=invalid>Invalid popup value - defaults to popup=manual</div>
 </div>
 
 <div id=nonpopups>
   <div>Not a pop-up</div>
+  <dialog open>Dialog without popup attribute</dialog>
 </div>
 
 <div popup class=animated>Animated pop-up</div>