Feedback from code review

src/textual/css/model.py

@@ -156,7 +156,7 @@ class RuleSet:
     styles: Styles = field(default_factory=Styles)
     errors: list[tuple[Token, str]] = field(default_factory=list)
     classes: set[str] = field(default_factory=set)
-    is_widget_rule_set: bool = False
+    is_default_rules: bool = False
 
     @classmethod
     def _selector_to_css(cls, selectors: list[Selector]) -> str:

src/textual/css/parse.py

@@ -80,7 +80,9 @@ def parse_selectors(css_selectors: str) -> tuple[SelectorSet, ...]:
     return selector_set
 
 
-def parse_rule_set(tokens: Iterator[Token], token: Token) -> Iterable[RuleSet]:
+def parse_rule_set(
+    tokens: Iterator[Token], token: Token, is_default_rules: bool = False
+) -> Iterable[RuleSet]:
     get_selector = SELECTOR_MAP.get
     combinator: CombinatorType | None = CombinatorType.DESCENDENT
     selectors: list[Selector] = []
@@ -149,7 +151,10 @@ def parse_rule_set(tokens: Iterator[Token], token: Token) -> Iterable[RuleSet]:
             errors.append((error.token, error.message))
 
     rule_set = RuleSet(
-        list(SelectorSet.from_selectors(rule_selectors)), styles_builder.styles, errors
+        list(SelectorSet.from_selectors(rule_selectors)),
+        styles_builder.styles,
+        errors,
+        is_default_rules=is_default_rules,
     )
     rule_set._post_parse()
     yield rule_set
