Skip to content

Commit

Permalink
Simplify pop-up CSS rules, and add !important to display:none
Browse files Browse the repository at this point in the history
Per the resolution and discussion at [1], we've decided to simplify
the UA rules for pop-up attribute selectors, to capture *any*
`popup` attribute value. Additionally (and this part is tentative),
it looks like we'll likely resolve to put back `!important` on
display:none, so that `[popup] {display:flex}` can work.

[1] openui/open-ui#561 (comment)

Bug: 1307772
Change-Id: If3d52cce0c9cbd3c6134ff57836d65c93eb12c48
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3840811
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Reviewed-by: David Baron <dbaron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1037921}
NOKEYCHECK=True
GitOrigin-RevId: 3f17bf65ec57702a1ec74c58d03a8dd09f08f68b
  • Loading branch information
mfreed7 authored and copybara-github committed Aug 22, 2022
1 parent 7bf8d3b commit 011f6f1
Show file tree
Hide file tree
Showing 8 changed files with 28 additions and 45 deletions.
25 changes: 8 additions & 17 deletions blink/renderer/core/css/popup.css
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,10 @@
@namespace "http://www.w3.org/1999/xhtml";

[popup]:-internal-popup-hidden {
display: none;
display: none !important;
}

[popup="" i],
[popup=auto i],
[popup=hint i],
[popup=manual i] {
[popup] {
position: fixed;
inset-inline-start: 0;
inset-inline-end: 0;
Expand All @@ -27,20 +24,14 @@
margin: auto;
border: solid;
overflow: auto;
padding: 1em;
color: CanvasText;
background-color: Canvas;
}

[popup="" i]::backdrop,
[popup=auto i]::backdrop,
[popup=hint i]::backdrop,
[popup=manual i]::backdrop {
position: fixed;
inset-inline-start: 0;
inset-inline-end: 0;
inset-block-start: 0;
inset-block-end: 0;
background: transparent;
pointer-events: none !important;
[popup]::backdrop {
/* From the fullscreen spec: https://fullscreen.spec.whatwg.org/#user-agent-level-style-sheet-defaults: */
position: fixed;
top:0; right:0; bottom:0; left:0;
/* Specific to [popup]: */
pointer-events: none !important;
}
8 changes: 7 additions & 1 deletion blink/renderer/core/css/selector_checker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1504,11 +1504,17 @@ bool SelectorChecker::CheckPseudoClass(const SelectorCheckingContext& context,
}
return false;
case CSSSelector::kPseudoPopupHidden:
if (!RuntimeEnabledFeatures::HTMLPopupAttributeEnabled(
element.GetDocument().GetExecutionContext())) {
return false;
}
if (element.HasValidPopupAttribute()) {
return element.GetPopupData()->visibilityState() ==
PopupVisibilityState::kHidden;
}
return false;
// Invalid values of the `popup` attribute should match the
// :-internal-popup-hidden pseudo selector.
return element.FastHasAttribute(html_names::kPopupAttr);
case CSSSelector::kPseudoFullscreen:
// fall through
case CSSSelector::kPseudoFullScreen:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@
</style>

<p>There should be four pop-ups with similar appearance, and
the word Unknown with no special styling.</p>
the word "Unknown" should not be visible on the page.</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=manual>Manual</div>
<div class="not-a-pop-up!" id=unknown>Unknown</div>
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@
</style>

<p>There should be four pop-ups with similar appearance, and
the word Unknown with no special styling.</p>
the word "Unknown" should not be visible on the page.</p>
<div popup>Blank
<div popup=auto>Auto</div>
</div>
<div popup=hint>Hint</div>
<div popup=manual>Manual</div>
<!-- This ensures unsupported popup values don't receive styling -->
<!-- This ensures unsupported popup values are hidden -->
<div popup=unknown>Unknown</div>
<script>
document.querySelectorAll('[popup]').forEach(p => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,14 @@
parent.appendChild(popUp);
}
function assertNotAPopUp(nonPopUp) {
// Non-popup elements should already be visible.
assert_true(popUpVisible(nonPopUp, /*isPopUp*/false));
// If the non-pop-up element nonetheless has a 'popup' attribute, it should
// be invisible. Otherwise, it should be visible.
const expectVisible = !nonPopUp.hasAttribute('popup');
assert_equals(popUpVisible(nonPopUp, /*isPopUp*/false),expectVisible);
assert_throws_dom("NotSupportedError",() => nonPopUp.showPopUp(),'Calling showPopUp on a non-pop-up should throw NotSupportedError');
assert_true(popUpVisible(nonPopUp, /*isPopUp*/false));
assert_equals(popUpVisible(nonPopUp, /*isPopUp*/false),expectVisible);
assert_throws_dom("NotSupportedError",() => nonPopUp.hidePopUp(),'Calling hidePopUp on a non-pop-up should throw NotSupportedError');
assert_true(popUpVisible(nonPopUp, /*isPopUp*/false));
assert_equals(popUpVisible(nonPopUp, /*isPopUp*/false),expectVisible);
}

Array.from(document.getElementById('popups').children).forEach(popUp => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,3 @@
<meta charset="utf-8">
<link rel=author href="mailto:masonf@chromium.org">
<link rel="stylesheet" href="resources/popup-styles.css">

<div class=fake-pop-up>This content should be visible</div>

<style>
.fake-pop-up {
top: 0;
left: 0;
width: 300px;
height: 200px;
}
</style>
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
<link rel=help href="https://open-ui.org/components/popup.research.explainer">
<link rel=match href="popup-hidden-display-ref.tentative.html">

<div popup>This content should be visible</div>
<div popup>This content should *not* be visible</div>
<div popup=invalid>This content should *not* be visible</div>

<style>
[popup] {
display: block; /* This should make the popup visible */
display: block !important; /* This should *not* make the popup visible */
top: 0;
width: 300px;
height: 200px;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,11 @@
margin: auto;
border: solid;
overflow: auto;
padding: 1em;
color: CanvasText;
background-color: Canvas;
}
.fake-pop-up-backdrop {
position: fixed;
inset-inline-start: 0;
inset-inline-end: 0;
inset-block-start: 0;
inset-block-end: 0;
background: transparent;
top:0; right:0; bottom:0; left:0;
pointer-events: none !important;
}

0 comments on commit 011f6f1

Please sign in to comment.