Not all attributes are removed with Remove() if there are multiple attributes of the same name

[kawaicheung]

1. Description

When using Remove() on the HtmlAttributeCollection where there are multiple attributes of the same name, only some get removed.

In the fiddle below, the attribute class appears five times. Upon calling Remove("class") on the attribute collection, I would expect all five would be removed but instead two still remain.

2. Fiddle or Project

(https://dotnetfiddle.net/zrfFnI)

4. Any further technical details

Add any relevant detail can help us, such as:

• NET version (net45)

[elgonzo]

Quick analysis

The removal logic is broken:

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

Lines 358 to 365 in def26b2

	for (int i = 0; i < items.Count; i++)
	{
	HtmlAttribute att = items[i];
	if (String.Equals(att.Name, name, StringComparison.OrdinalIgnoreCase))
	{
	RemoveAt(i);
	}
	}

Removing an element at position i will "shift" the next element (originally at position i+1) to be now at position i (in other words, all elements following after a removed element will have their collection indices reduced by one due to the removal of the preceding element).

As the loop is iterating in incrementing fashion, removing an element will therefore cause the loop to ignore/skip over the element following a removed element ¹, unless the iterator variable i is adjusted accordingly.

However, instead of clumsily adjusting the iterator variable ², i would like to suggest a somewhat simpler fix: Let the for loop iterate in decrementing fashion, thus it won't skip any elements naturally.

for (int i = items.Count - 1; i >= 0; i--)
{
    HtmlAttribute att = items[i];
    if (String.Equals(att.Name, name, StringComparison.OrdinalIgnoreCase))
    {
        RemoveAt(i);
    }
}

But in the end, either way (whether it is using a decrementing loop, or wether it is keeping the incrementing loop with an additional i-- after RemoveAt(i)) would work, and it's a matter of preference which to choose over the other...

---

¹ This can be seen in the dotnetfiddle demo provided by the original report. Removal of the class=hello attribute instance causes the next item in the attribute collection (class=hi) to be skipped/ignored, continuing removal with the class= attribute instance. Then, the removal of the class= attribute instance causes skipping/ignoring of class=1 (as it is the item directly following after the removed class= attribute instance).

² Manipulating an iterator variable inside a loop body is in my opinion not helping code readibility and maintainability when there are other simple ways that avoid scattering operations manipulating the iterator variable across the loop body.

[JonathanMagnan]

Hello @kawaicheung ,

Thank you for reporting, we will look at it.

Thank also @elgonzo for your detailed answer. The current issue is a very common mistake. We will check if we will apply the fix you provided or create a list of thing to remove which is both usually 2 good solutions for this issue.

Best Regards,

Jon

---

Sponsorship
Help us improve this library

Performance Libraries
context.BulkInsert(list, options => options.BatchSize = 1000);
Entity Framework Extensions • Bulk Operations • Dapper Plus

Runtime Evaluation
Eval.Execute("x + y", new {x = 1, y = 2}); // return 3
C# Eval Function • SQL Eval Function

[JonathanMagnan]

Hello @kawaicheung ,

The v1.11.40 has been released.

My developer preferred to do it a little bit differently but the current issue should be fixed.

Let me know if everything is now working correctly.

Best Regards,

Jon