@@ -307,7 +312,10 @@ def substitute_references(
 
 
 def parse(
-    css: str, path: str | PurePath, variables: dict[str, str] | None = None
+    css: str,
+    path: str | PurePath,
+    variables: dict[str, str] | None = None,
+    is_default_rules: bool = False,
 ) -> Iterable[RuleSet]:
     """Parse CSS by tokenizing it, performing variable substitution,
     and generating rule sets from it.
@@ -315,6 +323,9 @@ def parse(
     Args:
         css (str): The input CSS
         path (str): Path to the CSS
+        variables (dict[str, str]): Substitution variables to substitute tokens for.
+        is_default_rules (bool): True if the rules we're extracting are
+            default (i.e. in Widget.CSS) rules. False if they're from user defined CSS.
     """
     variable_tokens = tokenize_values(variables or {})
     tokens = iter(substitute_references(tokenize(css, path), variable_tokens))
@@ -323,7 +334,7 @@ def parse(
         if token is None:
             break
         if token.name.startswith("selector_start"):
-            yield from parse_rule_set(tokens, token)
+            yield from parse_rule_set(tokens, token, is_default_rules=is_default_rules)
 
 
 if __name__ == "__main__":

src/textual/css/styles.py

@@ -514,13 +514,15 @@ class Styles(StylesBase):
     def extract_rules(
         self,
         specificity: Specificity3,
-        is_widget_rule: bool = False,
+        is_default_rules: bool = False,
     ) -> list[tuple[str, Specificity5, Any]]:
         """Extract rules from Styles object, and apply !important css specificity as
         well as higher specificity of user CSS vs widget CSS.
 
         Args:
             specificity (Specificity3): A node specificity.
+            is_default_rules (bool): True if the rules we're extracting are
+                default (i.e. in Widget.CSS) rules. False if they're from user defined CSS.
 
         Returns:
             list[tuple[str, Specificity5, Any]]]: A list containing a tuple of <RULE NAME>, <SPECIFICITY> <RULE VALUE>.
@@ -531,7 +533,7 @@ class Styles(StylesBase):
             (
                 rule_name,
                 (
-                    0 if is_widget_rule else 1,
+                    0 if is_default_rules else 1,
                     1 if is_important(rule_name) else 0,
                     *specificity,
                 ),

src/textual/css/stylesheet.py

@@ -4,7 +4,7 @@ import os
 from collections import defaultdict
 from operator import itemgetter
 from pathlib import Path, PurePath
-from typing import cast, Iterable
+from typing import cast, Iterable, NamedTuple
 
 import rich.repr
 from rich.console import RenderableType, RenderResult, Console, ConsoleOptions
@@ -116,14 +116,26 @@ class StylesheetErrors:
         )
 
 
+class CssSource(NamedTuple):
+    """Contains the CSS content and whether or not the CSS comes from user defined stylesheets
+    vs widget-level stylesheets.
+
+    Args:
+        content (str): The CSS as a string.
+        is_defaults (bool): True if the CSS is default (i.e. that defined at the widget level).
+            False if it's user CSS (which will override the defaults).
+    """
+
+    content: str
+    is_defaults: bool
+
+
 @rich.repr.auto(angular=True)
 class Stylesheet:
     def __init__(self, *, variables: dict[str, str] | None = None) -> None:
         self._rules: list[RuleSet] = []
         self.variables = variables or {}
-        self.source: dict[str, str] = {}
+        self.source: dict[str, CssSource] = {}
-        # Records which of the source keys represent CSS defined at the widget level
-        self._widget_css_paths: set[str] = set()
         self._require_parse = False
 
     def __rich_repr__(self) -> rich.repr.Result:
@@ -149,7 +161,6 @@ class Stylesheet:
         """
         stylesheet = Stylesheet(variables=self.variables.copy())
         stylesheet.source = self.source.copy()
-        stylesheet._widget_css_paths = self._widget_css_paths.copy()
         return stylesheet
 
     def set_variables(self, variables: dict[str, str]) -> None:
@@ -160,12 +171,17 @@ class Stylesheet:
         """
         self.variables = variables
 
-    def _parse_rules(self, css: str, path: str | PurePath) -> list[RuleSet]:
+    def _parse_rules(
+        self, css: str, path: str | PurePath, is_default_rules: bool = False
+    ) -> list[RuleSet]:
         """Parse CSS and return rules.
 
         Args:
+            is_default_rules:
             css (str): String containing Textual CSS.
             path (str | PurePath): Path to CSS or unique identifier
+            is_default_rules (bool): True if the rules we're extracting are
+                default (i.e. in Widget.CSS) rules. False if they're from user defined CSS.
 
         Raises:
             StylesheetError: If the CSS is invalid.
@@ -174,16 +190,19 @@ class Stylesheet:
             list[RuleSet]: List of RuleSets.
         """
         try:
-            rules = list(parse(css, path, variables=self.variables))
+            rules = list(
+                parse(
+                    css,
+                    path,
+                    variables=self.variables,
+                    is_default_rules=is_default_rules,
+                )
+            )
         except TokenizeError:
             raise
         except Exception as error:
             raise StylesheetError(f"failed to parse css; {error}")
 
-        if path in self._widget_css_paths:
-            for rule in rules:
-                rule.is_widget_rule_set = True
-
         return rules
 
     def read(self, filename: str | PurePath) -> None:
@@ -203,11 +222,11 @@ class Stylesheet:
             path = os.path.abspath(filename)
         except Exception as error:
             raise StylesheetError(f"unable to read {filename!r}; {error}")
-        self.source[str(path)] = css
+        self.source[str(path)] = CssSource(content=css, is_defaults=False)
         self._require_parse = True
 
     def add_source(
-        self, css: str, path: str | PurePath | None = None, is_widget_css: bool = False
+        self, css: str, path: str | PurePath | None = None, is_default_css: bool = False
     ) -> None:
         """Parse CSS from a string.
 
@@ -215,7 +234,7 @@ class Stylesheet:
             css (str): String with CSS source.
             path (str | PurePath, optional): The path of the source if a file, or some other identifier.
                 Defaults to None.
-            is_widget_css (bool): True if the CSS is defined in the Widget, False if the CSS is defined
+            is_default_css (bool): True if the CSS is defined in the Widget, False if the CSS is defined
                 in a user stylesheet.
 
         Raises:
@@ -227,16 +246,11 @@ class Stylesheet:
             path = str(hash(css))
         elif isinstance(path, PurePath):
             path = str(css)
-        if path in self.source and self.source[path] == css:
+        if path in self.source and self.source[path].content == css:
             # Path already in source, and CSS is identical
             return
 
-        # Record any CSS defined at the widget level for
+        self.source[path] = CssSource(content=css, is_defaults=is_default_css)
-        # different specificity treatment from user CSS.
-        if is_widget_css:
-            self._widget_css_paths.add(path)
-
-        self.source[path] = css
         self._require_parse = True
 
     def parse(self) -> None:
@@ -247,8 +261,8 @@ class Stylesheet:
         """
         rules: list[RuleSet] = []
         add_rules = rules.extend
-        for path, css in self.source.items():
+        for path, (css, is_default_rules) in self.source.items():
-            css_rules = self._parse_rules(css, path)
+            css_rules = self._parse_rules(css, path, is_default_rules=is_default_rules)
             if any(rule.errors for rule in css_rules):
                 error_renderable = StylesheetErrors(css_rules)
                 raise StylesheetParseError(error_renderable)
@@ -266,8 +280,8 @@ class Stylesheet:
         """
         # Do this in a fresh Stylesheet so if there are errors we don't break self.
         stylesheet = Stylesheet(variables=self.variables)
-        for path, css in self.source.items():
+        for path, (css, is_defaults) in self.source.items():
-            stylesheet.add_source(css, path)
+            stylesheet.add_source(css, path, is_default_css=is_defaults)
         stylesheet.parse()
         self._rules = stylesheet.rules
         self.source = stylesheet.source
@@ -302,10 +316,10 @@ class Stylesheet:
 
         # Collect the rules defined in the stylesheet
         for rule in reversed(self.rules):
-            is_widget_rule = rule.is_widget_rule_set
+            is_default_rules = rule.is_default_rules
             for base_specificity in _check_rule(rule, node):
                 for key, rule_specificity, value in rule.styles.extract_rules(
-                    base_specificity, is_widget_rule
+                    base_specificity, is_default_rules
                 ):
                     rule_attributes[key].append((rule_specificity, value))
 

src/textual/widget.py

@@ -194,7 +194,7 @@ class Widget(DOMNode):
         """
         # Parse the Widget's CSS
         for path, css in self.css:
-            self.app.stylesheet.add_source(css, path=path, is_widget_css=True)
+            self.app.stylesheet.add_source(css, path=path, is_default_css=True)
 
     def get_box_model(
         self, container: Size, viewport: Size, fraction_unit: Fraction

tests/css/test_stylesheet.py

@@ -3,20 +3,18 @@ from typing import Any
 
 import pytest
 
-from tests.utilities.test_app import AppTest
-from textual.app import App, ComposeResult
 from textual.color import Color
 from textual.css._help_renderables import HelpText
-from textual.css.stylesheet import Stylesheet, StylesheetParseError
+from textual.css.stylesheet import Stylesheet, StylesheetParseError, CssSource
 from textual.css.tokenizer import TokenizeError
 from textual.dom import DOMNode
 from textual.geometry import Spacing
 from textual.widget import Widget
 
 
-def _make_stylesheet(css: str) -> Stylesheet:
+def _make_user_stylesheet(css: str) -> Stylesheet:
     stylesheet = Stylesheet()
-    stylesheet.source["test.css"] = css
+    stylesheet.source["test.css"] = CssSource(css, is_defaults=False)
     stylesheet.parse()
     return stylesheet
 
@@ -24,7 +22,7 @@ def _make_stylesheet(css: str) -> Stylesheet:
 def test_stylesheet_apply_highest_specificity_wins():
     """#ids have higher specificity than .classes"""
     css = "#id {color: red;} .class {color: blue;}"
-    stylesheet = _make_stylesheet(css)
+    stylesheet = _make_user_stylesheet(css)
     node = DOMNode(classes="class", id="id")
     stylesheet.apply(node)
 
@@ -33,7 +31,7 @@ def test_stylesheet_apply_highest_specificity_wins():
 
 def test_stylesheet_apply_doesnt_override_defaults():
     css = "#id {color: red;}"
-    stylesheet = _make_stylesheet(css)
+    stylesheet = _make_user_stylesheet(css)
     node = DOMNode(id="id")
     stylesheet.apply(node)
 
@@ -45,7 +43,7 @@ def test_stylesheet_apply_highest_specificity_wins_multiple_classes():
     """When we use two selectors containing only classes, then the selector
     `.b.c` has greater specificity than the selector `.a`"""
     css = ".b.c {background: blue;} .a {background: red; color: lime;}"
-    stylesheet = _make_stylesheet(css)
+    stylesheet = _make_user_stylesheet(css)
     node = DOMNode(classes="a b c")
     stylesheet.apply(node)
 
@@ -58,7 +56,7 @@ def test_stylesheet_many_classes_dont_overrule_id():
     a selector containing multiple classes cannot take priority over even a
     single class."""
     css = "#id {color: red;} .a.b.c.d {color: blue;}"
-    stylesheet = _make_stylesheet(css)
+    stylesheet = _make_user_stylesheet(css)
     node = DOMNode(classes="a b c d", id="id")
     stylesheet.apply(node)
 
@@ -67,7 +65,7 @@ def test_stylesheet_many_classes_dont_overrule_id():
 
 def test_stylesheet_last_rule_wins_when_same_rule_twice_in_one_ruleset():
     css = "#id {color: red; color: blue;}"
-    stylesheet = _make_stylesheet(css)
+    stylesheet = _make_user_stylesheet(css)
     node = DOMNode(id="id")
     stylesheet.apply(node)
 
@@ -76,7 +74,7 @@ def test_stylesheet_last_rule_wins_when_same_rule_twice_in_one_ruleset():
 
 def test_stylesheet_rulesets_merged_for_duplicate_selectors():
     css = "#id {color: red; background: lime;} #id {color:blue;}"
-    stylesheet = _make_stylesheet(css)
+    stylesheet = _make_user_stylesheet(css)
     node = DOMNode(id="id")
     stylesheet.apply(node)
 
@@ -88,7 +86,7 @@ def test_stylesheet_apply_takes_final_rule_in_specificity_clash():
     """.a and .b both contain background and have same specificity, so .b wins
     since it was declared last - the background should be blue."""
     css = ".a {background: red; color: lime;} .b {background: blue;}"
-    stylesheet = _make_stylesheet(css)
+    stylesheet = _make_user_stylesheet(css)
     node = DOMNode(classes="a b", id="c")
     stylesheet.apply(node)
 
@@ -99,7 +97,7 @@ def test_stylesheet_apply_takes_final_rule_in_specificity_clash():
 def test_stylesheet_apply_empty_rulesets():
     """Ensure that we don't crash when working with empty rulesets"""
     css = ".a {} .b {}"
-    stylesheet = _make_stylesheet(css)
+    stylesheet = _make_user_stylesheet(css)
     node = DOMNode(classes="a b")
     stylesheet.apply(node)
 
@@ -113,8 +111,8 @@ def test_stylesheet_apply_user_css_over_widget_css():
     node = MyWidget()
     node.add_class("a")
 
-    stylesheet = _make_stylesheet(user_css)
+    stylesheet = _make_user_stylesheet(user_css)
-    stylesheet.add_source(MyWidget.CSS, "widget.py:MyWidget", is_widget_css=True)
+    stylesheet.add_source(MyWidget.CSS, "widget.py:MyWidget", is_default_css=True)
     stylesheet.apply(node)
 
     # The node is red because user CSS overrides Widget.CSS