Skip to content

Commit b391993

Browse files
author
Alexey Shvayka
committed
formDisabledCallback() sometimes fires even when disabled-ness hasn't changed
https://bugs.webkit.org/show_bug.cgi?id=251097 Reviewed by Ryosuke Niwa. This change: 1. Replaces disabledAttributeChanged() hook, which was only used by <fieldset>, with hasDisabledAttribute() protected method, removing a virtual call and reducing sizeof(HTMLFieldSetElement) by 8. 2. Extracts setDisabledInternal() so that disabledStateChanged() hook is called only when disabled-ness is actually changed, always taking into account m_disabledByAncestorFieldset. The latter wasn't a huge issue before form-associated custom elements were introduced, whose disabledAttributeChanged() calls into userland JS code, yet even apart from that, this change eliminates redundant work for native controls and even makes psedo-class invalidation more precise, reducing matches{Valid,Invalid}PseudoClass() calls. Also, moving the pseudo-class invalidation into a single function paves the way for a follow-up patch that will invalidate a few more selectors to match the spec. Aligns WebKit with Blink and Gecko. * LayoutTests/imported/w3c/web-platform-tests/custom-elements/form-associated/form-disabled-callback-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/custom-elements/form-associated/form-disabled-callback.html: * Source/WebCore/html/HTMLFieldSetElement.cpp: (WebCore::HTMLFieldSetElement::~HTMLFieldSetElement): (WebCore::HTMLFieldSetElement::parseAttribute): (WebCore::HTMLFieldSetElement::didMoveToNewDocument): (WebCore::HTMLFieldSetElement::disabledAttributeChanged): Deleted. * Source/WebCore/html/HTMLFieldSetElement.h: * Source/WebCore/html/ValidatedFormListedElement.cpp: (WebCore::ValidatedFormListedElement::setDisabledByAncestorFieldset): (WebCore::ValidatedFormListedElement::setDisabledInternal): (WebCore::ValidatedFormListedElement::parseDisabledAttribute): (WebCore::ValidatedFormListedElement::syncWithFieldsetAncestors): (WebCore::ValidatedFormListedElement::removedFromAncestor): (WebCore::ValidatedFormListedElement::disabledAttributeChanged): Deleted. * Source/WebCore/html/ValidatedFormListedElement.h: (WebCore::ValidatedFormListedElement::hasDisabledAttribute const): Canonical link: https://commits.webkit.org/259372@main
1 parent d6d79fb commit b391993

File tree

6 files changed

+64
-34
lines changed

6 files changed

+64
-34
lines changed

‎LayoutTests/imported/w3c/web-platform-tests/custom-elements/form-associated/form-disabled-callback-expected.txt‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,14 @@
22

33

44

5+
6+
57
PASS Adding/removing disabled content attribute
68
PASS Relationship with FIELDSET
79
PASS A disabled form-associated custom element should not provide an entry for it
810
PASS A disabled form-associated custom element should not submit an entry for it
911
PASS Disabled attribute affects focus-capability
1012
PASS Upgrading an element with disabled content attribute
13+
PASS Toggling "disabled" attribute on a custom element inside disabled <fieldset> does not trigger a callback
14+
PASS Toggling "disabled" attribute on a <fieldset> does not trigger a callback on disabled custom element descendant
1115

‎LayoutTests/imported/w3c/web-platform-tests/custom-elements/form-associated/form-disabled-callback.html‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,5 +110,27 @@
110110
container.innerHTML = '<fieldset disabled><my-control>';
111111
assert_array_equals(container.querySelector('my-control').disabledHistory(), [true]);
112112
}, 'Upgrading an element with disabled content attribute');
113+
114+
test(() => {
115+
const container = document.createElement('div');
116+
document.body.appendChild(container);
117+
container.innerHTML = '<fieldset disabled><my-control></my-control></fieldset>';
118+
119+
const control = container.querySelector('my-control');
120+
control.setAttribute('disabled', '');
121+
control.removeAttribute('disabled');
122+
assert_array_equals(control.disabledHistory(), [true]);
123+
}, 'Toggling "disabled" attribute on a custom element inside disabled <fieldset> does not trigger a callback');
124+
125+
test(() => {
126+
const container = document.createElement('div');
127+
document.body.appendChild(container);
128+
container.innerHTML = '<fieldset><my-control disabled></my-control></fieldset>';
129+
130+
const fieldset = container.firstElementChild;
131+
fieldset.disabled = true;
132+
fieldset.disabled = false;
133+
assert_array_equals(container.querySelector('my-control').disabledHistory(), [true]);
134+
}, 'Toggling "disabled" attribute on a <fieldset> does not trigger a callback on disabled custom element descendant');
113135
</script>
114136
</body>

