Bug 220502 – Radio buttons with no form owner are not grouped

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Matt]

I created a series of tests to address issue #2551 (https://github.com/web-platform-tests/wpt/issues/2551) of the Web Platform Tests project. One of the tests is failing when run against Safari 118 Preview. The test consists of two radio button inputs that should be grouped according to the criteria in the HTML spec (type=radio, same tree, identical name attribute values, and no form owner).

The test results can be viewed in the Safari wpt.fyi link for the WPT pull request (https://github.com/web-platform-tests/wpt/pull/27029). The relevant test is labeled "Radio buttons in different groups (because they have different form owners or no form owner) do not affect each other's checkedness".

The test reveals that two radio buttons meeting all the criteria to be in the same group (type=radio, same tree, identical name attribute values, and no form owner) are not being properly grouped. The checked statuses of the two buttons are not mutually exclusive.

Link to the test results is https://wpt.fyi/results/html/semantics/forms/the-input-element/radio.html?diff&filter=ADC&run_id=5710239401771008&run_id=5647309809385472

[Radar WebKit Bug Importer]

<rdar://problem/73300895>

[Smoley]

Thanks for filing, I can reproduce this on Safari 13.1.3, STP118 and Firefox using http://wpt.live/html/semantics/forms/the-input-element/radio.html. This does not reproduce on Chrome.

[Chris Dumez]

*** Bug 233847 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Created attachment 446048 [details]
WIP Patch

[Chris Dumez]

Created attachment 446062 [details]
WIP Patch

[Chris Dumez]

Created attachment 446431 [details]
WIP Patch

[Chris Dumez]

Created attachment 446443 [details]
Patch

[Chris Dumez]

Created attachment 446548 [details]
Patch

[Darin Adler]

Comment on attachment 446548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446548&action=review

> Source/WebCore/html/HTMLInputElement.cpp:1617
> +    if (removalType.disconnectedFromDocument && !form() && isRadioButton())
> +        updateValidity();

Maybe it’s obvious, but why can we skip this update if form() is non-null?

> Source/WebCore/html/HTMLInputElement.cpp:1967
> +    if (checked())
> +        return const_cast<HTMLInputElement*>(this);

I guess this is an optimization. Or is it for correctness?

> Source/WebCore/html/HTMLInputElement.cpp:1980
> +    // The input is not managed by a RadioButtonGroups, we'll need to traverse the tree.
> +    for (auto& descendant : descendantsOfType<HTMLInputElement>(rootNode())) {
> +        if (descendant.isRadioButton() && name == descendant.name() && !descendant.form() && descendant.checked())
> +            return &descendant;
> +    }

I understand that the Chromium engineer who wrote this code was OK with the performance cost of walking the tree every time. But we are OK with that? What is the pathological worst case version of this?

Also, the checked() function is a super-fast one, so maybe it should go earlier in this clause. Faster than isRadioButton (which calls a virtual function), faster than a string comparison, even faster than the really simple form function call (which is not inlined).

> Source/WebCore/html/RadioInputType.cpp:49
> +    auto name = element()->name();

Maybe auto& to avoid some reference count churn.

> Source/WebCore/html/RadioInputType.cpp:59
> +    auto& treeRoot = element()->rootNode();
> +    for (auto* input = Traversal<HTMLInputElement>::inclusiveFirstWithin(treeRoot); input; input = Traversal<HTMLInputElement>::next(*input, &treeRoot)) {

I’d rather have this use inclusiveDescendantsOfType, iterator style. You’d have to add that, though. It would go in TypedDescendantsIterator.h:

    for (auto& input : inclusiveDescendantsOfType<HTMLInputElement>(element()->rootNode())) {

Same question as above about the cost of walking the tree every time.

> Source/WebCore/html/RadioInputType.cpp:67
> +        if (isChecked && isRequired)
> +            return false;

We can return false as soon as *isChecked* is true, doesn’t matter what the state of *isRequired* is. So we don’t need an isChecked boolean, just return false as soon as we find a checked one.

[Chris Dumez]

Comment on attachment 446548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446548&action=review

>> Source/WebCore/html/HTMLInputElement.cpp:1617
>> +        updateValidity();
> 
> Maybe it’s obvious, but why can we skip this update if form() is non-null?

If the radio button has an associated form, then it is part of a RadioButtonGroup (held by the form element) and RadioButtonGroups properly maintain radio buttons validity.

The issue is for radio buttons that are both disconnected AND have no form element. In this case, the radio button is no longer part of a RadioButtonGroup and we need to traverse its detached dom tree to figure out its validity.

>> Source/WebCore/html/HTMLInputElement.cpp:1967
>> +        return const_cast<HTMLInputElement*>(this);
> 
> I guess this is an optimization. Or is it for correctness?

This is an optimization. That said, it I dropped this optimization, I would need to also use inclusiveDescendantsOfType<> below instead of descendantsOfType<>.

>> Source/WebCore/html/HTMLInputElement.cpp:1980
>> +    }
> 
> I understand that the Chromium engineer who wrote this code was OK with the performance cost of walking the tree every time. But we are OK with that? What is the pathological worst case version of this?
> 
> Also, the checked() function is a super-fast one, so maybe it should go earlier in this clause. Faster than isRadioButton (which calls a virtual function), faster than a string comparison, even faster than the really simple form function call (which is not inlined).

It is true that traversing the DOM tree is a bit unfortunate. That said, this only occurs in the case where the input element is detached from the tree. The bad case is when you have a radio button that is part of a large detached subtree (e.g. in a document fragment).
I am currently A/B testing on Speedometer to make sure it has no impact there but I don't expect this to be a common case in practice.

>> Source/WebCore/html/RadioInputType.cpp:49
>> +    auto name = element()->name();
> 
> Maybe auto& to avoid some reference count churn.

OK.

>> Source/WebCore/html/RadioInputType.cpp:59
>> +    for (auto* input = Traversal<HTMLInputElement>::inclusiveFirstWithin(treeRoot); input; input = Traversal<HTMLInputElement>::next(*input, &treeRoot)) {
> 
> I’d rather have this use inclusiveDescendantsOfType, iterator style. You’d have to add that, though. It would go in TypedDescendantsIterator.h:
> 
>     for (auto& input : inclusiveDescendantsOfType<HTMLInputElement>(element()->rootNode())) {
> 
> Same question as above about the cost of walking the tree every time.

Good idea, I'll look into adding an inclusiveDescendantsOfType().

>> Source/WebCore/html/RadioInputType.cpp:67
>> +            return false;
> 
> We can return false as soon as *isChecked* is true, doesn’t matter what the state of *isRequired* is. So we don’t need an isChecked boolean, just return false as soon as we find a checked one.

Oh, that is a very good point. I completely missed that!
I'll fix.

[Chris Dumez]

Created attachment 446591 [details]
Patch

[Chris Dumez]

Still waiting on perf A/B results.

[Darin Adler]

Comment on attachment 446591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446591&action=review

> Source/WebCore/html/RadioInputType.cpp:51
> +        return buttons->isInRequiredGroup(*element()) && !buttons->checkedButtonForGroup(name);

Reverse the order of checks here if the second is less expensive?

> Source/WebCore/html/RadioInputType.cpp:63
> +        if (input.isRequired())
> +            isRequired = true;

If isRequired is costly, could call it only if boolean is not already true.

[Chris Dumez]

(In reply to Darin Adler from comment #13)
> Comment on attachment 446591 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446591&action=review
> 
> > Source/WebCore/html/RadioInputType.cpp:51
> > +        return buttons->isInRequiredGroup(*element()) && !buttons->checkedButtonForGroup(name);
> 
> Reverse the order of checks here if the second is less expensive?

Ok.

> 
> > Source/WebCore/html/RadioInputType.cpp:63
> > +        if (input.isRequired())
> > +            isRequired = true;
> 
> If isRequired is costly, could call it only if boolean is not already true.

isRequired() just returns a boolean data member. However, it is currently out-of-line, I think I should just inline it.

[Chris Dumez]

Created attachment 446595 [details]
Patch

[Chris Dumez]

Comment on attachment 446595 [details]
Patch

Perf-neutral on Speedometer

[Chris Dumez]

Comment on attachment 446595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446595&action=review

> Source/WebCore/html/RadioInputType.cpp:49
> +    auto name = element()->name();

Forgot to switch to auto& here. Will address.

[Chris Dumez]

Created attachment 446728 [details]
Patch for landing

[EWS]

Committed r286855 (245088@main): <https://commits.webkit.org/245088@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446728 [details].

[Chris Dumez]

Reopening since this got reverted in https://commits.webkit.org/246446@main.

[Chris Dumez]

Created attachment 450052 [details]
Patch

[Chris Dumez]

Created attachment 450182 [details]
Patch

[EWS]

Committed r288734 (246527@main): <https://commits.webkit.org/246527@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450182 [details].