department-of-veterans-affairs / va.gov-team

Public resources for building on and in support of VA.gov. Visit complete Knowledge Hub:
https://depo-platform-documentation.scrollhelp.site/index.html
282 stars 203 forks source link

Bug: FE is showing the CMS edit field label on the live form detail landing page #17678

Closed jenniferlee-dsva closed 3 years ago

jenniferlee-dsva commented 3 years ago

Bug: On form detail landing page templates, the CMS field label "Form name" is being pulled in by the FE and showing on the live page.

Desired behavior: Per launch implementation, the form name should appear without the CMS editing field label.

Screenshot of bug:

image.png

Correct way to display in FE:

https://web.archive.org/web/20201024170640/https://www.va.gov/find-forms/about-form-10-10ez/

image.png

zacharymorel commented 3 years ago

Good afternoon @jenniferlee-dsva ! The From Name: seen here was the result of a discussion between Jen, Ryan and I for fixing the following 508 a11y bug.

Does the solution be revisited?

ncksllvn commented 3 years ago

I think the Form name: needs to be wrapped in a tag expressing that it is text for screenreaders-only (because visually it is clear that it's a form name but the label adds meaning to it for a SR.) I think we can just do here the same thing we do for the main form's name.

<dfn class="vads-u-visibility--screen-reader">Form name:</dfn>
johnhashva commented 3 years ago

Love this ^^^ ... we need to be able to solve the accessibility issues when possible w/o negatively impacting the UX experience -- looks like a win-win approach here.

zacharymorel commented 3 years ago

I think the Form name: needs to be wrapped in a tag expressing that it is text for screenreaders-only (because visually it is clear that it's a form name but the label adds meaning to it for a SR.) I think we can just do here the same thing we do for the main form's name.

<dfn class="vads-u-visibility--screen-reader">Form name:</dfn>

What is funny is that in the thread I linked, we went through two solve iterations. The first being this suggestion here where the definition tag is wrapped in a paragraph tag (see this pr). However, we went back on that and settled with that is currently there.

I do not mind going back again.

jenniferlee-dsva commented 3 years ago

@zacharymorel - as this affected the content and design intent, please be sure to validate with myself or @rtwell in the future when anything affects or changes the design/content. We regularly use screenreader-only tags elsewhere and we could have advised so with this request. Thanks for fixing so quickly.

MarciMcGuireGCIO commented 3 years ago

@zacharymorel @ncksllvn @jenniferlee-dsva cc @johnhashva - thanks all for jumping on this, and I will make certain that we review proposed design changes with all the right folks prior to merging going forward. I take full responsibility for not ensuring it was approved by all stakeholders, and am sorry for the trouble.

zacharymorel commented 3 years ago

@johnhashva @MarciMcGuireGCIO maybe we can continue the conversation here rather than opening the closed original User story?

johnhashva commented 3 years ago

@zacharymorel and @MarciMcGuireGCIO works for me. To recap where we landed today:

MarciMcGuireGCIO commented 3 years ago

Thank you for the recap. I messaged Shawna (had her mixed up with Shira) already to request time from Josh.

jenniferlee-dsva commented 3 years ago

All - there is no need to churn on this. The above solution that @ncksllvn provided is the standard way we incorporate screen reader text. I was the content designer on this, so I know what the layout and design of this page is supposed to be. FWIW, this page template had already gone through Design Intent with the platform team.

All that is needed is implementing the FE code update that Nick has recommended.

johnhashva commented 3 years ago

@jenniferlee-dsva we got this. No worries.

MarciMcGuireGCIO commented 3 years ago

I think the Form name: needs to be wrapped in a tag expressing that it is text for screenreaders-only (because visually it is clear that it's a form name but the label adds meaning to it for a SR.) I think we can just do here the same thing we do for the main form's name.

<dfn class="vads-u-visibility--screen-reader">Form name:</dfn>

@joshkimux just reviewed this code snippet and says it's good to go from an accessibility perspective.

johnhashva commented 3 years ago

Just to confirm my understanding (something I do a lot!): This means we can go with the Nick Sullivan recommended approach and hide the label from the UX -- but still achieve Accessibility requirement of enabling the screen-reader to detect -- right?

MarciMcGuireGCIO commented 3 years ago

@johnhashva that's correct. Our goal is to show this to you and get it into today's deployment before tomorrow's code freeze.

zacharymorel commented 3 years ago

Just a warning, this most likely not go out before the code freeze. VSP has a 24-hour review process where we need to wait 24 hours before we ask them to peer review a pull request.

MarciMcGuireGCIO commented 3 years ago

Thank you for the heads-up, @zacharymorel . Do you know if that also applies to bug fixes?

zacharymorel commented 3 years ago

@1Copenut , can you validate that the screen reader still functions as desired for https://www.va.gov/find-forms/about-form-21p-0969/

MarciMcGuireGCIO commented 3 years ago

@joshkimux adding you to this issue. Can you please verify the screen reader behavior for the above fix? Thank you!

joshkimux commented 3 years ago

@MarciMcGuireGCIO Verified on VoiceOver/Safari and NVDA/Firefox, great work folks 😄 I'm confident it will read the same on JAWs/IE as well, but @1Copenut can double check if needed.

zacharymorel commented 3 years ago

@MarciMcGuireGCIO can we close now since a11y gave it the blessing?

MarciMcGuireGCIO commented 3 years ago

@zacharymorel it still needs to get merged after the freeze, and then I'll close it.

1Copenut commented 3 years ago

@MarciMcGuireGCIO && @joshkimux I took a look at this view in staging, and things look correct per the ticket description. No need to review with JAWS, Josh has done a thorough re-test. We're good to roll here.

I did find an interesting situation where we have some Spanish content on the page, without a lang="es" attribute. This is important so screen readers get the pronunciation and cadence right. I'll open a separate ticket (probably a priority 2 or 3 item) for that work.

Thanks all!

zacharymorel commented 3 years ago

@1Copenut Is this suppose to be open still?

MarciMcGuireGCIO commented 3 years ago

Thanks, @1Copenut. We have already a 508 issue https://github.com/department-of-veterans-affairs/va.gov-team/issues/14432 for language that I flagged as sitewide because these page are generated automatically when VA form managers add forms to the forms database. Can we close the new one you opened as a duplicate?

Also, just to re-iterate Zach's question, can this ticket be closed as well? Thanks!!

1Copenut commented 3 years ago

@1Copenut Is this suppose to be open still?

@zacharymorel I'm good to close it any time. I just saw the previous comment from Marci and thought I'd keep it open until she was ready to close.

1Copenut commented 3 years ago

@MarciMcGuireGCIO Excellent, thank you. Yes please, feel free to close this issue and my second item on the language block as a duplicate. Thank you!

MarciMcGuireGCIO commented 3 years ago

Thank you!