‎Source/WebCore/html/HTMLFieldSetElement.cpp‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ inline HTMLFieldSetElement::HTMLFieldSetElement(const QualifiedName& tagName, Do
5252

5353
HTMLFieldSetElement::~HTMLFieldSetElement()
5454
{
55-
if (m_hasDisabledAttribute)
55+
if (hasDisabledAttribute())
5656
document().removeDisabledFieldsetElement();
5757
}
5858

@@ -82,18 +82,16 @@ static void updateFromControlElementsAncestorDisabledStateUnder(HTMLElement& sta
8282
}
8383
}
8484

85-
void HTMLFieldSetElement::disabledAttributeChanged()
85+
void HTMLFieldSetElement::parseAttribute(const QualifiedName& name, const AtomString& value)
8686
{
87-
bool hasDisabledAttribute = hasAttributeWithoutSynchronization(disabledAttr);
88-
if (m_hasDisabledAttribute != hasDisabledAttribute) {
89-
m_hasDisabledAttribute = hasDisabledAttribute;
90-
if (hasDisabledAttribute)
87+
HTMLFormControlElement::parseAttribute(name, value);
88+
89+
if (name == disabledAttr) {
90+
if (hasDisabledAttribute())
9191
document().addDisabledFieldsetElement();
9292
else
9393
document().removeDisabledFieldsetElement();
9494
}
95-
96-
HTMLFormControlElement::disabledAttributeChanged();
9795
}
9896

9997
void HTMLFieldSetElement::disabledStateChanged()
@@ -136,7 +134,7 @@ void HTMLFieldSetElement::didMoveToNewDocument(Document& oldDocument, Document&
136134
{
137135
ASSERT_WITH_SECURITY_IMPLICATION(&document() == &newDocument);
138136
HTMLFormControlElement::didMoveToNewDocument(oldDocument, newDocument);
139-
if (m_hasDisabledAttribute) {
137+
if (hasDisabledAttribute()) {
140138
oldDocument.removeDisabledFieldsetElement();
141139
newDocument.addDisabledFieldsetElement();
142140
}

‎Source/WebCore/html/HTMLFieldSetElement.h‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* Copyright (C) 1999 Lars Knoll ([email protected])
33
* (C) 1999 Antti Koivisto ([email protected])
44
* (C) 2000 Dirk Mueller ([email protected])
5-
* Copyright (C) 2004, 2005, 2006, 2010 Apple Inc. All rights reserved.
5+
* Copyright (C) 2004-2023 Apple Inc. All rights reserved.
66
*
77
* This library is free software; you can redistribute it and/or
88
* modify it under the terms of the GNU Library General Public
@@ -49,7 +49,7 @@ class HTMLFieldSetElement final : public HTMLFormControlElement {
4949
RenderPtr<RenderElement> createElementRenderer(RenderStyle&&, const RenderTreePosition&) final;
5050
const AtomString& formControlType() const final;
5151
bool computeWillValidate() const final { return false; }
52-
void disabledAttributeChanged() final;
52+
void parseAttribute(const QualifiedName&, const AtomString&) final;
5353
void disabledStateChanged() final;
5454
void childrenChanged(const ChildChange&) final;
5555
void didMoveToNewDocument(Document& oldDocument, Document& newDocument) final;
@@ -58,7 +58,6 @@ class HTMLFieldSetElement final : public HTMLFormControlElement {
5858
bool matchesInvalidPseudoClass() const final;
5959

6060
WeakHashSet<HTMLElement, WeakPtrImplWithEventTargetData> m_invalidDescendants;
61-
bool m_hasDisabledAttribute { false };
6261
};
6362

6463
} // namespace

‎Source/WebCore/html/ValidatedFormListedElement.cpp‎

Lines changed: 27 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -197,16 +197,30 @@ bool ValidatedFormListedElement::isFocusingWithValidationMessage() const
197197
return m_isFocusingWithValidationMessage;
198198
}
199199

200-
void ValidatedFormListedElement::setDisabledByAncestorFieldset(bool isDisabled)
200+
void ValidatedFormListedElement::setDisabledByAncestorFieldset(bool newDisabledByAncestorFieldset)
201201
{
202-
ASSERT(computeIsDisabledByFieldsetAncestor() == isDisabled);
203-
if (m_disabledByAncestorFieldset == isDisabled)
204-
return;
202+
ASSERT(computeIsDisabledByFieldsetAncestor() == newDisabledByAncestorFieldset);
203+
setDisabledInternal(m_disabled, newDisabledByAncestorFieldset);
204+
}
205+
206+
void ValidatedFormListedElement::setDisabledInternal(bool disabled, bool disabledByAncestorFieldset)
207+
{
208+
bool newDisabledState = disabled || disabledByAncestorFieldset;
209+
bool changingDisabledState = newDisabledState != isDisabled();
210+
211+
std::optional<Style::PseudoClassChangeInvalidation> styleInvalidation;
212+
if (changingDisabledState) {
213+
emplace(styleInvalidation, asHTMLElement(), {
214+
{ CSSSelector::PseudoClassDisabled, newDisabledState },
215+
{ CSSSelector::PseudoClassEnabled, !newDisabledState },
216+
});
217+
}
205218

206-
Style::PseudoClassChangeInvalidation disabledInvalidation(asHTMLElement(), { { CSSSelector::PseudoClassDisabled, isDisabled }, { CSSSelector::PseudoClassEnabled, !isDisabled } });
219+
m_disabled = disabled;
220+
m_disabledByAncestorFieldset = disabledByAncestorFieldset;
207221

208-
m_disabledByAncestorFieldset = isDisabled;
209-
disabledStateChanged();
222+
if (changingDisabledState)
223+
disabledStateChanged();
210224
}
211225

212226
static void addInvalidElementToAncestorFromInsertionPoint(const HTMLElement& element, ContainerNode* insertionPoint)
@@ -285,11 +299,7 @@ void ValidatedFormListedElement::parseDisabledAttribute(const AtomString& value)
285299
ASSERT(asHTMLElement().canBeActuallyDisabled());
286300

287301
bool newDisabled = !value.isNull();
288-
if (m_disabled != newDisabled) {
289-
Style::PseudoClassChangeInvalidation disabledInvalidation(asHTMLElement(), { { CSSSelector::PseudoClassDisabled, newDisabled }, { CSSSelector::PseudoClassEnabled, !newDisabled } });
290-
m_disabled = newDisabled;
291-
disabledAttributeChanged();
292-
}
302+
setDisabledInternal(newDisabled, m_disabledByAncestorFieldset);
293303
}
294304

295305
void ValidatedFormListedElement::parseReadOnlyAttribute(const AtomString& value)
@@ -305,11 +315,6 @@ void ValidatedFormListedElement::parseReadOnlyAttribute(const AtomString& value)
305315
}
306316
}
307317

