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

GlobalAttributeValueQuote Initial influencing empty attributes #575

Open
cx-ricardop opened this issue Dec 3, 2024 · 4 comments
Open

GlobalAttributeValueQuote Initial influencing empty attributes #575

cx-ricardop opened this issue Dec 3, 2024 · 4 comments
Assignees

Comments

@cx-ricardop
Copy link

cx-ricardop commented Dec 3, 2024

_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;

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

@JonathanMagnan JonathanMagnan self-assigned this Dec 3, 2024
@JonathanMagnan
Copy link
Member

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
Copy link
Member

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-ricardop
Copy link
Author

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants