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

Fix for loop bug which breaks favicon replacement #171

Merged
merged 4 commits into from
Nov 9, 2023

Conversation

mdouek
Copy link
Contributor

@mdouek mdouek commented Nov 8, 2023

When iterating over the list , if a link is removed the list gets one element shorter which was causing things to be skipped over.
Switched to only bumping i if nothing is removed.
This was breaking the favicon replacement as "alternate icon" was getting skipped over.
Tested by building own plugin and testing own environment.

Testing done

Submitter checklist

Preview Give feedback

@mdouek mdouek requested a review from a team as a code owner November 8, 2023 00:32
@mdouek
Copy link
Contributor Author

mdouek commented Nov 8, 2023

Note: the commit message is wrong should be "only bump i when link isnt removed"

@TobiX
Copy link
Contributor

TobiX commented Nov 8, 2023

Hmm. Interesting. Is the view returned by getElementsByTagName dynamic in all browsers? So it does shrink when a matching item gets removed? Either way, it might make things cleared to keep the loop as-is, but copy the result of getElementsByTagName to a static array before the loop to avoid the dynamic nature of this array in the first place... (In fact, MDN suggests something similar: https://developer.mozilla.org/en-US/docs/Web/API/HTMLCollection)

@mdouek
Copy link
Contributor Author

mdouek commented Nov 8, 2023

@TobiX I took your advice and just created a static array, makes things easier. sorry for all the commits. I can create a new branch with the one commit and a new PR to keep git cleaner.

@TobiX TobiX merged commit 0da3393 into jenkinsci:main Nov 9, 2023
15 checks passed
@TobiX
Copy link
Contributor

TobiX commented Nov 9, 2023

All fine, I can simply squash all changes.

@TobiX TobiX added the bug label Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants