[EdJoPaTo]

Also, ensure that scripting_enabled is the same for parsing and serializing. If it's not (like before this commit) it escapes the content which results in broken noscript contents.

My first issue was that content of noscript was "destroyed" (= needlessly escaped) when I used scraper for parsing, then changing something unrelated and then serializing it back to an HTML string (basically Html::parse_document(input).html()), but I didn't understand why.

-<noscript><h1>Hello, world!</h1></noscript>
+<noscript>&lt;h1&gt;Hello, world!&lt;/h1&gt;</noscript>

And it gets worse when it's done multiple times in a row on the output of the run before.

Turns out, html5ever uses true as a Default for scripting_enabled for both parsing and serializing. Currently, scraper overrides this default for serializing but not for parsing which breaks the output. They need to be the same for parsing and serializing. Both true or both false does not matter as long as they are the same.
(Test case noscript_parse_roundtrip_stays_the_same prevent this from happening again.)

Looking into #123 the discussion is between disabling or enabling it. A browser definitely wants to enable this to reduce tree complexity and therefore resource reasons so the html5ever default is to enable it. That saves resources for contents which are not needed as a node tree as they are not needed to be shown anyway.
But for a scraper I don't see a reason to save on resources here.
Either you don't care about it, then it works with parsing and serializing either way. Or you want to select it, then you can select it as a whole either way. To get the content, you can still use inner_html and get a String. But with disabling it you can also select things within which is what this library wants to achieve in the first place: select things within the node tree. So I think the only logical choice is to disable it. And for the reasons I just mentioned I don't think it needs to be configurable either.

So for me, it's a bug fix with the side effect of allowing for #123 with the chosen value.

[adamreichold]

I don't think it is just a resource question: When these nodes are suddenly present in the DOM tree, selectors might match them and wrong data might be extracted. So at least, this would be a breaking change.

And then again, I would not mind saving resources either instead of building a part of the DOM tree that I do not care about.

I do agree, that the values for parsing and serialization should match and the current behaviour here is a bug.

What do you think about a scripting-disabled non-default Cargo feature which will change the value of SCRIPTING_ENABLED its default true to false? I think this would avoid the breakage for people who do not expect this content in their selectors while still allowing us to fix the inconsistency.

[EdJoPaTo]

I would not like this as a Cargo feature. They are too messy for configuration. If this really should be changeable it should be at runtime. The Html struct already has a (never used just written to?) quirks_mode so adding it there and reading from there seems also fine.

I would rather split it into two PR then. For now, use true as a default so it's just the bug fix for now. And later in a breaking change release this behavior can be changed? (scraper still uses 0.x.z versions so every update is either major or bug fix anyway. 0.30.0 will be a breaking change release.)

[adamreichold]

I would not like this as a Cargo feature. They are too messy for configuration. If this really should be changeable it should be at runtime. The Html struct already has a (never used just written to?) quirks_mode so adding it there and reading from there seems also fine.

I agree in principle, but then we have to thread it through so that equal values for parsing and serialization are ensured. If that is possible without too much overhead and complexity and you are willing to do the extra work, this would also be viable IMO.

For now, use true as a default so it's just the bug fix for now.

This is certainly reasonable.

And later in a breaking change release this behavior can be changed? (scraper still uses 0.x.z versions so every update is either major or bug fix anyway. 0.30.0 will be a breaking change release.)

Even in semver bump, this is icky as it is a silent behavioural change. So everything compiles, probably even tests (because your tests do not include <noscript> collisions) and then eventually something breaks in production. So I would still prefer that consumers have to actively opt in or at least there is some loud breakage in addition to the silent behavioural change.

[EdJoPaTo]

I went for using the html5ever Default as much as possible for brevity.

Good thing for a follow-up: It should be possible to parse with scripting disabled and serialize with scripting enabled for the same roundtrip output. So I think only the parser needs configurable scripting_enabled to support selections inside noscript.

[adamreichold]

I agree with the reasoning to stay as close to the defaults as possible.