Skip to content

fix(iOS): add a check that the href attribute exists in link params - #223

Closed
IvanIhnatsiuk wants to merge 4 commits into
software-mansion:mainfrom
IvanIhnatsiuk:fix/iOS-crash-caused-by-missing-href-attribute
Closed

fix(iOS): add a check that the href attribute exists in link params#223
IvanIhnatsiuk wants to merge 4 commits into
software-mansion:mainfrom
IvanIhnatsiuk:fix/iOS-crash-caused-by-missing-href-attribute

Conversation

@IvanIhnatsiuk

@IvanIhnatsiuk IvanIhnatsiuk commented Nov 3, 2025

Copy link
Copy Markdown
Contributor

Description:

If we pass a default value on iOS, like:

<html>
 <a>Test link</a>
</html>

Then we get a crash because we're trying to get a range of href that doesn't exist

What's changed:

  • added href location check on iOS

Screenshots:

Before:

Screen.Recording.2025-11-03.at.17.01.30.mov

After:

Screen.Recording.2025-11-03.at.17.00.41.mov

Crash stack trace:

crash.txt

@exploIF
exploIF requested a review from szydlovsky November 4, 2025 07:42
@szydlovsky

szydlovsky commented Nov 4, 2025

Copy link
Copy Markdown
Collaborator

Thanks for the PR, looks promising! I'll need to check for places where url might be taken for a granted in link and will come back to you

szydlovsky
szydlovsky requested changes Nov 6, 2025
View reviewed changes
Comment thread ios/inputParser/InputParser.mm Outdated
@szydlovsky

szydlovsky commented Nov 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

And one more thing @IvanIhnatsiuk; if you want the emitted html to not have the href attribute, you have to modify tagContentForStyle method in the InputParser as well and add a check to not add it when url is empty

szydlovsky
szydlovsky reviewed Nov 7, 2025
View reviewed changes

@szydlovsky szydlovsky left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, almost there.
I think we don't want to emit html with empty urls, see #223 (comment) - if the url is empty, don't add the href attribute.

@IvanIhnatsiuk

IvanIhnatsiuk commented Nov 7, 2025

Copy link
Copy Markdown
Contributor Author

@szydlovsky, from my point of view, if we generate a link, then the href attribute should always be there (ideally). This bug is addressed to only to prevent a crash when we get the link from the initial value or manually call setValue with HTML content

@szydlovsky

szydlovsky commented Nov 7, 2025

Copy link
Copy Markdown
Collaborator

Okay @IvanIhnatsiuk this sounds reasonable. Let me test things out for the last time on Monday and we can proceed with the PR

@szydlovsky
szydlovsky self-requested a review November 7, 2025 14:05
szydlovsky
szydlovsky reviewed Nov 14, 2025
View reviewed changes

@szydlovsky szydlovsky left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks and works good. Just this tiny one cosmetic change and we can merge it.

Comment thread ios/inputParser/InputParser.mm Outdated
@szydlovsky
szydlovsky self-requested a review November 14, 2025 10:21
@szydlovsky
szydlovsky self-requested a review November 18, 2025 09:37
@szydlovsky

szydlovsky commented Nov 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Hey @IvanIhnatsiuk, we spoke with @exploIF a little about the changes in here and created a counter-offer PR, see here.
The issue with this approach is the library never produces <a> tags with no href attribute, so there is no reason to consider such tag a proper link. Instead, the PR I made just gets rid of the crash and doesn't recognize links if there is no href or an empty one. Is that approach still okay with your use-case? Was it to just get rid of a crash or are the empty links something you need?

@IvanIhnatsiuk

IvanIhnatsiuk commented Nov 19, 2025

Copy link
Copy Markdown
Contributor Author

@IvanIhnatsiuk yes, it sounds good to me 🙂

@szydlovsky

szydlovsky commented Nov 20, 2025

Copy link
Copy Markdown
Collaborator

Then closing the PR and merging the other option, nonetheless thanks for the idea!

@szydlovsky szydlovsky closed this Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@szydlovsky szydlovsky Awaiting requested review from szydlovsky

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@IvanIhnatsiuk @szydlovsky