Fix AddEnglishSpecifier() producing illegal HTML #3293
Conversation
There was a problem hiding this comment.
To improve testing, I would recommend to add the previously removed strings from the english-texts.json (you can see the activity on that file here).
To be specific, we've had the following problems in the past:
- "Apple App Store" & "Google Play Store" in the app download prompt on the main page, removed in quick fix broken html #2688
- "Dashboard" in further dashboards linked below ours, removed in Remove "Science" from english-texts.json #2706
The fix for this function should be sufficient to re-add all initially defined English strings without no further problematic occurences. From my review, this PR solves both cases mentioned above.
The problem shown in the opening comment has also been fixed. However, one problem remains. Affected English texts within <title> tags are now being replaced, causing illegal HTML. See the screenshots below.
|
Ah, good catch. I will try to include it. Its really not that easy to write efficient Regex queries that are correct and catch all exceptions, but its the easiest fix that doesn't sacrifice functionality. My previous expression wouldn't even finish, it took that long. |
|
I will also add documentation for the expression as it is getting progressively harder to understand. |
|
https://www.coronawarn.app/de/blog/2020-12-16-corona-warn-app-version-1-9/ still shows bad HTML at the top of the page. Are you going to continue with this PR? |
Hey Mike, we made some progress and further discussed the technical side of things. Ultimately it would be nice to traverse the DOM or a tree like structure representing the HTML instead of trying to find a Regular Expression that matches HTML and replaces it. I just got back from the vacation but I won't be able to work on this in the very near future. We need to decide how to continue with this internally, I will get back to you. |
|
@brianebeling @MikeMcC399 Initially I fixed the regex to exclude the Right now GitHub Actions seems to be unavailable, according to the error message of the build pipeline. I will trigger the checks again as soon as possible. |
|
@larswmh I added the previously removed strings to |
There was a problem hiding this comment.
Looks good to me now, thanks for the fix @thorbenkuro and @brianebeling
https://www.coronawarn.app/de/blog/2020-12-16-corona-warn-app-version-1-9/
for example
It fixes it by not replacing the string, if its outside an HTML Element. This can and maybe will have side-effects and needs to be reviewed carefuly. Maybe we can integrate it with Cypress?
Internal Tracking ID: EXPOSUREAPP-14644