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/more invalid selector fixes #410

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

raxbg
Copy link
Contributor

@raxbg raxbg commented Feb 27, 2023

Pseudo classes cannot have a space after the : sign. We should consider such occurrences invalid.

@@ -77,7 +77,7 @@ class Selector
*/
public static function isValid($sSelector)
{
return preg_match(static::SELECTOR_VALIDATION_RX, $sSelector);
return preg_match(static::SELECTOR_VALIDATION_RX, $sSelector) && strpos($sSelector, ': ') === false;
Copy link
Contributor

@sabberworm sabberworm Mar 1, 2023

Choose a reason for hiding this comment

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

I guess this check is probably too simplistic since it will fail some valid selectors, e.g.:

  • a[href*=": "]
  • #some_id_that_ends_in_a\: > a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! These should be fixed now :)

@oliverklee oliverklee deleted the branch MyIntervals:main February 7, 2024 11:36
@oliverklee oliverklee closed this Feb 7, 2024
@oliverklee oliverklee reopened this Feb 7, 2024
@oliverklee oliverklee changed the base branch from master to main February 7, 2024 22:34
Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

Hi @raxbg,

Thanks for the suggestion.

However, moving forwards, we are looking to isolate core functionality (parsing) from additional CSS debugging features (#463).

I'd like to keep this open, so it's captured, but for now I think it's blocked by #461/#462/#463.

@JakeQZ JakeQZ added future enhancement blocked Needs another issue to be resolved first labels Feb 26, 2024
@JakeQZ JakeQZ added this to the 10.0.0 milestone Feb 26, 2024
@raxbg raxbg force-pushed the fix/more-invalid-selector-fixes branch from 8d364f3 to d43033b Compare July 10, 2024 06:42
@raxbg
Copy link
Contributor Author

raxbg commented Jul 10, 2024

Hi @raxbg,

Thanks for the suggestion.

However, moving forwards, we are looking to isolate core functionality (parsing) from additional CSS debugging features (#463).

I'd like to keep this open, so it's captured, but for now I think it's blocked by #461/#462/#463.

The invalid selectors shouldn't be included in the parsed tree I think. Take a look at this fiddle comparing how the browser parses the invalid cases: https://jsfiddle.net/sm67z4nb/

They do not seem to be included in the final tree.

I like the idea of the debugging tools btw :)

@JakeQZ
Copy link
Contributor

JakeQZ commented Jul 10, 2024

The invalid selectors shouldn't be included in the parsed tree I think. Take a look at this fiddle comparing how the browser parses the invalid cases: https://jsfiddle.net/sm67z4nb/

You're right that if there's something in any of a possibly comma-separated list of selectors the browser doesn't recognize, the entire rule is dropped.

For example, before ::placeholder was standardized, I had to do this:

.example input::-webkit-input-placeholder {
  /* lots or property declarations */
}
.example input::-ms-input-placeholder {
  /* the same property declarations repeated again */
}
.example input:-moz--placeholder {
  /* the same property declarations repeated again */
}
.example input::-moz--placeholder {
  /* the same property declarations repeated again */
}

It was not possible to put the selectors in a comma-separated list, because each browser would barf upon encountering a rival's vendor prefix.

Obviously the parser shouldn't be so prejudiced.

But if there is a selector consutruct which is invalid for all browsers, it will never work, and perhaps should be dropped. Either way, such situation would be an opportune use for the logger (TBI).

@sabberworm, @oliverklee, WDYT - should we drop rules with selector constructs that appear to be invalid?

We would need to be careful not to drop actually-valid constructs.

@oliverklee
Copy link
Contributor

Yes, let's drop them. To quote from https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_syntax/Error_handling:

When a CSS error is encountered, the browser's parser ignores the line containing the errors, discarding the minimum amount of CSS code before returning to parsing the CSS as normal. The "error recovery" is just the ignoring or skipping of invalid content.

@sabberworm
Copy link
Contributor

It was not possible to put the selectors in a comma-separated list, because each browser would barf upon encountering a rival's vendor prefix.

Take care not to confuse syntactically-invalid rules with rules that are syntactically valid but a specific browser doesn’t know the semantics of. div: hover is an example of the former (and it would be OK to warn and drop that) where input::-webkit-input-placeholder would be the latter for non-webkit browsers (and we should never drop such a rule).

However, to complicate matters, we also have some syntactically-invalid stuff that we do parse because they were wildly-deployed workarounds of browser bugs.

IMHO, we should validate selectors only once we actually parse them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Needs another issue to be resolved first enhancement future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants