From 30b9476061bdb00397f868f71ef72334a21f9b25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 16 Sep 2024 17:38:52 +0000 Subject: [PATCH] Bug 1918762 - Simplify theme attribute/state change invalidation. r=jwatt We had the same list of attributes in multiple places, put it in Theme.cpp. Only a few states can change widget state. Just hardcode them in RestyleManager.cpp (where there was already some of it) instead of reusing a weird API. Shouldn't have any behavior change. Differential Revision: https://phabricator.services.mozilla.com/D222160 --- gfx/src/nsITheme.h | 11 ++----- layout/base/RestyleManager.cpp | 31 ++++++++---------- widget/Theme.cpp | 38 +++++++++------------- widget/Theme.h | 5 ++- widget/cocoa/nsNativeThemeCocoa.h | 5 ++- widget/cocoa/nsNativeThemeCocoa.mm | 33 +++---------------- widget/gtk/nsNativeThemeGTK.cpp | 50 +++-------------------------- widget/gtk/nsNativeThemeGTK.h | 6 ++-- widget/windows/nsNativeThemeWin.cpp | 39 +++------------------- widget/windows/nsNativeThemeWin.h | 5 ++- 10 files changed, 52 insertions(+), 171 deletions(-) diff --git a/gfx/src/nsITheme.h b/gfx/src/nsITheme.h index 48f563751fc7..cf503f38704c 100644 --- a/gfx/src/nsITheme.h +++ b/gfx/src/nsITheme.h @@ -179,14 +179,9 @@ class nsITheme : public nsISupports { return eUnknownTransparency; } - /** - * Sets |*aShouldRepaint| to indicate whether an attribute or content state - * change should trigger a repaint. Call with null |aAttribute| (and - * null |aOldValue|) for content state changes. - */ - NS_IMETHOD WidgetStateChanged(nsIFrame* aFrame, StyleAppearance aWidgetType, - nsAtom* aAttribute, bool* aShouldRepaint, - const nsAttrValue* aOldValue) = 0; + /** Returns whether an attribute change should trigger a repaint. */ + virtual bool WidgetAttributeChangeRequiresRepaint(StyleAppearance, + nsAtom* aAttribute); NS_IMETHOD ThemeChanged() = 0; diff --git a/layout/base/RestyleManager.cpp b/layout/base/RestyleManager.cpp index 78d84d9272e5..b4700300f02a 100644 --- a/layout/base/RestyleManager.cpp +++ b/layout/base/RestyleManager.cpp @@ -596,9 +596,16 @@ static bool StateChangeMayAffectFrame(const Element& aElement, static bool RepaintForAppearance(nsIFrame& aFrame, const Element& aElement, ElementState aStateMask) { - if (aStateMask.HasAtLeastOneOfStates(ElementState::HOVER | - ElementState::ACTIVE) && - aElement.IsAnyOfXULElements(nsGkAtoms::checkbox, nsGkAtoms::radio)) { + constexpr auto kThemingStates = + ElementState::HOVER | ElementState::ACTIVE | ElementState::FOCUSRING | + ElementState::DISABLED | ElementState::CHECKED | + ElementState::INDETERMINATE | ElementState::READONLY | + ElementState::FOCUS; + if (!aStateMask.HasAtLeastOneOfStates(kThemingStates)) { + return false; + } + + if (aElement.IsAnyOfXULElements(nsGkAtoms::checkbox, nsGkAtoms::radio)) { // The checkbox inside these elements inherit hover state and so on, see // nsNativeTheme::GetContentState. // FIXME(emilio): Would be nice to not have these hard-coded. @@ -609,13 +616,7 @@ static bool RepaintForAppearance(nsIFrame& aFrame, const Element& aElement, return false; } nsPresContext* pc = aFrame.PresContext(); - nsITheme* theme = pc->Theme(); - if (!theme->ThemeSupportsWidget(pc, &aFrame, appearance)) { - return false; - } - bool repaint = false; - theme->WidgetStateChanged(&aFrame, appearance, nullptr, &repaint, nullptr); - return repaint; + return pc->Theme()->ThemeSupportsWidget(pc, &aFrame, appearance); } /** @@ -3654,13 +3655,9 @@ void RestyleManager::AttributeChanged(Element* aElement, int32_t aNameSpaceID, primaryFrame->StyleDisplay()->EffectiveAppearance(); if (appearance != StyleAppearance::None) { nsITheme* theme = PresContext()->Theme(); - if (theme->ThemeSupportsWidget(PresContext(), primaryFrame, appearance)) { - bool repaint = false; - theme->WidgetStateChanged(primaryFrame, appearance, aAttribute, - &repaint, aOldValue); - if (repaint) { - changeHint |= nsChangeHint_RepaintFrame; - } + if (theme->ThemeSupportsWidget(PresContext(), primaryFrame, appearance) && + theme->WidgetAttributeChangeRequiresRepaint(appearance, aAttribute)) { + changeHint |= nsChangeHint_RepaintFrame; } } diff --git a/widget/Theme.cpp b/widget/Theme.cpp index 5a653640bcd6..486040747fd6 100644 --- a/widget/Theme.cpp +++ b/widget/Theme.cpp @@ -1631,29 +1631,21 @@ nsITheme::Transparency Theme::GetWidgetTransparency( return eUnknownTransparency; } -NS_IMETHODIMP -Theme::WidgetStateChanged(nsIFrame* aFrame, StyleAppearance aAppearance, - nsAtom* aAttribute, bool* aShouldRepaint, - const nsAttrValue* aOldValue) { - if (!aAttribute) { - // Hover/focus/active changed. Always repaint. - *aShouldRepaint = true; - } else { - // Check the attribute to see if it's relevant. - // disabled, checked, dlgtype, default, etc. - *aShouldRepaint = false; - if (aAttribute == nsGkAtoms::disabled || aAttribute == nsGkAtoms::checked || - aAttribute == nsGkAtoms::selected || - aAttribute == nsGkAtoms::visuallyselected || - aAttribute == nsGkAtoms::menuactive || - aAttribute == nsGkAtoms::sortDirection || - aAttribute == nsGkAtoms::focused || aAttribute == nsGkAtoms::_default || - aAttribute == nsGkAtoms::open || aAttribute == nsGkAtoms::hover) { - *aShouldRepaint = true; - } - } - - return NS_OK; +bool Theme::WidgetAttributeChangeRequiresRepaint(StyleAppearance aAppearance, + nsAtom* aAttribute) { + // Check the attribute to see if it's relevant. + // TODO(emilio): The non-native theme doesn't use these attributes. Other + // themes do, but not all of them (and not all of the ones they check are + // here). + return aAttribute == nsGkAtoms::disabled || + aAttribute == nsGkAtoms::checked || + aAttribute == nsGkAtoms::selected || + aAttribute == nsGkAtoms::visuallyselected || + aAttribute == nsGkAtoms::menuactive || + aAttribute == nsGkAtoms::sortDirection || + aAttribute == nsGkAtoms::focused || + aAttribute == nsGkAtoms::_default || aAttribute == nsGkAtoms::open || + aAttribute == nsGkAtoms::hover; } NS_IMETHODIMP diff --git a/widget/Theme.h b/widget/Theme.h index c09c41405684..e7882d497bfe 100644 --- a/widget/Theme.h +++ b/widget/Theme.h @@ -77,9 +77,8 @@ class Theme : protected nsNativeTheme, public nsITheme { LayoutDeviceIntSize GetMinimumWidgetSize(nsPresContext*, nsIFrame*, StyleAppearance) override; Transparency GetWidgetTransparency(nsIFrame*, StyleAppearance) override; - NS_IMETHOD WidgetStateChanged(nsIFrame*, StyleAppearance, nsAtom* aAttribute, - bool* aShouldRepaint, - const nsAttrValue* aOldValue) override; + bool WidgetAttributeChangeRequiresRepaint(StyleAppearance, + nsAtom* aAttribute) override; NS_IMETHOD ThemeChanged() override; bool WidgetAppearanceDependsOnWindowFocus(StyleAppearance) override; /*bool NeedToClearBackgroundBehindWidget( diff --git a/widget/cocoa/nsNativeThemeCocoa.h b/widget/cocoa/nsNativeThemeCocoa.h index 66e5aec1df83..fca7672979ce 100644 --- a/widget/cocoa/nsNativeThemeCocoa.h +++ b/widget/cocoa/nsNativeThemeCocoa.h @@ -272,9 +272,8 @@ class nsNativeThemeCocoa : public mozilla::widget::ThemeCocoa { LayoutDeviceIntSize GetMinimumWidgetSize(nsPresContext*, nsIFrame*, StyleAppearance) override; - NS_IMETHOD WidgetStateChanged(nsIFrame*, StyleAppearance, nsAtom* aAttribute, - bool* aShouldRepaint, - const nsAttrValue* aOldValue) override; + bool WidgetAttributeChangeRequiresRepaint(StyleAppearance, + nsAtom* aAttribute) override; NS_IMETHOD ThemeChanged() override; bool ThemeSupportsWidget(nsPresContext* aPresContext, nsIFrame*, StyleAppearance) override; diff --git a/widget/cocoa/nsNativeThemeCocoa.mm b/widget/cocoa/nsNativeThemeCocoa.mm index cf432ee48528..fd89533dee74 100644 --- a/widget/cocoa/nsNativeThemeCocoa.mm +++ b/widget/cocoa/nsNativeThemeCocoa.mm @@ -2871,11 +2871,8 @@ LayoutDeviceIntSize nsNativeThemeCocoa::GetMinimumWidgetSize( NS_OBJC_END_TRY_BLOCK_RETURN(LayoutDeviceIntSize()); } -NS_IMETHODIMP -nsNativeThemeCocoa::WidgetStateChanged(nsIFrame* aFrame, - StyleAppearance aAppearance, - nsAtom* aAttribute, bool* aShouldRepaint, - const nsAttrValue* aOldValue) { +bool nsNativeThemeCocoa::WidgetAttributeChangeRequiresRepaint( + StyleAppearance aAppearance, nsAtom* aAttribute) { // Some widget types just never change state. switch (aAppearance) { case StyleAppearance::MozWindowTitlebar: @@ -2889,33 +2886,11 @@ nsNativeThemeCocoa::WidgetStateChanged(nsIFrame* aFrame, case StyleAppearance::ProgressBar: case StyleAppearance::Meter: case StyleAppearance::Meterchunk: - *aShouldRepaint = false; - return NS_OK; + return false; default: break; } - - // XXXdwh Not sure what can really be done here. Can at least guess for - // specific widgets that they're highly unlikely to have certain states. - // For example, a toolbar doesn't care about any states. - if (!aAttribute) { - // Hover/focus/active changed. Always repaint. - *aShouldRepaint = true; - } else { - // Check the attribute to see if it's relevant. - // disabled, checked, dlgtype, default, etc. - *aShouldRepaint = false; - if (aAttribute == nsGkAtoms::disabled || aAttribute == nsGkAtoms::checked || - aAttribute == nsGkAtoms::selected || - aAttribute == nsGkAtoms::visuallyselected || - aAttribute == nsGkAtoms::menuactive || - aAttribute == nsGkAtoms::sortDirection || - aAttribute == nsGkAtoms::focused || aAttribute == nsGkAtoms::_default || - aAttribute == nsGkAtoms::open || aAttribute == nsGkAtoms::hover) - *aShouldRepaint = true; - } - - return NS_OK; + return Theme::WidgetAttributeChangeRequiresRepaint(aAppearance, aAttribute); } NS_IMETHODIMP diff --git a/widget/gtk/nsNativeThemeGTK.cpp b/widget/gtk/nsNativeThemeGTK.cpp index 54f331992c19..45439292a1e4 100644 --- a/widget/gtk/nsNativeThemeGTK.cpp +++ b/widget/gtk/nsNativeThemeGTK.cpp @@ -1127,58 +1127,16 @@ LayoutDeviceIntSize nsNativeThemeGTK::GetMinimumWidgetSize( GetWidgetScaleFactor(aFrame)); } -NS_IMETHODIMP -nsNativeThemeGTK::WidgetStateChanged(nsIFrame* aFrame, - StyleAppearance aAppearance, - nsAtom* aAttribute, bool* aShouldRepaint, - const nsAttrValue* aOldValue) { - *aShouldRepaint = false; - - if (IsWidgetNonNative(aFrame, aAppearance) != NonNative::No) { - return Theme::WidgetStateChanged(aFrame, aAppearance, aAttribute, - aShouldRepaint, aOldValue); - } - +bool nsNativeThemeGTK::WidgetAttributeChangeRequiresRepaint( + StyleAppearance aAppearance, nsAtom* aAttribute) { // Some widget types just never change state. if (aAppearance == StyleAppearance::Progresschunk || aAppearance == StyleAppearance::ProgressBar || aAppearance == StyleAppearance::Tooltip || aAppearance == StyleAppearance::MozWindowDecorations) { - return NS_OK; + return false; } - - if (aAppearance == StyleAppearance::MozWindowTitlebar || - aAppearance == StyleAppearance::MozWindowTitlebarMaximized || - aAppearance == StyleAppearance::MozWindowButtonClose || - aAppearance == StyleAppearance::MozWindowButtonMinimize || - aAppearance == StyleAppearance::MozWindowButtonMaximize || - aAppearance == StyleAppearance::MozWindowButtonRestore) { - *aShouldRepaint = true; - return NS_OK; - } - - // XXXdwh Not sure what can really be done here. Can at least guess for - // specific widgets that they're highly unlikely to have certain states. - // For example, a toolbar doesn't care about any states. - if (!aAttribute) { - // Hover/focus/active changed. Always repaint. - *aShouldRepaint = true; - return NS_OK; - } - - // Check the attribute to see if it's relevant. - // disabled, checked, dlgtype, default, etc. - *aShouldRepaint = false; - if (aAttribute == nsGkAtoms::disabled || aAttribute == nsGkAtoms::checked || - aAttribute == nsGkAtoms::selected || - aAttribute == nsGkAtoms::visuallyselected || - aAttribute == nsGkAtoms::focused || aAttribute == nsGkAtoms::readonly || - aAttribute == nsGkAtoms::_default || - aAttribute == nsGkAtoms::menuactive || aAttribute == nsGkAtoms::open) { - *aShouldRepaint = true; - return NS_OK; - } - return NS_OK; + return Theme::WidgetAttributeChangeRequiresRepaint(aAppearance, aAttribute); } NS_IMETHODIMP diff --git a/widget/gtk/nsNativeThemeGTK.h b/widget/gtk/nsNativeThemeGTK.h index 62a046c95989..67a0398129c0 100644 --- a/widget/gtk/nsNativeThemeGTK.h +++ b/widget/gtk/nsNativeThemeGTK.h @@ -64,10 +64,8 @@ class nsNativeThemeGTK final : public mozilla::widget::Theme { nsPresContext* aPresContext, nsIFrame* aFrame, StyleAppearance aAppearance) override; - NS_IMETHOD WidgetStateChanged(nsIFrame* aFrame, StyleAppearance aAppearance, - nsAtom* aAttribute, bool* aShouldRepaint, - const nsAttrValue* aOldValue) override; - + bool WidgetAttributeChangeRequiresRepaint(StyleAppearance, + nsAtom* aAttribute) override; NS_IMETHOD ThemeChanged() override; NS_IMETHOD_(bool) diff --git a/widget/windows/nsNativeThemeWin.cpp b/widget/windows/nsNativeThemeWin.cpp index 1cbad3777c9b..fb4ef0142a39 100644 --- a/widget/windows/nsNativeThemeWin.cpp +++ b/widget/windows/nsNativeThemeWin.cpp @@ -1290,49 +1290,18 @@ LayoutDeviceIntSize nsNativeThemeWin::GetMinimumWidgetSize( return result; } -NS_IMETHODIMP -nsNativeThemeWin::WidgetStateChanged(nsIFrame* aFrame, - StyleAppearance aAppearance, - nsAtom* aAttribute, bool* aShouldRepaint, - const nsAttrValue* aOldValue) { +bool nsNativeThemeWin::WidgetAttributeChangeRequiresRepaint( + StyleAppearance aAppearance, nsAtom* aAttribute) { // Some widget types just never change state. if (aAppearance == StyleAppearance::Progresschunk || aAppearance == StyleAppearance::ProgressBar || aAppearance == StyleAppearance::Tabpanels || aAppearance == StyleAppearance::Tabpanel || aAppearance == StyleAppearance::Separator) { - *aShouldRepaint = false; - return NS_OK; + return false; } - // We need to repaint the dropdown arrow in vista HTML combobox controls when - // the control is closed to get rid of the hover effect. - if ((aAppearance == StyleAppearance::Menulist || - aAppearance == StyleAppearance::MenulistButton) && - nsNativeTheme::IsHTMLContent(aFrame)) { - *aShouldRepaint = true; - return NS_OK; - } - - // XXXdwh Not sure what can really be done here. Can at least guess for - // specific widgets that they're highly unlikely to have certain states. - // For example, a toolbar doesn't care about any states. - if (!aAttribute) { - // Hover/focus/active changed. Always repaint. - *aShouldRepaint = true; - } else { - // Check the attribute to see if it's relevant. - // disabled, checked, dlgtype, default, etc. - *aShouldRepaint = false; - if (aAttribute == nsGkAtoms::disabled || aAttribute == nsGkAtoms::checked || - aAttribute == nsGkAtoms::selected || - aAttribute == nsGkAtoms::visuallyselected || - aAttribute == nsGkAtoms::readonly || aAttribute == nsGkAtoms::open || - aAttribute == nsGkAtoms::menuactive || aAttribute == nsGkAtoms::focused) - *aShouldRepaint = true; - } - - return NS_OK; + return Theme::WidgetAttributeChangeRequiresRepaint(aAppearance, aAttribute); } NS_IMETHODIMP diff --git a/widget/windows/nsNativeThemeWin.h b/widget/windows/nsNativeThemeWin.h index 99370181977b..e6eb1622856f 100644 --- a/widget/windows/nsNativeThemeWin.h +++ b/widget/windows/nsNativeThemeWin.h @@ -69,9 +69,8 @@ class nsNativeThemeWin : public Theme { virtual Transparency GetWidgetTransparency( nsIFrame* aFrame, StyleAppearance aAppearance) override; - NS_IMETHOD WidgetStateChanged(nsIFrame* aFrame, StyleAppearance aAppearance, - nsAtom* aAttribute, bool* aShouldRepaint, - const nsAttrValue* aOldValue) override; + bool WidgetAttributeChangeRequiresRepaint(StyleAppearance aAppearance, + nsAtom* aAttribute) override; NS_IMETHOD ThemeChanged() override;