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

[bug] CSS selector issue -moz-drag-over nonstandard Pseudo-classes #3193

Closed
stoivo opened this issue May 15, 2024 · 16 comments · Fixed by #3197
Closed

[bug] CSS selector issue -moz-drag-over nonstandard Pseudo-classes #3193

stoivo opened this issue May 15, 2024 · 16 comments · Fixed by #3197
Labels
state/needs-triage Inbox for non-installation-related bug reports or help requests

Comments

@stoivo
Copy link

stoivo commented May 15, 2024

I am getting warings when I use roadie when I want inline styles. It loops over the stylessheet and inlines the styles for each selector. It also inlines the styles from normalize.css

button:-moz-focusring,
[type="button"]:-moz-focusring,
[type="reset"]:-moz-focusring,
[type="submit"]:-moz-focusring {
  outline: 1px dotted ButtonText;
}

I created a little sample script to demonstrate the issue.

require 'nokogiri'
doc = Nokogiri.XML('<foo></foo>')

p doc.send(:css_rules_to_xpath, ['a:hover'], {})
p doc.css('a:hover')

p doc.send(:css_rules_to_xpath, ['div:empty'], {})
p doc.css('div:empty')

p doc.send(:css_rules_to_xpath, ['a:a-moz-drag-over'], {})
p doc.css('a:a-moz-drag-over')

p doc.send(:css_rules_to_xpath, ['a:-moz-drag-over'], {})
p doc.css('a:-moz-drag-over')
output["//a[nokogiri:hover(.)]"] [] ["//div[not(node())]"] [] ["//a[nokogiri:a-moz-drag-over(.)]"] [] ["//a[nokogiri:-moz-drag-over(.)]"] /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/nokogiri-1.16.5-arm64-darwin/lib/nokogiri/xml/searchable.rb:238:in `evaluate': ERROR: Invalid expression: //a[nokogiri:-moz-drag-over(.)] (Nokogiri::XML::XPath::SyntaxError) from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/nokogiri-1.16.5-arm64-darwin/lib/nokogiri/xml/searchable.rb:238:in `xpath_impl' from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/nokogiri-1.16.5-arm64-darwin/lib/nokogiri/xml/searchable.rb:219:in `xpath_internal' from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/nokogiri-1.16.5-arm64-darwin/lib/nokogiri/xml/searchable.rb:211:in `css_internal' from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/nokogiri-1.16.5-arm64-darwin/lib/nokogiri/xml/searchable.rb:132:in `css' from /Users/simon/dev/work/dodo/fakturabank/repo/bb.rb:28:in `<main>' from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/railties-7.0.8/lib/rails/commands/runner/runner_command.rb:43:in `load' from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/railties-7.0.8/lib/rails/commands/runner/runner_command.rb:43:in `perform' from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/thor-1.2.2/lib/thor/command.rb:27:in `run' from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/thor-1.2.2/lib/thor/invocation.rb:127:in `invoke_command' from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/thor-1.2.2/lib/thor.rb:392:in `dispatch' from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/railties-7.0.8/lib/rails/command/base.rb:87:in `perform' from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/railties-7.0.8/lib/rails/command.rb:48:in `invoke' from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/railties-7.0.8/lib/rails/commands.rb:18:in `<main>' from <internal:/Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/site_ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:38:in `require' from <internal:/Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/site_ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:38:in `require' from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/bootsnap-1.16.0/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:32:in `require' from bin/rails:4:in `<main>'

As you can see it don't crash for a-moz-drag-over which is an unknown pseudo class. Maybe it crashed because it starts on a -. Is this something we can fix or should I try to fix it on the outside?

@stoivo stoivo added the state/needs-triage Inbox for non-installation-related bug reports or help requests label May 15, 2024
@flavorjones
Copy link
Member

@stoivo Thank you for opening this issue!

This error is coming from libxml2 (and not from Nokogiri), but let me investigate and see if we can work around it.

@flavorjones
Copy link
Member

flavorjones commented May 17, 2024

@stoivo Can you tell me more about your use case? I'm not familiar with roadie.

The :-moz-drag-over pseudoclass is not something that can be implemented without a browser, and so will never match anything. Why are you (or why is roadie) doing a search on the document that you know ahead of time will not return any matches?

I just want to make sure I understand the use case before getting too far into solving this.

@stoivo
Copy link
Author

stoivo commented May 20, 2024

