Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow pseudo classes in nested tcss #4163

Closed
wants to merge 9 commits into from

Conversation

rodrigogiraoserrao
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao commented Feb 15, 2024

This PR allows the usage of pseudo classes in nested TCSS while doing the best possible to preserve helpful error messages when the dev writes invalid CSS.

Fixes #4039.

Before this PR we couldn't use pseudo-classes inside nested TCSS because in a nested context the colon can be used to separate a rule name from its value OR a selector from a pseudo-class.
This would throw off the tokenizer.

To disambiguate the two, we try to tokenize rule names by only accepting correct rule names and we only parse selectors if, when they are followed by a colon, the thing after the colon is a valid pseudo-class.
This lookahead enables near-perfect disambiguation when the user types their CSS correctly.

However, the user might introduce typos in their CSS and we still want to provide error messages as helpful as possible.
Previously, we could “guess” whether the user was trying to type a pseudo-class or a rule name because there was no context in which both could appear at the same time.
This enabled pretty lenient tokenizing, and then if the token value wasn't valid, we'd issue the error message.

Because we can no longer be that lenient, we provide a final “catch all” rule that will accept situations where a colon is used and we neither tokenize a rule name nor a selector followed by a future pseudo-class.
We handle this bottom rule as a special case and we use the matched contents to try and figure out if the user made a typo in a rule name or a pseudo-class.
If we can't know, we'll just say that the CSS that was typed is invalid.

The screenshot below shows examples of the error messages we show with different CSS sets:

Screenshot 2024-02-29 at 17 49 49

Each terminal shows some CSS pasted and then the error produced by Textual when tokenizing said CSS.

Demo app below. Just change the widget W? being yielded from within the app.

from textual.app import App, ComposeResult
from textual.widget import Widget

class W1(Widget):
    DEFAULT_CSS = """
    Screen {
        Button {
            color: red;
        }
        Label:bananas {
            background: red;
        }
    }
    """

class W2(Widget):
    DEFAULT_CSS = """
    Screen {
        Button {
            color: red;
        }
        Label:hover {
            backgound: red;
        }
    }
    """

class W3(Widget):
    DEFAULT_CSS = """
    Screen {
        Button {
            color: red;
        }
        baground: red;
    }
    """

class W4(Widget):
    DEFAULT_CSS = """
    Screen:bananas {
        Button {
            color: red;
        }
        background: red;
    }
    """

class W5(Widget):
    DEFAULT_CSS = """
    * {
        Button {
            color: red;
        }
        border: blue;
        backgroundu: red;
    }
    """

class W6(Widget):
    DEFAULT_CSS = """
    Screen {
        Button:hover {
            background: red;
        }
        bacgound: red;
    }
    """

class NestedCSSTokenErrorApp(App[None]):
    def compose(self) -> ComposeResult:
        yield W1()

if __name__ == "__main__":
    NestedCSSTokenErrorApp().run()

'rule: value' and 'selector:pseudo-class' look too similar to the tokenizer.
@rodrigogiraoserrao rodrigogiraoserrao marked this pull request as ready for review February 15, 2024 16:22
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this impact error messages at all?

i.e. will the following generate a helpful "did you mean" error?

Screen {
    Label:locus {
        border red;
    }
}

@rodrigogiraoserrao
Copy link
Contributor Author

rodrigogiraoserrao commented Feb 15, 2024

@willmcgugan that does not generate a helpful “did you mean” error.
It didn't before the PR and still doesn't after the PR.

