Bug 223946 – Do not paint native decorations for search fields without "-webkit-appearance: searchfield"

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/75621365>

[Aditya Keerthi]

Created attachment 424675 [details]
Patch

[zalan]

How does this handle dynamic style change?

[Aditya Keerthi]

(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]

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]

(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]

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]

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]

(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]

(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 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]

Splitting up this patch to separate the iOS-specific changes.

[Aditya Keerthi]

Created attachment 424917 [details]
Patch

[Aditya Keerthi]

Created attachment 424971 [details]
Patch

[Antti Koivisto]

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 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 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]

Created attachment 425017 [details]
Patch for landing

[Aditya Keerthi]

(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]

(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]

Committed r275426: <https://commits.webkit.org/r275426>

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