...
<rdar://problem/75621365>
Created attachment 424675 [details] Patch
How does this handle dynamic style change?
(In reply to zalan from comment #3) > How does this handle dynamic style change? RenderSearchField::updateFromElement() is called after style recalc (via HTMLFormControlElement::didRecalcStyle).
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.
(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.
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?
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.
(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.
(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.
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.
Splitting up this patch to separate the iOS-specific changes.
Created attachment 424917 [details] Patch
Created attachment 424971 [details] Patch
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.
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.
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.
Created attachment 425017 [details] Patch for landing
(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.
(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.
Committed r275426: <https://commits.webkit.org/r275426> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425017 [details].