You get that error because of the ambiguity of this situation. The tokenizer has to try the "rule:value" path first and because "locus" is not a valid pseudoclass, the negative lookahead won't prevent it from thinking it's a declaration.
It will then be mad at you when it finds the {.

@rodrigogiraoserrao
Copy link
Contributor Author

P.S. it will if you add the nested & because that removes the ambiguity, which is pretty much what you mentioned when we talked earlier.

@willmcgugan
Copy link
Collaborator

willmcgugan commented Feb 15, 2024

It will give an error if its not nested.

Screenshot 2024-02-15 at 16 58 25

What error does your change give you with this CSS ?

Screen {
    Label:locus {
        border red;
    }
}

@rodrigogiraoserrao rodrigogiraoserrao changed the title WIP: Allow pseudo classes in nested tcss Allow pseudo classes in nested tcss Feb 15, 2024
@rodrigogiraoserrao
Copy link
Contributor Author

This is the error you get before and after the PR. 👇
I know we are able to generate a more helpful “did you mean” error when the TCSS is not nested.

I can't add the helpful error message in this case without either:

  • changing the tokenizer significantly to remove the ambiguity of parsing this nested TCSS; or
  • breaking helpful error messages for rule names (e.g., when you do baground: red it asks if you meant background: red).

Screenshot 2024-02-19 at 10 25 02

@willmcgugan
Copy link
Collaborator

willmcgugan commented Feb 27, 2024

I think we can do better. I think we can resolve the ambiguity while maintaining helpful error messages.

We might be able to exploit the fact that there are a limited set of rule names. i.e. in the case above Label: is clearly not a declaration because there is no Label rule.

Up for the challenge? @rodrigogiraoserrao

Before this commit we couldn't use pseudo-classes inside nested TCSS because in a nested context the colon can be used to separate a rule name from its value OR a selector from a pseudo-class.
To disambiguate the two, we try to tokenize rule names by only accepting correct rule names and we only parse selectors if, when they are followed by a colon, the thing after the colon is a valid pseudo-class.
This lookahead enables near-perfect disambiguation when the user types their CSS correctly.
However, the user might introduce typos in their CSS and we still want to provide error messages as helpful as possible.
Previously, we could “guess” whether the user was trying to type a pseudo-class or a rule name because there was no context in which both could appear at the same time.
This enabled pretty lenient tokenizing, and then if the token value wasn't valid, we'd issue the error message.
Because we can no longer be that lenient, we provide a final “catch all” rule that will accept situations where a colon is used and we neither tokenize a rule name nor a selector followed by a future pseudo-class.
We handle this bottom rule as a special case and we use the matched contents to try and figure out if the user made a typo in a rule name or a pseudo-class.
If we can't know, we'll just say that the CSS that was typed is invalid.
@rodrigogiraoserrao
Copy link
Contributor Author

@willmcgugan I've updated the top comment to describe what I'm doing and show some of the error messages we get. Images 2-6 are probably what you expect and image 1 (top-left) is probably the most unexpected one.

On the one hand, for a Human reading it it's “obvious” that the dev was trying to define a nested selector + pseudo-class, even though “bananas” is a ridiculous pseudo-class.
On the other hand, I don't see how to determine this with regex.

I thought of looking at the end of the line:

  • if the line ends with { it's probably a selector + pseudo-class
  • if the line ends with ; it's probably a rule + value

But then I'd also need to check for comments (multi-line and single line), check if there were loads of CSS inlined or not, etc.
This feels like an impossible task for regex.

@willmcgugan
Copy link
Collaborator

I think we can do better than "Invalid CSS".

What if we were to simply mandate that types begin with a capital letter? If that were the case, there would be no ambiguity with a declaration versus a selector.

@rodrigogiraoserrao
Copy link
Contributor Author

I'm assuming all future declarations would then start with a lower-case letter.
That looks promising. Should I go down that route?

@willmcgugan
Copy link
Collaborator

Declarations will always be lower case. Go for it!

rodrigogiraoserrao added a commit that referenced this pull request Mar 4, 2024
To be able to disambiguate between selector:pseudo-class and declaration:rule-value in nested TCSS (see #4039 for the original issue and #4163 for a first attempt at solving this) we establish that selectors with widget type names always start with an upper case letter A-Z or an underscore _ whereas declarations always start with a lower case letter a-z.
When a user creates a widget subclass that doesn't conform to this, we issue a 'SyntaxWarning' to let the user know.
Because we do this with the standard module 'warnings', the warning can easily be supressed if the user has a good reason to create a widget subclass with a name starting with a lower case letter (which is valid Python, just unhelpful to Textual).
@rodrigogiraoserrao rodrigogiraoserrao self-assigned this Mar 4, 2024
@rodrigogiraoserrao
Copy link
Contributor Author

rodrigogiraoserrao commented Mar 4, 2024

Superseded by #4252 given that the trade-off suggested by Will simplifies the implementation.
I created a new PR because I don't think there's any reason to leave all these commits and reverts in the history of the PR that gets merged.

@rodrigogiraoserrao rodrigogiraoserrao deleted the pseudo-classes-in-nested-tcss branch March 4, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested TCSS interacts poorly with pseudo-classes in selectors.
2 participants