Bug 238551

Created attachment 456118 [details]
Patch

Created attachment 456350 [details]
Patch

Comment on attachment 456350 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        The test failures though, are not fully fixed. The issue left is that the search cancel button
> +        for input type="search" still present while it is expected to disappear.

Can we try to get the 2 tests passing? Just to match WebKit's policy of having tests that start passing for every patch.

Maybe this is a good place to start looking: https://webkit-search.igalia.com/webkit/source/Source/WebCore/html/shadow/TextControlInnerElements.cpp#290-315

<rdar://problem/91252542>

I filed bug 238751 for input[type=search]

This is a good starting point:

diff --git a/Source/WebCore/html/shadow/TextControlInnerElements.cpp b/Source/WebCore/html/shadow/TextControlInnerElements.cpp
index c0e8fee503fe..0098997ddc63 100644
--- a/Source/WebCore/html/shadow/TextControlInnerElements.cpp
+++ b/Source/WebCore/html/shadow/TextControlInnerElements.cpp
@@ -306,11 +306,13 @@ Ref<SearchFieldCancelButtonElement> SearchFieldCancelButtonElement::create(Docum
     return element;
 }
 
-std::optional<Style::ElementStyle> SearchFieldCancelButtonElement::resolveCustomStyle(const Style::ResolutionContext& resolutionContext, const RenderStyle*)
+std::optional<Style::ElementStyle> SearchFieldCancelButtonElement::resolveCustomStyle(const Style::ResolutionContext& resolutionContext, const RenderStyle* shadowHostStyle)
 {
     auto elementStyle = resolveStyle(resolutionContext);
     auto& inputElement = downcast<HTMLInputElement>(*shadowHost());
     elementStyle.renderStyle->setVisibility(elementStyle.renderStyle->visibility() == Visibility::Hidden || inputElement.value().isEmpty() ? Visibility::Hidden : Visibility::Visible);
+    if (shadowHostStyle->effectiveAppearance() == TextFieldPart)
+        elementStyle.renderStyle->setEffectiveDisplay(DisplayType::None);
     return elementStyle;
 }



Remaining issues are:
- width is too large compared to <input type="text"> (not quite sure why)
- focusring offset is too large compared to <input type="text"> (html.css sets input[type=text] outline-offset to -2px, but [type=search] to 0, outline-offset needs to be tied to appearance: textfield instead of input[type])

It would be good to fix this first in bug 238751

Comment on attachment 456350 [details]
Patch

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

