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

Fix CSS pseudo-elements not recognized issue #10501

Closed
wants to merge 5 commits into from

Conversation

donho
Copy link
Member

@donho donho commented Sep 4, 2021

Update CSS keywords and add new styles.

Fix #10425

Update CSS keywords and add new styles.

Fix notepad-plus-plus#10425
@@ -342,6 +342,17 @@
<WordsStyle name="ID" styleID="10" fgColor="0080FF" bgColor="FFFFFF" fontName="" fontStyle="1" fontSize="" />
<WordsStyle name="IMPORTANT" styleID="11" fgColor="FF0000" bgColor="FFFFFF" fontName="" fontStyle="1" fontSize="" />
<WordsStyle name="DIRECTIVE" styleID="12" fgColor="0080FF" bgColor="FFFFFF" fontName="" fontStyle="0" fontSize="" />
<WordsStyle name="DOUBLESTRING" styleID="13" fgColor="808080" bgColor="FFFFFF" fontName="" fontStyle="0" fontSize="" />
<WordsStyle name="SINGLESTRING" styleID="14" fgColor="808080" bgColor="FFFFFF" fontName="" fontStyle="0" fontSize="" />
<WordsStyle name="IDENTIFIER2" styleID="15" fgColor="0040E0" bgColor="FFFFFF" fontName="" fontStyle="0" fontSize="" />
Copy link
Contributor

Choose a reason for hiding this comment

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

@donho
IDENTIFIER2, IDENTIFIER3, EXTENDED_IDENTIFIER, EXTENDED_PSEUDOCLASS, EXTENDED_PSEUDOELEMENT will not work without coresponding list for them: LIST_2, LIST_3, LIST_5, LIST_6,, LIST_7 setting by setLexer() as above.
https://github.com/notepad-plus-plus/notepad-plus-plus/blob/master/scintilla/lexers/LexCSS.cxx#L75

Could be listed all of them in this fix, then in future it will be easier to add something in the langs.model.xml itself, without asking you to add some missing list again in ScintillaEditView.h.

Copy link
Member Author

Choose a reason for hiding this comment

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

For CSS2 & CSS3 (IDENTIFIER2 & IDENTIFIER3 respectively) are in current existing list, so I don't think we will add (separate) them in the future. Anyway, I will comment all not used keyword lists for the eventual future added.
Thank you for your review.

@ArkadiuszMichalski
Copy link
Contributor

ArkadiuszMichalski commented Sep 4, 2021

@donho
What about with this old notation : for pseudoelement? Should be recognized as pseudoclass because after before first-letter first-line are still listed in pseudoclasses (instre2)? Also selection was listed, but these can be removed.

@donho
Copy link
Member Author

donho commented Sep 4, 2021

@ArkadiuszMichalski
I removed after from instre2 (by keeping after in type3), here's the result:

image

@ArkadiuszMichalski
Copy link
Contributor

And it's correct, because :after is a pseudo-element (in old notation) and shouldn't use the pseudo-class colors, now it just has a color from UNKNOWN_PSEUDOCLASS. New notation ::after use PSEUDOELEMENT style.

Like I wrote here #10425 (comment), there is a trick to handle :after with the same colors as ::after, without changing the lexer. An additional list EXTENDED_PSEUDOCLASS (LIST_6) can be used.

setLexer(SCLEX_CSS, L_CSS, LIST_0 | LIST_1 | LIST_4 | LIST_6);

<Keywords name="type5">after before first-letter first-line</Keywords>

<WordsStyle name="PSEUDOELEMENT" styleID="18" fgColor="FF8000" bgColor="FFFFFF" fontName="" fontStyle="0" fontSize="" />
<WordsStyle name="EXTENDED_PSEUDOCLASS" styleID="20" fgColor="FF8000" bgColor="FFFFFF" fontName="" fontStyle="0" fontSize="" />

Now bot :after and ::after will use same style. Unfortunately, the style configurator will show EXTENDED_PSEUDOCLASS, which can be confusing for others (because it will cover these 4 pseudoelement cases and not everyone has to know about it).

@donho
Copy link
Member Author

donho commented Sep 4, 2021

You have mentioned in #10425 (comment) :

@donho
:after is also pseudoelement (notation with : comes from old specs). In new specs only :: should be used for pseudoelement (browsers still support old notation for compatibility reasons).

So I suppose :after should be recognized. Now my question is: if :after (and the other :*) is (are) valid in the most of browsers, should we make it valid as well ?

@ArkadiuszMichalski
Copy link
Contributor

ArkadiuszMichalski commented Sep 4, 2021

So I suppose :after should be recognized.

It would be most appropriate, but the lexer does not understand it now (it can only consider them as pseudo-classes or use abve trick).

Now my question is: if :after (and the other :*) is (are) valid in the most of browsers, should we make it valid as well?

