RESOLVED FIXED 223946
Do not paint native decorations for search fields without "-webkit-appearance: searchfield"
https://bugs.webkit.org/show_bug.cgi?id=223946
Summary Do not paint native decorations for search fields without "-webkit-appearance...
Aditya Keerthi
Reported 2021-03-30 12:38:37 PDT
...
Attachments
Patch (13.26 KB, patch)
2021-03-30 12:41 PDT, Aditya Keerthi
no flags
Patch (11.47 KB, patch)
2021-04-01 11:42 PDT, Aditya Keerthi
no flags
Patch (16.70 KB, patch)
2021-04-01 18:23 PDT, Aditya Keerthi
koivisto: review+
Patch for landing (16.38 KB, patch)
2021-04-02 07:13 PDT, Aditya Keerthi
no flags
Aditya Keerthi
Comment 1 2021-03-30 12:38:52 PDT
Aditya Keerthi
Comment 2 2021-03-30 12:41:09 PDT
zalan
Comment 3 2021-03-30 12:45:26 PDT
How does this handle dynamic style change?
Aditya Keerthi
Comment 4 2021-03-30 13:06:04 PDT
(In reply to zalan from comment #3) > How does this handle dynamic style change? RenderSearchField::updateFromElement() is called after style recalc (via HTMLFormControlElement::didRecalcStyle).
zalan
Comment 5 2021-03-30 13:38:35 PDT
updateFromElement is so confusing to me, like what's the purpose of this function from a renderer's point of view, and whether when a change is purely style driven, wouldn't styleDid/WillChange be the appropriate place to trigger things like adjusting inner style? I am not saying this is incorrect, I am just wondering if someone has a good understanding on updateFromElement in general.
Aditya Keerthi
Comment 6 2021-03-30 14:20:32 PDT
(In reply to zalan from comment #5) > updateFromElement is so confusing to me, like what's the purpose of this > function from a renderer's point of view, and whether when a change is > purely style driven, wouldn't styleDid/WillChange be the appropriate place > to trigger things like adjusting inner style? I am not saying this is > incorrect, I am just wondering if someone has a good understanding on > updateFromElement in general. styleDidChange does seem like a better place to make this change, since the adjustment doesn't dependent on any element state (unlike the visibility of the cancel button, which depends on whether or not the element has a non-empty value). I'll update the patch.
Aditya Keerthi
Comment 7 2021-03-30 15:28:14 PDT
I tried moving the logic into styleDidChange, but I'm now facing some issues with dynamic style change: 1. Using inline styles (setInlineStyleProperty/removeInlineStyleProperty) does not immediately repaint, so the icon is still visible until I scroll the view or interact with the field. 2. Since inline styles weren't working, I tried modifying the renderer's mutableStyle(). This works as expected, but I'm unsure how to reset the mutableStyle's appearance to the original value if the appearance is changed back to "searchfield" (this was handled by removeInlineStyleProperty with the inline style approach). Any suggestions on how to proceed?
Aditya Keerthi
Comment 8 2021-03-30 15:52:47 PDT
mutableStyle() is probably not the right approach either, since a dynamic style change while the input is focused results in the icon re-appearing after the focus is lost.
zalan
Comment 9 2021-03-30 16:37:47 PDT
(In reply to Aditya Keerthi from comment #7) > I tried moving the logic into styleDidChange, but I'm now facing some issues > with dynamic style change: > > 1. Using inline styles (setInlineStyleProperty/removeInlineStyleProperty) > does not immediately repaint, so the icon is still visible until I scroll > the view or interact with the field. > > 2. Since inline styles weren't working, I tried modifying the renderer's > mutableStyle(). This works as expected, but I'm unsure how to reset the > mutableStyle's appearance to the original value if the appearance is changed > back to "searchfield" (this was handled by removeInlineStyleProperty with > the inline style approach). > > Any suggestions on how to proceed? I am not familiar with how setInlineStyleProperty works but you can certainly issue repaint() in styleDidChange when appearance changes.
Aditya Keerthi
Comment 10 2021-03-30 18:56:36 PDT
(In reply to zalan from comment #9) > (In reply to Aditya Keerthi from comment #7) > > I tried moving the logic into styleDidChange, but I'm now facing some issues > > with dynamic style change: > > > > 1. Using inline styles (setInlineStyleProperty/removeInlineStyleProperty) > > does not immediately repaint, so the icon is still visible until I scroll > > the view or interact with the field. > > > > 2. Since inline styles weren't working, I tried modifying the renderer's > > mutableStyle(). This works as expected, but I'm unsure how to reset the > > mutableStyle's appearance to the original value if the appearance is changed > > back to "searchfield" (this was handled by removeInlineStyleProperty with > > the inline style approach). > > > > Any suggestions on how to proceed? > I am not familiar with how setInlineStyleProperty works but you can > certainly issue repaint() in styleDidChange when appearance changes. Tried repaint() and setNeedsLayout(), but neither worked when using setInlineStyleProperty.
Antti Koivisto
Comment 11 2021-03-31 00:50:01 PDT
Comment on attachment 424675 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424675&action=review > Source/WebCore/rendering/RenderSearchField.cpp:193 > +void RenderSearchField::updateResultsButtonAppearance() const > +{ > + if (style().appearance() != SearchFieldPart || document().quirks().shouldHideSearchFieldResultsButton()) { > + resultsButtonElement()->setInlineStyleProperty(CSSPropertyWebkitAppearance, CSSValueNone); > + return; > + } > + > + resultsButtonElement()->removeInlineStyleProperty(CSSPropertyWebkitAppearance); > +} RenderSearchField::updateFromElement() gets called after style resolution from HTMLFormControlElement::didRecalcStyle. Mutating the style here will result in another style resolution. Also mutating DOM from the render tree is generally wrong, DOM/style is the input not the output. I believe the correct way to do this is to implement SearchFieldResultsButtonElement::resolveCustomStyle() function and looking into appearance of the passed in shadowHostStyle. See TextControlPlaceholderElement::resolveCustomStyle and others for examples.
Aditya Keerthi
Comment 12 2021-03-31 14:26:57 PDT
Splitting up this patch to separate the iOS-specific changes.
Aditya Keerthi
Comment 13 2021-04-01 11:42:30 PDT
Aditya Keerthi
Comment 14 2021-04-01 18:23:10 PDT
Antti Koivisto
Comment 15 2021-04-02 06:37:17 PDT
Comment on attachment 424971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424971&action=review > Source/WebCore/html/shadow/TextControlInnerElements.cpp:249 > + SetForScope<bool> canAdjustStyleForAppearance(m_canAdjustStyleForAppearance, false); Bit of a hack but ok.
Antti Koivisto
Comment 16 2021-04-02 06:40:33 PDT
Comment on attachment 424971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424971&action=review > Source/WebCore/html/shadow/TextControlInnerElements.cpp:252 > + elementStyle.renderStyle->setAppearance(NoControlPart); This adjustment could be done in Theme as a result of canAdjustStyleForAppearance being false.
Antti Koivisto
Comment 17 2021-04-02 06:46:47 PDT
Comment on attachment 424971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424971&action=review > Source/WebCore/rendering/RenderTheme.h:71 > + void adjustSearchFieldDecorationStyle(RenderStyle&, const Element*) const; This probably doesn't need to be public if you are not invoking it directly. Always invoking it directly from shadow tree Element classes instead of from adjustStyle would be an alternative strategy that could avoid out-of-band signalling via the bit.
Aditya Keerthi
Comment 18 2021-04-02 07:13:06 PDT
Created attachment 425017 [details] Patch for landing
Aditya Keerthi
Comment 19 2021-04-02 07:13:41 PDT
(In reply to Antti Koivisto from comment #16) > Comment on attachment 424971 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424971&action=review > > > Source/WebCore/html/shadow/TextControlInnerElements.cpp:252 > > + elementStyle.renderStyle->setAppearance(NoControlPart); > > This adjustment could be done in Theme as a result of > canAdjustStyleForAppearance being false. Moved into Theme.
Aditya Keerthi
Comment 20 2021-04-02 07:18:37 PDT
(In reply to Antti Koivisto from comment #17) > Comment on attachment 424971 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424971&action=review > > > Source/WebCore/rendering/RenderTheme.h:71 > > + void adjustSearchFieldDecorationStyle(RenderStyle&, const Element*) const; > > This probably doesn't need to be public if you are not invoking it directly. > > Always invoking it directly from shadow tree Element classes instead of from > adjustStyle would be an alternative strategy that could avoid out-of-band > signalling via the bit. Made the method private. Discussed offline - keeping the patch as-is since invoking it directly from SearchFieldResultsButtonElement would still require keeping some logic in the Theme to adjust other elements. Consequently, we would still need to skip adjustments in the Theme when the element is a SearchFieldResultsButtonElement. The logic in Theme needs to be preserved to support cases like: ``` <style> #decoration { -webkit-appearance: searchfield-results-decoration; } </style> <div id="decoration"></div> ``` Long term, we should find a way to make the shadow host style available to the style adjuster, avoiding the need for any additional logic in SearchFieldResultsButtonElement.
EWS
Comment 21 2021-04-02 11:17:13 PDT
Committed r275426: <https://commits.webkit.org/r275426> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425017 [details].
Note You need to log in before you can comment on or make changes to this bug.