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

stoivo · 2024-05-15T10:56:28Z

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?

The text was updated successfully, but these errors were encountered:

flavorjones · 2024-05-16T14:47:46Z

@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 · 2024-05-17T15:05:54Z

@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 · 2024-05-20T08:07:22Z

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 · 2024-05-20T19:08:44Z

@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 · 2024-05-20T19:10:57Z

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?

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 · 2024-05-20T20:42:39Z

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

stoivo · 2024-05-21T07:31:34Z

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 · 2024-05-24T20:13:42Z

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.

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 · 2024-05-24T20:34:16Z

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

…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 · 2024-05-27T13:43:58Z

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 · 2024-05-27T13:56:36Z

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 · 2024-05-27T14:14:38Z

@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 · 2024-05-28T06:51:42Z

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 · 2024-05-28T13:06:49Z

@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

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.

**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 · 2024-06-11T14:42:14Z

note I reverted #3197 and instead implemented #3223

stoivo · 2024-06-12T07:51:12Z

The new code looks nice

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

flavorjones closed this as not planned Won't fix, can't repro, duplicate, stale May 20, 2024

flavorjones reopened this May 20, 2024

flavorjones mentioned this issue May 20, 2024

fix: CSS queries with xpath-incompatible pseudo-classes raise an exception at query parse time #3197

Merged

flavorjones mentioned this issue May 24, 2024

[Nokogiri should accept multiple selectors as arguments to the :not CSS pseudo-class #3207

Open

flavorjones closed this as completed in #3197 May 24, 2024

flavorjones mentioned this issue Jun 11, 2024

fix: css pseudo-class error raised by XPathVisitor #3223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stoivo commented May 15, 2024 •

edited

Loading

flavorjones commented May 16, 2024

flavorjones commented May 17, 2024 •

edited

Loading

stoivo commented May 20, 2024

flavorjones commented May 20, 2024 •

edited

Loading

flavorjones commented May 20, 2024

flavorjones commented May 20, 2024

stoivo commented May 21, 2024

flavorjones commented May 24, 2024 •

edited

Loading

flavorjones commented May 24, 2024

stoivo commented May 27, 2024

stoivo commented May 27, 2024 •

edited

Loading

flavorjones commented May 27, 2024

stoivo commented May 28, 2024

flavorjones commented May 28, 2024

flavorjones commented Jun 11, 2024

stoivo commented Jun 12, 2024

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

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

Comments

stoivo commented May 15, 2024 • edited Loading

flavorjones commented May 16, 2024

flavorjones commented May 17, 2024 • edited Loading

stoivo commented May 20, 2024

flavorjones commented May 20, 2024 • edited Loading

flavorjones commented May 20, 2024

flavorjones commented May 20, 2024

stoivo commented May 21, 2024

flavorjones commented May 24, 2024 • edited Loading

flavorjones commented May 24, 2024

stoivo commented May 27, 2024

stoivo commented May 27, 2024 • edited Loading

flavorjones commented May 27, 2024

stoivo commented May 28, 2024

flavorjones commented May 28, 2024

flavorjones commented Jun 11, 2024

stoivo commented Jun 12, 2024

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

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

stoivo commented May 15, 2024 •

edited

Loading

flavorjones commented May 17, 2024 •

edited

Loading

flavorjones commented May 20, 2024 •

edited

Loading

flavorjones commented May 24, 2024 •

edited

Loading

stoivo commented May 27, 2024 •

edited

Loading