yeah, roadie help to inline styles for mails, since that recommended way to using css in mails.

We take application.css and inlines those rules. It's done by parsing the css stylesheet and looping over css rule by css rule and searching for the element by css. in our application.css we have included normalize.css which has -moz-focusring rules. We could solve this in roadi too by not searching if the rule ends with -moz-focusring

@flavorjones
Copy link
Member

flavorjones commented May 20, 2024

@stoivo Thanks for giving additional context.

I read through the relevant portions of the XPath query spec. For context, pseudo-classes are implemented as function calls in Nokogir's CSS-to-XPath translator, and libxml2 has implemented the XPath's spec for parsing function names properly.

Unfortunately, in XPath, function names are required to start with a NameStartChar which does not include the - character. There's nothing we can easily do to change that, and I don't think it's beneficial enough to warrant the work at this time.

For now, your best bet is to rescue Nokogiri::XML::XPath::SyntaxError and skip to the next search. Sorry I couldn't be more helpful!

@flavorjones flavorjones closed this as not planned Won't fix, can't repro, duplicate, stale May 20, 2024
@flavorjones flavorjones reopened this May 20, 2024
@flavorjones
Copy link
Member

Hmm, actually, please tell me if there's a way Nokogiri can signal this type of error to users earlier that would be helpful for you?

It might make sense to have the XPath translation raise an error (and not the search). Would that be helpful?

flavorjones added a commit that referenced this issue May 20, 2024
Some pseudo-classes cannot be converted into an XPath function name,
and libxml2 will raise an Nokogiri::XML::XPath::SyntaxError at
query-time:

    nokogiri/xml/searchable.rb:238:in `evaluate': ERROR: Invalid expression: //*:div[nokogiri:-moz-drag-over(.)] (Nokogiri::XML::XPath::SyntaxError)

This change moves the error from query-time to parse-time, in the
hopes that this is more rescuable (and the error is more descriptive):

    nokogiri/css/parser_extras.rb:86:in `on_error': unexpected '-' after ':' (Nokogiri::CSS::SyntaxError)

Closes #3193
@flavorjones
Copy link
Member

@stoivo I'm curious how the branch at #3197 works for you?

@stoivo
Copy link
Author

stoivo commented May 21, 2024

I didn't test the branch. It seam nice to have a more descriptive error

