Bug 1624080 - Simplify the implementation of HasAuthorSpecifiedRules.…

… r=heycam

This patch computes the author-specified properties during the CSS cascade, and
removes the complex rule-tree-based implementation that tries to do the cascade
again.

This changes behavior in two ways, one of them which is not observable to
content, I believe:

 * revert now re-enables the native styling. This was brought up in
   w3c/csswg-drafts#4777 and I think it is a bug-fix.

   This is observable to content, and I'm adding a test for it.

 * We don't look at inherited styles from our ancestors when `inherit` is
   specified in a non-author stylesheet. This was introduced for bug 452969 but
   we don't seem to inherit background anymore for file controls or such. It
   seems back then file controls used to have a text-field.

   I audited forms.css and ua.css and we don't explicitly inherit
   padding / border / background-color into any nested form control.

We keep the distinction between border/background and padding, because the later
has some callers. I think we should try to align with Chromium in the long run
and remove the padding bit.

We need to give an appearance to the range-thumb and such so that we can assert
that we don't call HasAuthorSpecifiedRules on non-themed stuff. I used a new
internal value for that.

Differential Revision: https://phabricator.services.mozilla.com/D67722

--HG--
extra : moz-landing-system : lando

devtools/shared/css/generated/properties-db.js

@@ -264,7 +264,9 @@ exports.CSS_PROPERTIES = {
       "radio-label",
       "radiomenuitem",
       "range",
+      "range-progress",
       "range-thumb",
+      "range-track",
       "resizer",
       "resizerpanel",
       "revert",
@@ -1581,7 +1583,9 @@ exports.CSS_PROPERTIES = {
       "radio-label",
       "radiomenuitem",
       "range",
+      "range-progress",
       "range-thumb",
+      "range-track",
       "resizer",
       "resizerpanel",
       "revert",

layout/base/nsPresContext.cpp

@@ -1628,28 +1628,23 @@ void nsPresContext::CountReflows(const char* aName, nsIFrame* aFrame) {
 
 bool nsPresContext::HasAuthorSpecifiedRules(const nsIFrame* aFrame,
                                             uint32_t aRuleTypeMask) const {
-  Element* elem = aFrame->GetContent()->AsElement();
-
-  // We need to handle non-generated content pseudos too, so we use
-  // the parent of generated content pseudo to be consistent.
-  if (elem->GetPseudoElementType() != PseudoStyleType::NotPseudo) {
-    MOZ_ASSERT(elem->GetParent(), "Pseudo element has no parent element?");
-    elem = elem->GetParent()->AsElement();
-  }
-  if (MOZ_UNLIKELY(!elem->HasServoData())) {
-    // Probably shouldn't happen, but does. See bug 1387953
-    return false;
+  MOZ_ASSERT(aFrame->StyleDisplay()->HasAppearance(),
+             "This should only be used to disable native appearance");
+  const bool padding = aRuleTypeMask & NS_AUTHOR_SPECIFIED_PADDING;
+  const bool borderBackground =
+      aRuleTypeMask & NS_AUTHOR_SPECIFIED_BORDER_OR_BACKGROUND;
+  const auto& style = *aFrame->Style();
+
+  if (padding && style.HasAppearanceAndAuthorSpecifiedPadding()) {
+    return true;
   }
 
-  // Anonymous boxes are more complicated, and we just assume that they
-  // cannot have any author-specified rules here.
-  if (aFrame->Style()->IsAnonBox()) {
-    return false;
+  if (borderBackground &&
+      style.HasAppearanceAndAuthorSpecifiedBorderOrBackground()) {
+    return true;
   }
 
-  auto* set = PresShell()->StyleSet()->RawSet();
-  return Servo_HasAuthorSpecifiedRules(set, aFrame->Style(), elem,
-                                       aRuleTypeMask);
+  return false;
 }
 
 gfxUserFontSet* nsPresContext::GetUserFontSet() {

layout/base/nsPresContext.h

@@ -117,9 +117,8 @@ enum class nsLayoutPhase : uint8_t {
 #endif
 
 /* Used by nsPresContext::HasAuthorSpecifiedRules */
-#define NS_AUTHOR_SPECIFIED_BACKGROUND (1 << 0)
-#define NS_AUTHOR_SPECIFIED_BORDER (1 << 1)
-#define NS_AUTHOR_SPECIFIED_PADDING (1 << 2)
+#define NS_AUTHOR_SPECIFIED_BORDER_OR_BACKGROUND (1 << 0)
+#define NS_AUTHOR_SPECIFIED_PADDING (1 << 1)
 
 class nsRootPresContext;
 

layout/forms/nsMeterFrame.cpp

@@ -232,11 +232,9 @@ bool nsMeterFrame::ShouldUseNativeStyle() const {
   //   background.
   return StyleDisplay()->mAppearance == StyleAppearance::Meter &&
          !PresContext()->HasAuthorSpecifiedRules(
-             this,
-             NS_AUTHOR_SPECIFIED_BORDER | NS_AUTHOR_SPECIFIED_BACKGROUND) &&
+             this, NS_AUTHOR_SPECIFIED_BORDER_OR_BACKGROUND) &&
          barFrame &&
          barFrame->StyleDisplay()->mAppearance == StyleAppearance::Meterchunk &&
          !PresContext()->HasAuthorSpecifiedRules(
-             barFrame,
-             NS_AUTHOR_SPECIFIED_BORDER | NS_AUTHOR_SPECIFIED_BACKGROUND);
+             barFrame, NS_AUTHOR_SPECIFIED_BORDER_OR_BACKGROUND);
 }

layout/forms/nsNumberControlFrame.cpp

@@ -231,9 +231,8 @@ bool nsNumberControlFrame::SpinnerDownButtonIsDepressed() const {
       ->NumberSpinnerDownButtonIsDepressed();
 }
 
-#define STYLES_DISABLING_NATIVE_THEMING                          \
-  NS_AUTHOR_SPECIFIED_BACKGROUND | NS_AUTHOR_SPECIFIED_PADDING | \
-      NS_AUTHOR_SPECIFIED_BORDER
+#define STYLES_DISABLING_NATIVE_THEMING \
+  NS_AUTHOR_SPECIFIED_BORDER_OR_BACKGROUND | NS_AUTHOR_SPECIFIED_PADDING
 
 bool nsNumberControlFrame::ShouldUseNativeStyleForSpinner() const {
   MOZ_ASSERT(mSpinUp && mSpinDown,

layout/forms/nsProgressFrame.cpp

@@ -248,12 +248,10 @@ bool nsProgressFrame::ShouldUseNativeStyle() const {
   //   background.
   return StyleDisplay()->mAppearance == StyleAppearance::ProgressBar &&
          !PresContext()->HasAuthorSpecifiedRules(
-             this,
-             NS_AUTHOR_SPECIFIED_BORDER | NS_AUTHOR_SPECIFIED_BACKGROUND) &&
+             this, NS_AUTHOR_SPECIFIED_BORDER_OR_BACKGROUND) &&
          barFrame &&
          barFrame->StyleDisplay()->mAppearance ==
              StyleAppearance::Progresschunk &&
          !PresContext()->HasAuthorSpecifiedRules(
-             barFrame,
-             NS_AUTHOR_SPECIFIED_BORDER | NS_AUTHOR_SPECIFIED_BACKGROUND);
+             barFrame, NS_AUTHOR_SPECIFIED_BORDER_OR_BACKGROUND);
 }

layout/forms/nsRangeFrame.cpp

@@ -800,17 +800,15 @@ double nsRangeFrame::GetValue() const {
       .toDouble();
 }
 
-#define STYLES_DISABLING_NATIVE_THEMING                          \
-  NS_AUTHOR_SPECIFIED_BACKGROUND | NS_AUTHOR_SPECIFIED_PADDING | \
-      NS_AUTHOR_SPECIFIED_BORDER
+#define STYLES_DISABLING_NATIVE_THEMING \
+  NS_AUTHOR_SPECIFIED_BORDER_OR_BACKGROUND | NS_AUTHOR_SPECIFIED_PADDING
 
 bool nsRangeFrame::ShouldUseNativeStyle() const {
   nsIFrame* trackFrame = mTrackDiv->GetPrimaryFrame();
   nsIFrame* progressFrame = mProgressDiv->GetPrimaryFrame();
   nsIFrame* thumbFrame = mThumbDiv->GetPrimaryFrame();
 
-  return (StyleDisplay()->mAppearance == StyleAppearance::Range) &&
-         trackFrame &&
+  return StyleDisplay()->mAppearance == StyleAppearance::Range && trackFrame &&
          !PresContext()->HasAuthorSpecifiedRules(
              trackFrame, STYLES_DISABLING_NATIVE_THEMING) &&
          progressFrame &&

layout/reftests/forms/button/appearance-revert-ref.html

@@ -0,0 +1,2 @@
+<!doctype html>
+<button>Foo</button>

layout/reftests/forms/button/appearance-revert.html

@@ -0,0 +1,2 @@
+<!doctype html>
+<button style="border: revert">Foo</button>

layout/reftests/forms/button/reftest.list

@@ -50,3 +50,5 @@ fails-if(Android&&nativeThemePref) == disabled-1.html disabled-1-ref.html
 == dynamic-text-indent.html dynamic-text-indent-ref.html
 
 == 1349646.html 1349646-ref.html
+
+== appearance-revert.html appearance-revert-ref.html

layout/style/ComputedStyle.h

@@ -122,6 +122,19 @@ class ComputedStyle {
     return mPseudoType != PseudoStyleType::NotPseudo;
   }
 
+  // Whether there are author-specified rules for padding properties.
+  // Only returns something meaningful if the appearance property is not `none`.
+  bool HasAppearanceAndAuthorSpecifiedPadding() const {
+    return bool(Flags() & Flag::HAS_AUTHOR_SPECIFIED_PADDING);
+  }
+
+  // Whether there are author-specified rules for border or background
+  // properties.
+  // Only returns something meaningful if the appearance property is not `none`.
+  bool HasAppearanceAndAuthorSpecifiedBorderOrBackground() const {
+    return bool(Flags() & Flag::HAS_AUTHOR_SPECIFIED_BORDER_BACKGROUND);
+  }
+
   // Does this ComputedStyle or any of its ancestors have text
   // decoration lines?
   // Differs from nsStyleTextReset::HasTextDecorationLines, which tests

layout/style/ServoBindings.toml

@@ -167,8 +167,6 @@ rusty-enums = [
     "mozilla::StyleMaskComposite",
 ]
 whitelist-vars = [
-    "NS_AUTHOR_SPECIFIED_.*",
-    "NS_THEME_.*",
     "NS_ATTRVALUE_.*",
     "NODE_.*",
     "ELEMENT_.*",

layout/style/res/forms.css

@@ -909,6 +909,7 @@ input[type=range]::-moz-focus-outer {
  * set the width/height of this pseudo-element.
  */
 input[type=range]::-moz-range-track {
+  -moz-appearance: range-track !important;
   /* Prevent styling that would change the type of frame we construct. */
   display: block !important;
   float: none !important;
@@ -934,6 +935,7 @@ input[type=range][orient=vertical]::-moz-range-track {
  * is ignored.
  */
 input[type=range]::-moz-range-progress {
+  -moz-appearance: range-progress !important;
   /* Prevent styling that would change the type of frame we construct. */
   display: block !important;
   float: none !important;

servo/components/style/properties/cascade.rs

@@ -13,7 +13,7 @@ use crate::media_queries::Device;
 use crate::properties::{ComputedValues, StyleBuilder};
 use crate::properties::{LonghandId, LonghandIdSet, CSSWideKeyword};
 use crate::properties::{PropertyDeclaration, PropertyDeclarationId, DeclarationImportanceIterator};
-use crate::properties::CASCADE_PROPERTY;
+use crate::properties::{CASCADE_PROPERTY, ComputedValueFlags};
 use crate::rule_cache::{RuleCache, RuleCacheConditions};
 use crate::rule_tree::StrongRuleNode;
 use crate::selector_parser::PseudoElement;
@@ -411,6 +411,7 @@ struct Cascade<'a, 'b: 'a> {
     context: &'a mut computed::Context<'b>,
     cascade_mode: CascadeMode<'a>,
     seen: LonghandIdSet,
+    author_specified: LonghandIdSet,
     reverted: PerOrigin<LonghandIdSet>,
 }
 
@@ -420,6 +421,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
             context,
             cascade_mode,
             seen: LonghandIdSet::default(),
+            author_specified: LonghandIdSet::default(),
             reverted: Default::default(),
         }
     }
@@ -557,6 +559,9 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
             }
 
             self.seen.insert(physical_longhand_id);
+            if origin == Origin::Author {
+                self.author_specified.insert(physical_longhand_id);
+            }
 
             let unset = css_wide_keyword.map_or(false, |css_wide_keyword| {
                 match css_wide_keyword {
@@ -679,6 +684,15 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
             if let Some(svg) = builder.get_svg_if_mutated() {
                 svg.fill_arrays();
             }
+
+            if !builder.get_box().clone__moz_appearance().is_none() {
+                if self.author_specified.contains_any(LonghandIdSet::border_background_properties()) {
+                    builder.add_flags(ComputedValueFlags::HAS_AUTHOR_SPECIFIED_BORDER_BACKGROUND);
+                }
+                if self.author_specified.contains_any(LonghandIdSet::padding_properties()) {
+                    builder.add_flags(ComputedValueFlags::HAS_AUTHOR_SPECIFIED_PADDING);
+                }
+            }
         }
 
         #[cfg(feature = "servo")]
@@ -699,12 +713,26 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
             None => return false,
         };
 
-        let cached_style = match cache.find(guards, &self.context.builder) {
+        let builder = &mut self.context.builder;
+
+        let cached_style = match cache.find(guards, &builder) {
             Some(style) => style,
             None => return false,
         };
 
-        self.context.builder.copy_reset_from(cached_style);
+        builder.copy_reset_from(cached_style);
+
+        // We're using the same reset style as another element, and we'll skip
+        // applying the relevant properties. So we need to do the relevant
+        // bookkeeping here to keep these two bits correct.
+        //
+        // Note that all the properties involved are non-inherited, so we don't
+        // need to do anything else other than just copying the bits over.
+        let reset_props_bits =
+            ComputedValueFlags::HAS_AUTHOR_SPECIFIED_BORDER_BACKGROUND |
+            ComputedValueFlags::HAS_AUTHOR_SPECIFIED_PADDING;
+        builder.add_flags(cached_style.flags & reset_props_bits);
+
         true
     }
 

servo/components/style/properties/computed_value_flags.rs

@@ -70,6 +70,20 @@ bitflags! {
 
         /// Whether this element is inside an `opacity: 0` subtree.
         const IS_IN_OPACITY_ZERO_SUBTREE = 1 << 12;
+
+        /// Whether there are author-specified rules for border-* properties
+        /// (except border-image-*), background-color, or background-image.
+        ///
+        /// TODO(emilio): Maybe do include border-image, see:
+        ///
+        /// https://github.com/w3c/csswg-drafts/issues/4777#issuecomment-604424845
+        const HAS_AUTHOR_SPECIFIED_BORDER_BACKGROUND = 1 << 13;
+
+        /// Whether there are author-specified rules for padding-* properties.
+        ///
+        /// FIXME(emilio): Try to merge this with BORDER_BACKGROUND, see
+        /// https://github.com/w3c/csswg-drafts/issues/4777
+        const HAS_AUTHOR_SPECIFIED_PADDING = 1 << 14;
     }
 }