GlobalAttributeValueQuote Initial influencing empty attributes

[cx-ricardo-puga]

html-agility-pack/src/HtmlAgilityPack.Shared/HtmlDocument.cs

Line 1870 in 973972a

_currentattribute.InternalQuoteType = AttributeValueQuote.WithoutValue;

1. Description

Since version v1.11.62, when the InternalQuoteType was introduced, I have noticed some unwanted changes in my conversions because it mixes the behavior of the QuoteType and WithoutValue in the attributes.

I'm my case I want to set the document to consider the Inicial QuoteType (the default, I believe) to maintain the original QuoteType and also set it to convert to an empty attribute (att=""). Supustly, converting to an empty attribute was the expected behavior but that isn't happening. Setting the GlobalAttributeValueQuote to Inicial the behavior is the same.

Example:
<bar ng-app class='message'></bar>
Actual:
<bar ng-app class='message'></bar>
Expected:
<bar ng-app="" class='message'></bar>

UT with default configs:

[Fact]
public void PreserveEmptyAttributesWithInitialTest()
{
    var d = new HtmlDocument();
    d.LoadHtml("<bar ng-app class='message'></bar>");
    var node = d.DocumentNode.SelectSingleNode("//bar");
    var outer = node.OuterHtml;
    Assert.Equal("<bar ng-app=\"\" class='message'></bar>", outer);
}

My suggestions:

1. Considering by default that setting the GlobalAttributeValueQuote to Initial will set the isWithoutValue to false;

   • PR - Fix quotetype and withoutvalue behavior #576

2. Splitting the QuoteType and WithoutValue setting in two different settings;

   • PR - Slipt quotetype and withoutvalue; #577

[JonathanMagnan]

Hello @cx-ricardop ,

Thank you for your PR; I will look at them soon.

Indeed, v1.11.62 introduced many unwanted changes, and I feel that I should revert to the previous version for this part of the code. Since this version, we kept "patching" the code to get the right behavior.

I will certainly look at the PR you are proposing.

Best Regards,

Jon

[JonathanMagnan]

Hello @cx-ricardop ,

If I understand correctly, you are currently suggesting two different fixes. So, I need to choose between both of your pull requests and cancel the other one.

At this moment, I prefer the #576 as no new option has been added.

What do you prefer?

I will try to add more unit tests once we choose around the quoting to ensure we have and keep the right behavior.

Best Regards,

Jon

[cx-ricardo-puga]

Hi Jonathan,

Thank you for taking the time to look into this issue.

You’re correct, I’ve proposed two different fixes to provide options or better clarify the problem:

Fix 1: Adjusts the initial behavior of quotetype, offering a minimal-impact fix that should not affect current users.
Fix 2: Introduces an enhancement for greater flexibility. While it resolves the issue, it may impact users relying on AttributeValueQuote.WithoutValue.

In my view, the first fix is safer for the current version, while the second could be considered for a future major update.

Do you have any doubts about my issue?

Best Regards

[JonathanMagnan]

Hello @cx-ricardop ,

Perfect, I will check more the #576 this week.

I don't have any doubt about your code. But I want to ensure that we don't cause more side impact as we did in the v1.11.62

[JonathanMagnan]

Hello @cx-ricardop ,

Sorry for the long delay,

What about adding a new AttributeValueQuote.InitialExceptWithoutValue?

Fix 1: will introduce a kind of new bug (at least the same behavior as before those changes). People would expect that an attribute without value keep being without a quote.

Fix 2: will introduce a new option that most people will not be aware. So it might be harder for people to understand or to know this option.

Fix 3: Adding new options such as AttributeValueQuote.InitialExceptWithoutValue will allow people using this attribute to change it to have the expected behavior easily.

I feel that Fix 3 is like a combination of both of your PRs. A little bit the best of both worlds.

Let me know your opinion

Best Regards,

Jon

[cx-ricardo-puga]

Hi @JonathanMagnan,

I agree with you, your suggestion is the best to solve the problem easily with less impact.
From my side, you can go ahead with this approach.

Many thanks for your effort in this issue.

best regards

[JonathanMagnan]

Hello @cx-ricardop ,

The v1.11.72 has been released.

As said, you will now find the InitialExceptWithoutValue that should behave as you want.

Feel free to offer a PR if you have to change the behavior if I missed something. We still have time to change it ;)

That is no problem. Thank you for helping me improve this library.

Best Regards,

Jon

[cx-ricardo-puga]

Hi @JonathanMagnan,

I’ve tested it and it works perfectly... exactly as expected! Well done.
I’m the one who should be thanking you so much for your dedication and effort.

Wishing you a fantastic year ahead!

Best regards