Literal display of `&shy` in the footer of the page

Ein-Tim commented 1 year ago

Where to find the issue

In the footer of the page. Please note that the problem is not visible on the production domains p.coronawarn.app, s.coronawarn.app or e.coronawarn.app, as the faulty change has not been deployed yet. It was introduced with https://github.com/corona-warn-app/cwa-event-landingpage/pull/51. The issue is visible when locally building the page or on my Vercel preview deployment here: https://cwa-event-landingpage.vercel.app/de/

Describe the issue

The page shows a literal display of the HTML attribute &shy, instead of processing it as HTML code and hiding it from the UI.

Suggested change

This problem needs further investigation and fixing. @MikeMcC399 made an educated guess on why this happens in https://github.com/corona-warn-app/cwa-event-landingpage/pull/67#issuecomment-1409373764. I'm not qualified enough to further look into this, sorry. However, we could go the simple way and just remove the &shy attribute. If you want to go this way, please let me know and I will provide a PR.

Internal Tracking ID: EXPOSUREAPP-14671

MikeMcC399 commented 1 year ago

@Ein-Tim

we could go the simple way and just remove the &shy attribute.

This would not solve the complete set of issues here.

MikeMcC399 commented 1 year ago

The faulty PR https://github.com/corona-warn-app/cwa-event-landingpage/pull/51 is causing multiple issues. These might have been noticed earlier if it had ever been deployed to production.

Issues

Cosmetic

Display of  character

Functional

Open http://localhost:8000/de/ Scroll to bottom Click on link Barrierefreiheits&shy;erklärung to http://localhost:8000/de/coronawarn.app/de/accessibility and note the error message "Cannot GET /de/coronawarn.app/de/accessibility"

Accessibility

Enable a screen reader and navigate to the social media icons. Note that their names are not read out.
View the HTML code and note that the accessibility CSS class aria-label is missing.

Conclusion

This repository was never converted to contain accessible code like the main site https://www.coronawarn.app/ was.

It is misleading to link to the Accessibility statement on https://www.coronawarn.app/en/accessibility/ when the sites e.coronawarn.app (for event-registration), s.coronawarn.app (for rapid antigen tests) and p.coronawarn.app (for rapid PCR tests) were never made accessible. Although the Accessibility statement only refers in its text to the www.coronawarn.app site, the presence of the link also on the e.coronawarn.app, s.coronawarn.app and p.coronawarn.app sites implies that they are accessible when they are not.

It also introduces technical issues (see above) when the accessibility link is added.

Suggested fix

Revert PR https://github.com/corona-warn-app/cwa-event-landingpage/pull/51 to remove the link to the Accessibility statement since it is inaccurate in this position and causes technical issues.

Ein-Tim commented 1 year ago

@svengabr Could you take a look please, as you merged PR #51. I certainly agree with @MikeMcC399 and would offer a PR reverting the change, however, it's up to you to decide what we finally do.

MikeMcC399 commented 1 year ago

@Ein-Tim

The addition of soft-hyphen () on www.coronawarn.app was through https://github.com/corona-warn-app/cwa-website/pull/2560. This included scss changes (remove nowrap) as well as text changes.
The soft-hyphen should not have been applied to this website in PR #51 without the scss change. Apparently nobody tested it on a mobile device and nobody checked that the change had been successfully moved to production. 😟 This has been a breakdown in testing and in process. Luckily two errors have cancelled each other out, because the lack of publishing the changed website meant that the error was never moved to production and it can now be corrected internally first. 🙂

Ein-Tim commented 1 year ago

@MikeMcC399 I still wonder why everything looks correct in the screenshots I added to #51. I'm very certain I took them from the footer of a local instance build from the branch of PR #51... Anyway, I'm waiting for further advice from the Open-Source-Team on this. cc @dsarkar & @larswmh

MikeMcC399 commented 1 year ago

@Ein-Tim

I cannot say why your screenshot of the German page does not fit the merged PR. You can recreate the state after the PR was merged using the following:

git switch main
git switch -c recreate-PR51-state
git reset 2f5e3c72fddcf67e6ab9fa19ca350f3425d27dc2 --hard
nvm use 14
npm ci
npm start

Then open http://localhost:8000/de/ and scroll to the bottom.

dsarkar commented 1 year ago

@MikeMcC399 @Ein-Tim no need to revert, @larswmh will come back to you with a solution. Thanks!

dsarkar commented 1 year ago

regarding see https://github.com/corona-warn-app/cwa-website/pull/221/files
actually, the link to the accessibility notice should be removed, that part should be reverted, as suggested by @MikeMcC399

MikeMcC399 commented 1 year ago

@dsarkar

regarding see https://github.com/corona-warn-app/cwa-website/pull/221/files

That is unrelated to the footer.

actually, the link to the accessibility notice should be removed, that part should be reverted, as suggested by @MikeMcC399

Good you agree! I see now that PR #70 will revert the insertion of the problematic "Accessibility statement" link in the footer.

dsarkar commented 1 year ago

@MikeMcC399 what I meant was, that three braces will solve the issue.

MikeMcC399 commented 1 year ago

@dsarkar

My apologies for misunderstanding you! Sorry!

dsarkar commented 1 year ago

@MikeMcC399 No sorries, no worries! ;-)

corona-warn-app / cwa-event-landingpage