Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 4 commits into from
+6 −3

Conversation

@IvanIhnatsiuk
Copy link
Contributor

@IvanIhnatsiuk IvanIhnatsiuk commented Nov 3, 2025

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

szydlovsky commented Nov 4, 2025

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
@szydlovsky
Copy link
Collaborator

szydlovsky commented Nov 6, 2025 •
edited
Loading

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

@szydlovsky szydlovsky left a comment •
edited
Loading

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

IvanIhnatsiuk commented Nov 7, 2025

@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
Copy link
Collaborator

szydlovsky commented Nov 7, 2025

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

@szydlovsky szydlovsky left a comment •
edited
Loading

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.

@szydlovsky szydlovsky self-requested a review November 14, 2025 10:21
@szydlovsky szydlovsky self-requested a review November 18, 2025 09:37
@szydlovsky
Copy link
Collaborator

szydlovsky commented Nov 18, 2025 •
edited
Loading

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?

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