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

Avoid logging warning for missing value for some html attributes such as defer #160

Closed
apupier opened this issue Nov 3, 2020 · 6 comments · Fixed by #338
Closed

Avoid logging warning for missing value for some html attributes such as defer #160

apupier opened this issue Nov 3, 2020 · 6 comments · Fixed by #338
Labels
documentation Improvements or additions to documentation help-wanted External contributions welcome spec:DOM Living Standard https://dom.spec.whatwg.org/ spec:HTML
Milestone

Comments

@apupier
Copy link

apupier commented Nov 3, 2020

When using this kind of notation:
<script defer src="https://use.fontawesome.com/releases/v5.3.1/js/all.js"></script>

a warning is logged: [xmldom warning] attribute "defer" missed value!! "defer" instead2!!

based on html specification https://html.spec.whatwg.org/#attr-script-defer this notation is valid.
So when the mimetype is text/html, I think that there is no warniogn log.

@karfau
Copy link
Member

karfau commented Nov 3, 2020

  • It's possible to pass an errorHandler to the DOMParser constructor, either for all levels or for a single level to tweak how to handle them, e.g. not log warnings or throw on errors. An error handler can either have one or two arguments, if it has two arguments it receives errorLevel and message if it has one one argument it only receives message. (I know they are not documented very well, this should be changed.) Of course skipping only attribute "defer" would mean you need to test the string which could break if we would decide to change the message...

  • If we want to prevent those warnings in text/html mode we would need to maintain a list of tag/attribute combinations that are OK, I'm not sure we want/would be able to invest that time at the moment.

  • I guess we don't want to skip all attribute warnings when there is 'text/html' mime type applied?

  • There is also a behavior implemented for those cases: setting the attribute value to the attribute name, so in the resulting DOM you will see 'defer="defer"'. If that is an issue we would be talking about something that is not that obvious (to me) from your description, but just wanted to mention it to be sure.

@apupier
Copy link
Author

apupier commented Nov 3, 2020

thanks for the quick feedback.

It's possible to pass an errorHandler to the DOMParser constructor, either for all levels or for a single level to tweak how to handle them, e.g. not log warnings or throw on errors. An error handler can either have one or two arguments, if it has two arguments it receives errorLevel and message if it has one one argument it only receives message. (I know they are not documented very well, this should be changed.) Of course skipping only attribute "defer" would mean you need to test the string which could break if we would decide to change the message...

agree that ti is a possible workaround on our side. Not a very maintainable one.

If we want to prevent those warnings in text/html mode we would need to maintain a list of tag/attribute combinations that are OK, I'm not sure we ant to invest that time at the moment.

  • I guess we don't want to skip all attribute warnings when there is 'text/html' mime type applied?

I think not but I'm not an expert of html.
To be complete, there will be the need to first find which attributes can be skipped.
Wondering if it remains valuable to skip the one that I spotted on the script attribute to get started (script defer, script async, script nomodule ) and when other people are spottign othe rplaces like that, they can provide a PR to handle it.

for "do not want to invest time for the moment', do you mean for implementing it? or also for reviewing potential Pull Request?

  • There is also a behavior implemented for those cases: setting the attribute value to the attribute name, so in the resulting DOM you will see 'defer="defer"'. If that is an issue we would be talking about something that is not that obvious (to me) from your description, but just wanted to mention it to be sure.

I think it would be incorrect as the defer is a boolean based the html specification.

@karfau
Copy link
Member

karfau commented Nov 3, 2020

for "do not want to invest time for the moment', do you mean for implementing it? or also for reviewing potential Pull Request?

PRs are always welcome, as long as they don't break anything :)

@karfau
Copy link
Member

karfau commented Nov 3, 2020

I think implementing "boolean" attributes would actually be a breaking change if it would be the default behaviour. So I think it would need some way to "opt-in". The html mime type might be one way to do that but I would expect test to break when it's always enabled for those mime types.

Maybe it would be possible to tweak error handlers so that they get an optional 3rd argument with a well defined data to understand the issue, so that the error handler would also be able to resolve the issue. In this specific case they could even return (or tweak) the attribute value, but I'm not sure if that would also be possible for other error cases.

Just thinking out loud here, for anybody that would try to implement it.

@karfau karfau added enhancement help-wanted External contributions welcome labels Nov 3, 2020
@karfau karfau added the spec:DOM Living Standard https://dom.spec.whatwg.org/ label Jan 21, 2021
@karfau karfau added spec:HTML documentation Improvements or additions to documentation labels Aug 23, 2021
@karfau karfau added this to the 1.0.0 milestone Aug 23, 2021
@karfau
Copy link
Member

karfau commented Oct 1, 2023

I actually decided to no longer log a warning for missing attribute values at all in the context of HTML documents as part of #338, which as been released as part of https://github.com/xmldom/xmldom/releases/tag/0.9.0-beta.1 (available under the dist-tag next).

I is still using the attribute name as the fallback value in that case.

PS: It would now be straight forward to add logic to use a boolean value for some values when being in HTML mode.
But since this issue was about logging the warning, I decided to close it.

@karfau karfau closed this as completed Oct 1, 2023
@karfau karfau reopened this Oct 1, 2023
@karfau karfau closed this as completed Oct 1, 2023
@karfau karfau reopened this Oct 1, 2023
@karfau karfau linked a pull request Oct 1, 2023 that will close this issue
@karfau karfau closed this as completed Oct 1, 2023
@karfau karfau modified the milestones: planning 1.0.0, 0.9.0 Oct 1, 2023
@rschristian
Copy link

The language in the warning is rather bizarre:

attribute "simple" missed value!! "simple" instead2!!

What on earth is "missed value" or "instead2!!" supposed to mean?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation help-wanted External contributions welcome spec:DOM Living Standard https://dom.spec.whatwg.org/ spec:HTML
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants