Add support for alias appearance <compat-auto> keywords to 'auto' for textfield
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].