nj.gov: Background color incorrect for 'State Vehicles' section
Created attachment 455006 [details] Patch
Comment on attachment 455006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455006&action=review > Source/WebCore/rendering/RenderTheme.cpp:114 > +static bool shouldIgnoreAppearance(RenderStyle& style) Not really my area but it feels like this parameter could/should be const.
Created attachment 455009 [details] Patch
Comment on attachment 455006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455006&action=review > Source/WebCore/ChangeLog:12 > + controlled elements when they are styled by the author. "controlled" -> "control" >> Source/WebCore/rendering/RenderTheme.cpp:114 >> +static bool shouldIgnoreAppearance(RenderStyle& style) > > Not really my area but it feels like this parameter could/should be const. Would be nice if this method took two RenderStyle parameters, and was renamed, so that `RenderTheme::isControlStyled` can share the logic. (and make the parameters const, as Chris said) > LayoutTests/fast/css/non-form-control-element-drop-appearance.html:3 > + -webkit-appearance: button; Let's use the unprefixed "appearance".
(In reply to Aditya Keerthi from comment #4) > Comment on attachment 455006 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=455006&action=review > > > Source/WebCore/ChangeLog:12 > > + controlled elements when they are styled by the author. > > "controlled" -> "control" > will fix. > >> Source/WebCore/rendering/RenderTheme.cpp:114 > >> +static bool shouldIgnoreAppearance(RenderStyle& style) > > > > Not really my area but it feels like this parameter could/should be const. > > Would be nice if this method took two RenderStyle parameters, and was > renamed, so that `RenderTheme::isControlStyled` can share the logic. > > (and make the parameters const, as Chris said) > sounds good, will fix > > LayoutTests/fast/css/non-form-control-element-drop-appearance.html:3 > > + -webkit-appearance: button; > > Let's use the unprefixed "appearance". ok. why?
(In reply to Kate Cheney from comment #5) > (In reply to Aditya Keerthi from comment #4) > > Comment on attachment 455006 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=455006&action=review > > > > > Source/WebCore/ChangeLog:12 > > > + controlled elements when they are styled by the author. > > > > "controlled" -> "control" > > > > will fix. > > > >> Source/WebCore/rendering/RenderTheme.cpp:114 > > >> +static bool shouldIgnoreAppearance(RenderStyle& style) > > > > > > Not really my area but it feels like this parameter could/should be const. > > > > Would be nice if this method took two RenderStyle parameters, and was > > renamed, so that `RenderTheme::isControlStyled` can share the logic. > > > > (and make the parameters const, as Chris said) > > > sounds good, will fix > > > > LayoutTests/fast/css/non-form-control-element-drop-appearance.html:3 > > > + -webkit-appearance: button; > > > > Let's use the unprefixed "appearance". > > ok. why? There's no behavioral difference, because it's an alias, but I think it's better to use the unprefixed version of the property in new code.
Created attachment 455021 [details] Patch
failing layout tests seem possibly related..
Created attachment 455054 [details] Patch
Comment on attachment 455054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455054&action=review > Source/WebCore/rendering/RenderTheme.cpp:114 > +static bool styleBorderAndBackgroundDiffer(const RenderStyle& style, const RenderStyle& otherStyle) I saw that we have the following methods in RenderStyle which compare two styles: ``` bool inheritedEqual(const RenderStyle&) const; bool descendantAffectingNonInheritedPropertiesEqual(const RenderStyle&) const; ``` Consider declaring this as `bool borderAndBackgroundEqual(const RenderStyle&) const;` and implementing in `RenderStyle`. > Source/WebCore/rendering/RenderTheme.cpp:124 > + auto part = adjustAppearanceForElement(style, element, autoAppearanceForElement); I prefer to avoid out-parameters when possible, as they add indirection when reading code, and can give a single method too much responsibility. Consider moving the call to `autoAppearanceForElement()` to the line above, and passing the result into `adjustAppearanceForElement`.
Comment on attachment 455054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455054&action=review (just style comments, leaving substantive review to Aditya) > Source/WebCore/rendering/RenderTheme.cpp:87 > +ControlPart RenderTheme::adjustAppearanceForElement(RenderStyle& style, const Element* element, ControlPart& autoAppearance) const It would be more stylish to return both ControlPart as return values, either as struct or std::pair. Caller can then use structured bindings to unpack auto [appareance, autoAppearance] = adjustAppearanceForElement(...). > Source/WebCore/rendering/RenderTheme.cpp:114 > +static bool styleBorderAndBackgroundDiffer(const RenderStyle& style, const RenderStyle& otherStyle) "differ" is an odd function name. Maybe something more descriptive like shouldIgnoreApparanceForStyle(style, baseStyle)? > Source/WebCore/rendering/RenderTheme.cpp:151 > + if (!userAgentAppearanceStyle && autoAppearanceForElement == NoControlPart && styleBorderAndBackgroundDiffer(style, style.defaultStyle())) defaultStyle() is a static function, it is clearer to use RenderStyle::defaultStyle() to call it.
Created attachment 455102 [details] Patch
Created attachment 455103 [details] PFL once EWS is happy
Created attachment 455266 [details] Patch
Committed r291588 (248683@main): <https://commits.webkit.org/248683@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 455266 [details].
<rdar://problem/90600712>
*** Bug 223980 has been marked as a duplicate of this bug. ***