Skip to content

Commit

Permalink
Make font inherit for appearance:base-select
Browse files Browse the repository at this point in the history
This matches what WebKit is prototyping and what we have been discussing
in standards.
https://github.com/WebKit/WebKit/blob/9b7ebfe8b3ba328a4f1e26a9d8c714bad1fff079/Source/WebCore/css/html.css#L1498-L1542

This patch also changes the behavior of
-internal-appearance-auto-base-select(): it now takes into account any
appearance:base-select specified on an ancestor <select> element.

Fixed: 368325983
Change-Id: Ibe795ff560f8f8e92667da9955850f426c58911d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5883714
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Auto-Submit: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1376415}
  • Loading branch information
josepharhar authored and pull[bot] committed Nov 6, 2024
1 parent da1dccc commit 1042877
Show file tree
Hide file tree
Showing 9 changed files with 83 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,10 @@ static void AdjustStyleForMarker(ComputedStyleBuilder& builder,

static void AdjustStyleForHTMLElement(ComputedStyleBuilder& builder,
HTMLElement& element) {
if (builder.HasBaseSelectAppearance()) {
builder.SetInBaseSelectAppearance(true);
}

// <div> and <span> are the most common elements on the web, we skip all the
// work for them.
if (IsA<HTMLDivElement>(element) || IsA<HTMLSpanElement>(element)) {
Expand Down
26 changes: 5 additions & 21 deletions third_party/blink/renderer/core/css/resolver/style_cascade.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1713,27 +1713,11 @@ bool StyleCascade::ResolveAppearanceAutoBaseSelectInto(
}
LookupAndApply(appearance, resolver);

// The UA stylesheet only uses -internal-appearance-auto-base-select(),
// on select elements, which is currently the only element which supports
// appearance:base-select.
CHECK(IsA<HTMLSelectElement>(state_.GetUltimateOriginatingElementOrSelf()));
bool has_base_appearance = state_.StyleBuilder().HasBaseSelectAppearance();
if (state_.IsForPseudoElement()) {
CHECK_EQ(state_.GetPseudoElement()->GetPseudoId(), kPseudoIdAfter)
<< " -internal-appearance-base-select() is only supported on "
"select::after right now.";
// There is a rule in the UA sheet for select::after which uses
// -internal-appearance-auto-base-select(), so for that rule we have to
// account for this here by checking the style of the select element instead
// of this state_ which is for ::after.
// Both state_.LayoutParentStyle() and
// state_.GetElement().GetComputedStyle() seem to have the correct
// appearance value set.
// TODO(crbug.com/1511354): LayoutParentStyle might not be the right thing
// to call for all pseudo-elements.
has_base_appearance = state_.LayoutParentStyle()->EffectiveAppearance() ==
ControlPart::kBaseSelectPart;
}
// Note that the InBaseSelectAppearance() flag is set by StyleAdjuster,
// which hasn't happened yet. Therefore we also need to check
// HasBaseSelectAppearance() here.
bool has_base_appearance = state_.StyleBuilder().HasBaseSelectAppearance() ||
state_.StyleBuilder().InBaseSelectAppearance();

if (has_base_appearance) {
// We want to the second argument.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

@namespace "http://www.w3.org/1999/xhtml";

select:not(:-internal-list-box) {
font: -internal-appearance-auto-base-select(-webkit-small-control, inherit);
}

/* These rules are intended to apply to appearance:base-select when the author
* provides a child <button> element. They still apply when appearance computes
* to auto, but the values provided in the
Expand Down Expand Up @@ -111,6 +115,7 @@ select:not(:-internal-list-box) option {
min-inline-size: 24px;
min-block-size: max(24px, 1.2em);
align-content: center;
font-weight: -internal-appearance-auto-base-select(normal, inherit);
}


Expand Down Expand Up @@ -160,6 +165,7 @@ select:not(:-internal-list-box) optgroup {
/* This can be merged with the :-internal-list-box rule in html.css */
select:not(:-internal-list-box) optgroup option {
padding-inline-start: 20px;
font-weight: normal;
}

/* These styles are copied from optgroup's label attribute inline style properties */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1307,5 +1307,13 @@
default_value: "false",
inherited: false,
},
{
name: "InBaseSelectAppearance",
inherited: true,
field_template: "primitive",
type_name: "bool",
field_group: "*",
default_value: "false",
},
],
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@

.customizable-select-button, .customizable-select-popover {
box-sizing: border-box;
/* TODO(crbug.com/1511354): Make the UA stylesheet use something standardized
* instead of -webkit-small-control. */
font: -webkit-small-control;
}

.customizable-select-popover {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<!DOCTYPE html>
<link rel=stylesheet href="resources/customizable-select-styles.css">

<div class="customizable-select-button open" popovertarget=popover id=button>
<span class=customizable-select-selectedoption>one</span>
</div>
<div id=popover popover=auto anchor=button class=customizable-select-popover>
<div class="customizable-select-option selected">one</div>
<div class=customizable-select-option>two</div>
</div>

<style>
* {
font-style: italic !important;
font-weight: bold !important;
font-stretch: expanded !important;
font-size: 13px !important;
font-family: monospace !important;
}
</style>

<script>
document.getElementById('popover').showPopover();
</script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<!DOCTYPE html>
<html class=reftest-wait>
<meta name=fuzzy content="maxDifference=0-21;totalPixels=0-8">
<link rel=author href="mailto:jarhar@chromium.org">
<link rel=help href="https://github.com/whatwg/html/issues/9799">
<link rel=match href="select-appearance-font-inheriting-ref.html">
<script src="/resources/testdriver.js"></script>
<script src="/resources/testdriver-vendor.js"></script>

<style>
select, select::picker(select) {
appearance: base-select;
}
body {
font-style: italic;
font-weight: bold;
font-stretch: expanded;
font-size: 13px;
font-family: monospace;
}
</style>

<select>
<option>one</option>
<option>two</option>
</select>

<script>
(async () => {
await test_driver.bless();
document.querySelector('select').showPicker();
document.documentElement.classList.remove('reftest-wait');
})();
</script>
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<!DOCTYPE html>
<html class=reftest-wait>
<meta name=fuzzy content="maxDifference=0-16;totalPixels=0-6">
<meta name=fuzzy content="maxDifference=0-41;totalPixels=0-6">
<link rel=author href="mailto:jarhar@chromium.org">
<link rel=help href="https://github.com/whatwg/html/issues/9799">
<link rel=match href="select-appearance-picker-select-border-ref.html">
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<!DOCTYPE html>
<html class=reftest-wait>
<meta name=fuzzy content="maxDifference=0-16;totalPixels=0-6">
<meta name=fuzzy content="maxDifference=0-41;totalPixels=0-6">
<link rel=author href="mailto:jarhar@chromium.org">
<link rel=match href="select-popover-exit-animation-ref.html">
<script src="/resources/testdriver.js"></script>
Expand Down

0 comments on commit 1042877

Please sign in to comment.