fix: parsing of srcset URLs after the first URL #1890
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
previously, the first character of srcset URLs past the first one
had their first character dropped. this led to absolute URLs becoming
relative and other big problems. this happened when the srcset did not
have spaces after its commas: `/300.png 300w,/600.png 600w`
really, this could be fixed by simply deleting the `index += 1;` line.
this increment causes a mismatch between the index and the remaining
string which causes the off-by-one error.
this pr makes some extra changes to avoid this duplicated logic by
removing the index variable entirely, since the remaining string is all
you need. this avoids needing to think about indices and should avoid
similar bugs in future.
this pr also changes some pattern match of space to accept other ASCII
whitespace, following the spec.
for those curious, the new test case previously failed with this error:
```
left: ["/300.png", "600.png", "900.png", "ttps://x.invalid/a.png", "elative.png"]
right: ["/300.png", "/600.png", "/900.png", "https://x.invalid/a.png", "relative.png"]
```
related to https://www.github.com/lycheeverse/lychee/issues/1190
katrinafyi
force-pushed
the
srcset
branch
2 times, most recently
from
8fed2ef to
b589a56
Compare
info messages be a warning instead?
thomas-zahner
requested changes
Oct 30, 2025
Member
thomas-zahner
left a comment
thomas-zahner
left a comment
There was a problem hiding this comment.
Ah thank you very much for this bugfix 🚀
| /// Otherwise, in case of srcset syntax errors, returns Err. | ||
| /// | ||
| /// <https://html.spec.whatwg.org/multipage/images.html#parsing-a-srcset-attribute> | ||
| fn parse_one_url(remaining: &str) -> Result<(&str, Option<&str>), String> { |
Member
There was a problem hiding this comment.
Could you extract this function, just above skip_descriptor. Less nesting makes it more readable IMO.
Member
Author
There was a problem hiding this comment.
Sure! I like less indentation too
Merged
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
previously, the first character of srcset URLs past the first URL had their first character dropped. this led to absolute URLs becoming relative and other problems. this happened when the srcset did not have spaces after its commas:
/300.png 300w,/600.png 600wreally, this could be fixed by simply deleting the
index += 1;line. this increment causes a mismatch between the index and the remaining string which causes the off-by-one error.this pr makes some extra changes to avoid this duplicated logic by removing the index variable entirely, since the remaining string is all you need. this avoids needing to think about indices and should avoid similar bugs in future.
this pr also changes some pattern match of space to accept other ASCII whitespace, following the spec.
for those curious, the new test case previously failed with this error:
related to #1190