We have css sheets with selectors which are invalid like div:not(.one which we want to warn the user about. This selector also raises Nokogiri::CSS::SyntaxError which is good. I think it fine that we add a small check to do nothing if the function starts with -.

I did a bit more testing and it seam like the CSS parser don't like "div:not(.one,.two)"

parser = Nokogiri::CSS::Parser.new
pp parser.parse("div:not(.one,.two)")

__END__
/Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/nokogiri-1.16.5-arm64-darwin/lib/nokogiri/css/parser_extras.rb:86:in `on_error': unexpected ',' after '#<Nokogiri::CSS::Node:0x0000000117852470>' (Nokogiri::CSS::SyntaxError)
  from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/racc-1.7.3/lib/racc/parser.rb:267:in `_racc_do_parse_c'
  from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/racc-1.7.3/lib/racc/parser.rb:267:in `do_parse'
  from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/nokogiri-1.16.5-arm64-darwin/lib/nokogiri/css/parser_extras.rb:68:in `parse'
  from /Users/simon/dev/work/dodo/fakturabank/repo/bb.rb:36:in `<main>'
  from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/railties-7.0.8/lib/rails/commands/runner/runner_command.rb:43:in `load'
  from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/railties-7.0.8/lib/rails/commands/runner/runner_command.rb:43:in `perform'
  from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/thor-1.2.2/lib/thor/command.rb:27:in `run'
  from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/thor-1.2.2/lib/thor/invocation.rb:127:in `invoke_command'
  from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/thor-1.2.2/lib/thor.rb:392:in `dispatch'
  from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/railties-7.0.8/lib/rails/command/base.rb:87:in `perform'
  from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/railties-7.0.8/lib/rails/command.rb:48:in `invoke'
  from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/railties-7.0.8/lib/rails/commands.rb:18:in `<main>'
  from <internal:/Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/site_ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:38:in `require'
  from <internal:/Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/site_ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:38:in `require'
  from /Users/simon/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/bootsnap-1.16.0/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:32:in `require'
  from bin/rails:4:in `<main>'

@flavorjones
Copy link
Member

flavorjones commented May 24, 2024

I did a bit more testing and it seam like the CSS parser don't like "div:not(.one,.two)"

Sorry, is this comment related to the original issue or the pull request fixing it? If you've found another bug, please open a new issue.

I would really appreciate you trying this branch out. I'm trying to be responsive to the issue you filed.

flavorjones added a commit that referenced this issue May 24, 2024
Some pseudo-classes cannot be converted into an XPath function name,
and libxml2 will raise an Nokogiri::XML::XPath::SyntaxError at
query-time:

    nokogiri/xml/searchable.rb:238:in `evaluate': ERROR: Invalid expression: //*:div[nokogiri:-moz-drag-over(.)] (Nokogiri::XML::XPath::SyntaxError)

This change moves the error from query-time to parse-time, in the
hopes that this is more rescuable (and the error is more descriptive):

    nokogiri/css/parser_extras.rb:86:in `on_error': unexpected '-' after ':' (Nokogiri::CSS::SyntaxError)

Closes #3193
@flavorjones
Copy link
Member

I've created a new issue for the multiple-selector issue you described: #3207

flavorjones added a commit that referenced this issue May 24, 2024
…ption at query parse time (#3197)

**What problem is this PR intended to solve?**

fix: raise CSS::SyntaxError if a pseudo-class is not an XPath Name

Some pseudo-classes cannot be converted into an XPath function name, and
libxml2 will raise an Nokogiri::XML::XPath::SyntaxError at query-time:

```
nokogiri/xml/searchable.rb:238:in `evaluate': ERROR: Invalid expression: //*:div[nokogiri:-moz-drag-over(.)] (Nokogiri::XML::XPath::SyntaxError)
```

This change moves the error from query-time to parse-time, in the hopes
that this is more rescuable (and the error is more descriptive):

```
nokogiri/css/parser_extras.rb:86:in `on_error': unexpected '-' after ':' (Nokogiri::CSS::SyntaxError)
```

Closes #3193


**Have you included adequate test coverage?**

Yes.


**Does this change affect the behavior of either the C or the Java
implementations?**

N/A
@stoivo
Copy link
Author

stoivo commented May 27, 2024

I wasn't sure if this issue was related or not. The issue you created explains the issue very well.

I will test your branch now

@stoivo
Copy link
Author

stoivo commented May 27, 2024

I tested your branch and I think it's an improvement.

I also wonder if Nokogiri::CSS::SyntaxError is misleading since the CSS selector is valid. The error is that this CSS selector can't be converted into an XPath selector. Would it be possible to have a separate exception for that without too much work?

@flavorjones
Copy link
Member

@stoivo I don't have any objections to creating a new subclass of Nokogiri::CSS:SyntaxError for this. Are you up for creating a pull request for it?

@stoivo
Copy link
Author

stoivo commented May 28, 2024

Sounds like a fun challenge but I would need some guidance on where to start. Also I have never worked with racc before but I can learn it.

@flavorjones
Copy link
Member

@stoivo No need to learn rexical or racc ... it's just Ruby code. The on_error handler for the parser is here: https://github.com/sparklemotion/nokogiri/blob/main/lib/nokogiri/css/parser_extras.rb#L84-L87

flavorjones added a commit that referenced this issue Jun 11, 2024
This is an alternative to #3197 (which was reverted) in which the
exception is raised from the XPathVisitor and not the CSS
Parser.

Semantically, this is valid CSS, and so the Parser shouldn't
raise. But it is invalid XPath, and so it's the responsibility of the
Visitor to raise.

Closes #3193.
flavorjones added a commit that referenced this issue Jun 11, 2024
**What problem is this PR intended to solve?**

After some thought, I decided I didn't like the solution to #3193
introduced in #3197, in which the exception was raised by the CSS
parser, because it's an XPath translation problem and not a CSS problem.

This PR reverts that change, and instead raises the exception from the
XPathVisitor class when it tries to translate the pseudo-class selector
or function into an XPath function call.


**Have you included adequate test coverage?**

Yes.


**Does this change affect the behavior of either the C or the Java
implementations?**

N/A
@flavorjones
Copy link
Member

note I reverted #3197 and instead implemented #3223

@stoivo
Copy link
Author

stoivo commented Jun 12, 2024

The new code looks nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state/needs-triage Inbox for non-installation-related bug reports or help requests
Projects
None yet
2 participants