308-
void ValidatedFormListedElement::disabledAttributeChanged()
309-
{
310-
disabledStateChanged();
311-
}
312-
313318
void ValidatedFormListedElement::insertedIntoAncestor(Node::InsertionType insertionType, ContainerNode& parentOfInsertedTree)
314319
{
315320
m_isInsideDataList = TriState::Indeterminate;
@@ -326,10 +331,11 @@ void ValidatedFormListedElement::setDataListAncestorState(TriState isInsideDataL
326331

327332
void ValidatedFormListedElement::syncWithFieldsetAncestors(ContainerNode* insertionPoint)
328333
{
334+
HTMLElement& element = asHTMLElement();
329335
if (matchesInvalidPseudoClass())
330-
addInvalidElementToAncestorFromInsertionPoint(asHTMLElement(), insertionPoint);
331-
if (asHTMLElement().document().hasDisabledFieldsetElement())
332-
setDisabledByAncestorFieldset(computeIsDisabledByFieldsetAncestor());
336+
addInvalidElementToAncestorFromInsertionPoint(element, insertionPoint);
337+
if (element.document().hasDisabledFieldsetElement())
338+
setDisabledInternal(m_disabled, computeIsDisabledByFieldsetAncestor());
333339
}
334340

335341
void ValidatedFormListedElement::removedFromAncestor(Node::RemovalType removalType, ContainerNode& oldParentOfRemovedTree)
@@ -339,7 +345,7 @@ void ValidatedFormListedElement::removedFromAncestor(Node::RemovalType removalTy
339345

340346
m_validationMessage = nullptr;
341347
if (m_disabledByAncestorFieldset)
342-
setDisabledByAncestorFieldset(computeIsDisabledByFieldsetAncestor());
348+
setDisabledInternal(m_disabled, computeIsDisabledByFieldsetAncestor());
343349

344350
bool wasInsideDataList = m_isInsideDataList == TriState::True;
345351
if (wasInsideDataList)

‎Source/WebCore/html/ValidatedFormListedElement.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@ class ValidatedFormListedElement : public FormListedElement {
9191
virtual const AtomString& formControlType() const = 0;
9292

9393
protected:
94+
bool hasDisabledAttribute() const { return m_disabled; }
9495
virtual bool computeWillValidate() const;
9596
virtual bool readOnlyBarsFromConstraintValidation() const { return false; }
9697
void updateWillValidateAndValidity();
@@ -105,7 +106,6 @@ class ValidatedFormListedElement : public FormListedElement {
105106
void parseDisabledAttribute(const AtomString&);
106107
void parseReadOnlyAttribute(const AtomString&);
107108

108-
virtual void disabledAttributeChanged();
109109
virtual void disabledStateChanged();
110110
virtual void readOnlyStateChanged();
111111

@@ -118,6 +118,7 @@ class ValidatedFormListedElement : public FormListedElement {
118118

119119
private:
120120
bool computeIsDisabledByFieldsetAncestor() const;
121+
void setDisabledInternal(bool disabled, bool disabledByAncestorFieldset);
121122
virtual HTMLElement* validationAnchorElement() = 0;
122123

123124
void startDelayingUpdateValidity() { ++m_delayedUpdateValidityCount; }

0 commit comments

Comments
 (0)