Yes, for sure these 4 old ones are valid in the browser (here nobody wants to break backwards compatibility), I did not check the rest (because it is possible that all pseudo-elements with ":" are supported in browsers). I'll check it out later.

should we make it valid as well?

It would be most appropriate, then the question is how? Some options:

  • recognize them as pseudoclass
  • not recognize so treat as UNKNOWN_PSEUDOCLASS
  • use EXTENDED_PSEUDOCLASS list for them and use same style as for pseudoelement
  • open issue in Lexilla to recognize this old notation

Edit: I checked this and browsers recognize only this 4 pseudoelement with old notation (after before first-letter first-line). All others works only with new notation :: .

I would also add marker to the <Keywords name="type3"> because browsers also support it widely.

@donho
Copy link
Member Author

donho commented Sep 4, 2021

OK, I'll do the following:

  • after before first-letter first-line will added into LIST_6 (EXTENDED_PSEUDOCLASS 20)
  • marker will be added into the "type3" Keywords

Which keywords should I remove from PsudoClass besides of after before first-letter first-line ?

@ArkadiuszMichalski
Copy link
Contributor

Which keywords should I remove from PsudoClass besides of after before first-letter first-line ?

also selection

@donho
Copy link
Member Author

donho commented Sep 4, 2021

BTW, since :after :before :first-letter :first-line are psudo-element, why don't we add them rather into EXTENDED_PSEUDOELEMENT (21) ?

@ArkadiuszMichalski
Copy link
Contributor

ArkadiuszMichalski commented Sep 4, 2021

PSEUDOELEMENT and EXTENDED_PSEUDOELEMENT (21) and thieir coresponding list recognize only "::" notation, we need ":" so EXTENDED_PSEUDOCLASS may be the solution here (we don't use this list anyway).

You mean this?

<WordsStyle name="EXTENDED_PSEUDOELEMENT" styleID="21" fgColor="FF8000" bgColor="FFFFFF" fontName="" fontStyle="0" fontSize="" keywordClass="type5" />

This will work, can we point to any list using keywordClass attr? If so, it would be better.

Edit: no, it will not work. keywordClass only points to the list to be visible in the Style Configurator. To recognize ":" and stylize it as a pseudo-element it is necessary to use EXTENDED_PSEUDOCLASS (20).

Test:

:active{ /* pseudoclass */
	
}

::after{ /* pseudoelement */

}

::placeholder{ /* pseudoelement */
	
}

:after{ /* pseudoelement */

}

Efect (I changed the colors to make them different):
image

@ArkadiuszMichalski
Copy link
Contributor

ArkadiuszMichalski commented Sep 4, 2021

@donho

But we can do this:

<WordsStyle name="LEGACY_PSEUDOELEMENT" styleID="20" fgColor="FF8000" bgColor="FFFFFF" fontName="" fontStyle="0" fontSize="" keywordClass="type5" />  <!-- Original name is EXTENDED_PSEUDOCLASS -->

We can use any name="" so in Style Configurator won't be any confusing. And don't forget to add marker #10501 (comment).

image

Copy link
Contributor

@ArkadiuszMichalski ArkadiuszMichalski left a comment

Choose a reason for hiding this comment

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

Check last comment #10501 (comment).

<!--WordsStyle name="IDENTIFIER3" styleID="17" fgColor="00A0E0" bgColor="3F3F3F" fontName="" fontStyle="0" fontSize="" /-->
<WordsStyle name="PSEUDOELEMENT" styleID="18" fgColor="CEDF99" bgColor="3F3F3F" fontName="" fontStyle="0" fontSize="" keywordClass="type3"/>
<!--WordsStyle name="EXTENDED_IDENTIFIER" styleID="19" fgColor="7F7F00" bgColor="3F3F3F" fontName="" fontStyle="0" fontSize="" /-->
<WordsStyle name="EXTENDED_PSEUDOCLASS" styleID="20" fgColor="CEDF99" bgColor="3F3F3F" fontName="" fontStyle="0" fontSize="" keywordClass="type5" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Make analogous correct as in stylers.model.xml.

<Keywords name="instre2">active after before check checked disabled empty enabled first first-child first-letter first-line first-of-type focus hover indeterminate invalid lang last-child last-of-type left link not nth-child nth-last-child nth-of-type nth-last-of-type only-child only-of-type optional read-only read-write required right root selection target valid visited</Keywords>
</Language>
<Keywords name="instre2">active check checked disabled empty enabled first first-child first-of-type focus hover indeterminate invalid lang last-child last-of-type left link not nth-child nth-last-child nth-of-type nth-last-of-type only-child only-of-type optional read-only read-write required right root target valid visited</Keywords>
<Keywords name="type3">after before first-letter first-line selection cue file-selector-button placeholder slotted part</Keywords>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add marker here.

@donho donho closed this in cddcbb5 Sep 5, 2021
@donho donho deleted the update_css branch September 5, 2021 14:45
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.

CSS pseudo-elements not recognized
2 participants