WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(11.47 KB, patch)
2021-04-01 11:42 PDT
,
Aditya Keerthi
no flags
Details
Formatted Diff
Diff
Patch
(16.70 KB, patch)
2021-04-01 18:23 PDT
,
Aditya Keerthi
koivisto
: review+
Details
Formatted Diff
Diff
Patch for landing
(16.38 KB, patch)
2021-04-02 07:13 PDT
,
Aditya Keerthi
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Aditya Keerthi
Comment 1
2021-03-30 12:38:52 PDT
<
rdar://problem/75621365
>
Aditya Keerthi
Comment 2
2021-03-30 12:41:09 PDT
Created
attachment 424675
[details]
Patch
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
Created
attachment 424917
[details]
Patch
Aditya Keerthi
Comment 14
2021-04-01 18:23:10 PDT
Created
attachment 424971
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug