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(NcRichText): modify MENTION_START regex to work on older MobileSafari versions #5976

Conversation

arthurlockman
Copy link
Contributor

Fixes RichText causes errors on WebKit | Invalid regex #5589

This regex performs the same function as the one used before, but does not make use of negative lookbehinds and therefore should work on older MobileSafari versions.

To test, I looked first at the original mentioned regex from nextcloud server to get an idea of what the intended valid and invalid captures were. I tested both the original and modified regexes for both the server one and the one used in this component to verify that the matching groups were the same. The overall match 1 (blue highlight) is slightly different however the capture group 1 (green highlight) has the same content across both so this should be compatible.

☑️ Resolves

🖼️ Screenshots

🏚️ Before 🏡 After
original modified original
original concatenated userid modified concatenated userid

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@arthurlockman arthurlockman force-pushed the bugfix/5589-invalid-regex-older-ios branch from 251ebea to f7ea735 Compare August 19, 2024 01:16
@arthurlockman
Copy link
Contributor Author

To add: I manually edited the minified JS on one of the instances I administer to the new regex pattern and the forms issue I linked is resolved!

@susnux susnux requested review from Antreesy and ShGKme August 19, 2024 09:34
@susnux susnux added bug Something isn't working 3. to review Waiting for reviews regression Regression of a previous working feature feature: richtext Related to the richtext component labels Aug 19, 2024
@susnux susnux added this to the 8.16.1 milestone Aug 19, 2024
@Antreesy
Copy link
Contributor

Failed tests show that a whitespace (which is now a part of matching group) is trimmed

@arthurlockman
Copy link
Contributor Author

Roger roger, will fix.

@arthurlockman arthurlockman force-pushed the bugfix/5589-invalid-regex-older-ios branch from f7ea735 to 6bba2d8 Compare August 20, 2024 01:43
@arthurlockman
Copy link
Contributor Author

Tests now pass with this new iteration. It's using positive lookaheads which have been supported in MobileSafari since the beginning. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Lookahead_assertion

Copy link

codecov bot commented Aug 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.86%. Comparing base (957f0ac) to head (38d3afe).
Report is 785 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5976      +/-   ##
==========================================
- Coverage   38.62%   37.86%   -0.77%     
==========================================
  Files         142      148       +6     
  Lines        4958     5261     +303     
  Branches     1497     1577      +80     
==========================================
+ Hits         1915     1992      +77     
- Misses       2954     3185     +231     
+ Partials       89       84       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ShGKme ShGKme changed the title Modifying MENTION_START regex to work on older MobileSafari versions fix(NcRichText): modify MENTION_START regex to work on older MobileSafari versions Aug 20, 2024
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Checked test examples with both regex rules, seems to return the same matching groups now. Thanks for the contribution!

Could you try to pull the latest master and rebase your PR on it? Codecov shouldn't be triggered here

src/mixins/richEditor/index.js Outdated Show resolved Hide resolved
…ions

Fixes RichText causes errors on WebKit | Invalid regex nextcloud-libraries#5589

This regex performs the same function as the one used before, but does not make use of negative lookbehinds and therefore should work on older MobileSafari versions.

Co-authored-by: Maksim Sukharev <[email protected]>
Signed-off-by: Arthur Rosa <[email protected]>
@susnux susnux force-pushed the bugfix/5589-invalid-regex-older-ios branch from 1879c1a to 38d3afe Compare August 20, 2024 16:05
@susnux
Copy link
Contributor

susnux commented Aug 20, 2024

/backport to next

@susnux susnux merged commit 8b0065c into nextcloud-libraries:master Aug 20, 2024
15 checks passed
@susnux susnux mentioned this pull request Aug 20, 2024
@susnux susnux modified the milestones: 8.16.1, 8.17.0 Aug 20, 2024
@arthurlockman
Copy link
Contributor Author

Thanks for rebasing! Glad I could help sort this out. It was affecting forms users on one of the instances I manage.

@Iwios
Copy link

Iwios commented Sep 27, 2024

Hello can I manually modify files?
It's really blocking

@susnux
Copy link
Contributor

susnux commented Sep 27, 2024

It is already fixed since a couple of versions, where do you experience this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working feature: richtext Related to the richtext component regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RichText causes errors on WebKit | Invalid regex Form isn't shown on iOS <= 16.2
5 participants