> Source/WebCore/rendering/RenderTheme.cpp:98
> +    if (part == AutoPart || part == SearchFieldPart || part == SearchFieldCancelButtonPart || part == TextAreaPart || part == CheckboxPart || part == RadioPart || part == ListboxPart || part == MeterPart || part == ProgressBarPart

I unexposed SearchFieldCancelButtonPart in bug 240384, this isn't needed

> Source/WebCore/rendering/RenderTheme.cpp:128
> +#if ENABLE(INPUT_TYPE_COLOR)
> +        // Add restriction of using appearances of `textfield` for color element. See 
> +        // https://bugs.webkit.org/show_bug.cgi?id=191279
> +        if (is<HTMLInputElement>(*element) && downcast<HTMLInputElement>(*element).isColorControl())
> +            return part;
> +#endif

I'm not really sure this behavior is worth preserving. It doesn't match the spec and I don't see the point of setting appearance: textfield on <input type=color>.

data:text/html,<input%20type=color%20style="appearance:textfield">

Aditya/Wenson, what do you think?

> > Source/WebCore/rendering/RenderTheme.cpp:128
> > +#if ENABLE(INPUT_TYPE_COLOR)
> > +        // Add restriction of using appearances of `textfield` for color element. See 
> > +        // https://bugs.webkit.org/show_bug.cgi?id=191279
> > +        if (is<HTMLInputElement>(*element) && downcast<HTMLInputElement>(*element).isColorControl())
> > +            return part;
> > +#endif
> 
> I'm not really sure this behavior is worth preserving. It doesn't match the
> spec and I don't see the point of setting appearance: textfield on <input
> type=color>.
> 
> data:text/html,<input%20type=color%20style="appearance:textfield">
> 
> Aditya/Wenson, what do you think?

Fwiw, the relevant test (fast/forms/color/color-input-uses-color-well-appearance.html) argues that <input type=color> and <input type=color style="appearance:textfield"> should look different. The spec argues for otherwise.

Looking at the original bug (bug 191279), I don't think testing this was necessarily the intent.

(In reply to Tim Nguyen (:ntim) from comment #7)
> Comment on attachment 456350 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456350&action=review
> 
> > Source/WebCore/rendering/RenderTheme.cpp:98
> > +    if (part == AutoPart || part == SearchFieldPart || part == SearchFieldCancelButtonPart || part == TextAreaPart || part == CheckboxPart || part == RadioPart || part == ListboxPart || part == MeterPart || part == ProgressBarPart
> 
> I unexposed SearchFieldCancelButtonPart in bug 240384, this isn't needed
> 
> > Source/WebCore/rendering/RenderTheme.cpp:128
> > +#if ENABLE(INPUT_TYPE_COLOR)
> > +        // Add restriction of using appearances of `textfield` for color element. See 
> > +        // https://bugs.webkit.org/show_bug.cgi?id=191279
> > +        if (is<HTMLInputElement>(*element) && downcast<HTMLInputElement>(*element).isColorControl())
> > +            return part;
> > +#endif
> 
> I'm not really sure this behavior is worth preserving. It doesn't match the
> spec and I don't see the point of setting appearance: textfield on <input
> type=color>.
> 
> data:text/html,<input%20type=color%20style="appearance:textfield">
> 
> Aditya/Wenson, what do you think?

Agreed. This appears to be an artifact of the time when <input type=color> was just a text box with a hex color code.

We can remove the test, or change '-expected-mismatch.html' to '-expected.html', if this is not already covered by a WPT.

(In reply to Aditya Keerthi from comment #9)
> (In reply to Tim Nguyen (:ntim) from comment #7)
> > Comment on attachment 456350 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=456350&action=review
> > 
> > > Source/WebCore/rendering/RenderTheme.cpp:98
> > > +    if (part == AutoPart || part == SearchFieldPart || part == SearchFieldCancelButtonPart || part == TextAreaPart || part == CheckboxPart || part == RadioPart || part == ListboxPart || part == MeterPart || part == ProgressBarPart
> > 
> > I unexposed SearchFieldCancelButtonPart in bug 240384, this isn't needed
> > 
> > > Source/WebCore/rendering/RenderTheme.cpp:128
> > > +#if ENABLE(INPUT_TYPE_COLOR)
> > > +        // Add restriction of using appearances of `textfield` for color element. See 
> > > +        // https://bugs.webkit.org/show_bug.cgi?id=191279
> > > +        if (is<HTMLInputElement>(*element) && downcast<HTMLInputElement>(*element).isColorControl())
> > > +            return part;
> > > +#endif
> > 
> > I'm not really sure this behavior is worth preserving. It doesn't match the
> > spec and I don't see the point of setting appearance: textfield on <input
> > type=color>.
> > 
> > data:text/html,<input%20type=color%20style="appearance:textfield">
> > 
> > Aditya/Wenson, what do you think?
> 
> Agreed. This appears to be an artifact of the time when <input type=color>
> was just a text box with a hex color code.
> 
> We can remove the test, or change '-expected-mismatch.html' to
> '-expected.html', if this is not already covered by a WPT.

I'd be ok with the renaming option, given we don't fully pass the corresponding WPT for other reasons (bug 238751), and that would allow us to have a test for this change.

Ziran, does that plan sound ok to you?

Created attachment 459411 [details]
Patch

Created attachment 459412 [details]
Patch

Comment on attachment 459412 [details]
Patch

r=me if EWS is green

Created attachment 459418 [details]
[fast-cq] Patch

Committed r294249 (250605@main): <https://commits.webkit.org/250605@main>

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