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

Add "use div as legend" config and features value #803

Closed
wants to merge 1 commit into from

Conversation

plocket
Copy link
Contributor

@plocket plocket commented Dec 3, 2024

Let devs replace visible legend elems with div. Follows accessibility guidelines by using aria-labelledby and id.

Devs can set "use div as legend" in the config file or the same key in the features block to a boolean. The feature block value takes precedence. This will change the visible HTML legend elements to divs. If the dev uses anything other than a boolean, they get an error message.

The motivation is Safari's (old and current) desktop versions and some user testing. Bootstrap says it supports current versions of Safari and that's correct - this isn't a direct Bootstrap problem. It's about the HTML is put together and which of Bootstrap's features are used there. With HTML that has a legend that uses Bootstrap's flexbox styles, Safari desktop handles those legend nodes incorrectly. The legend text is above the choices and right-aligned:

image

User tests found that this visual was confusing. @VTskier might be able to speak about that.

This change fixes that issue with minimal side effects. Here is a decision matrix that I've shared.

Why allow both config and feature block keys? If a server has multiple organizations, as Suffolk's server does, different orgs might want to control their own settings.

Why change only visible legend elements? It is a smaller change and I thought it would be less trouble to review.

Why ensure always unique random ids? Devs should not rely on the selector. Avoids later breaking changes.

Manual tests

Outcomes of tests for configuration combinations:

config feature result
absent absent legend
false absent legend
true absent div
absent true div
false true div
true true div
absent false legend
false false legend
true false legend
chair absent legend
chair false legend
chair true div

Tests for error messages:

where value error location error message
config chair websockets.log The Configuration directive "use div as legend" must be True or False.
feature chair webpage "use div as legend" in the "features" block must be either True or False.

Relevant files

Edited:

  • docassemble_base/docassemble/base/config.py
  • docassemble_base/docassemble/base/parse.py
  • docassemble_base/docassemble/base/standardformatter.py
  • docassemble_webapp/docassemble/webapp/static/app/app.css
  • docassemble_webapp/docassemble/webapp/server.py
  • tests/features/TestExamples.feature

Ignored:

Compiled files (ignored):

  • docassemble_webapp/docassemble/webapp/static/app/adminbundle.js
  • docassemble_webapp/docassemble/webapp/static/app/app.min.css
  • docassemble_webapp/docassemble/webapp/static/app/bundle.css
  • docassemble_webapp/docassemble/webapp/static/app/bundle.js
  • docassemble_webapp/docassemble/webapp/static/app/cm6.min.js
  • docassemble_webapp/docassemble/webapp/static/bootstrap/css/bootstrap.min.css.map

I found no appearances of "disabled" associated with legends.

Let devs replace visible `legend` elems with `div`. Follows
accessibility guidelines by using `aria-labelledby` and `id`.

Devs can set "use div as legend" in the config file or the same key in
the features block to a boolean. The feature block value takes
precedence. This will change the visible HTML legend elements to divs.
If the dev uses anything other than a boolean, they get an error
message.

Why change only visible legend elements?

It is a smaller change that I thought would be more acceptable.

Why ensure always unique random ids?

Devs should not rely on the selector. Avoids later breaking changes.

Outcome of configuration combinations:

- [x] config: none/feature: none - legend
- [x] config: false/feature: none - legend
- [x] config: true/feature: none - div

- [x] config: none/feature: true - div
- [x] config: false/feature: true - div
- [x] config: true/feature: true - div

- [x] config: none/feature: false - legend
- [x] config: false/feature: false - legend
- [x] config: true/feature: false - legend

- [x] config: chair/feature: none - legend
- [x] config: chair/feature: true - div
- [x] config: chair/feature: false - legend

Error messages:

- [x] config: chair - websockets.log: The Configuration directive "use div as legend" must be True or False.
- [x] feature: chair - webpage: "use div as legend" in "features" must be either True or False.

Edited:

- docassemble_base/docassemble/base/config.py
- docassemble_base/docassemble/base/parse.py
- docassemble_base/docassemble/base/standardformatter.py
- docassemble_webapp/docassemble/webapp/static/app/app.css
- docassemble_webapp/docassemble/webapp/server.py
- tests/features/TestExamples.feature

Ignored:

- docassemble_demo/docassemble/demo/data/static/lumen.min.css:
    Disabling the styles in the DOM didn't have an effect.
- docassemble_webapp/docassemble/webapp/static/bootstrap/css/bootstrap.min.css:
    Same styles as lumen.
- docassemble_webapp/docassemble/webapp/static/app/cm6.js:
    Treats `div` and `legend` the same.
- docassemble_webapp/docassemble/webapp/static/app/jquery.min.js:
    jQuery uses `legend` in checks for disabled fields. Both `div`
    and `legend` cannot be disabled and from what I saw in the code
    I believe jQuery would treat them the same way:
    https://github.com/jquery/jquery/blob/03e183c4ccf22bf4031920c3270c9f113cb72d1d/src/selector.js#L277
    jQuery mentions the spec for disableable elements:
    https://github.com/jquery/jquery/blob/03e183c4ccf22bf4031920c3270c9f113cb72d1d/src/selector.js#L248-L250
- docassemble_webapp/docassemble/webapp/static/app/MaterialIcons-Regular.json:
    MaterialIcons icons are independent of the html that uses them.

Compiled files (ignored):

- docassemble_webapp/docassemble/webapp/static/app/adminbundle.js
- docassemble_webapp/docassemble/webapp/static/app/app.min.css
- docassemble_webapp/docassemble/webapp/static/app/bundle.css
- docassemble_webapp/docassemble/webapp/static/app/bundle.js
- docassemble_webapp/docassemble/webapp/static/app/cm6.min.js
- docassemble_webapp/docassemble/webapp/static/bootstrap/css/bootstrap.min.css.map

I found no a appearances of disabled associated with legends.

Co-authored-by: ttiimm <likarish@gmail.com>
@VTskier
Copy link

VTskier commented Dec 4, 2024

We would love to see this fix as we ran into the issue during public user testing. During a recent interview testing cycle, 2 of our first 3 testers were (randomly) on Mac laptops and saw this issue. Thanks for considering!

@mnewsted
Copy link

mnewsted commented Dec 6, 2024

ILAO and its users would like to see this issue addressed as well. We have encountered it in our internal testing. Thanks @plocket for writing this up and thanks @jhpyle for considering.

@jhpyle jhpyle closed this Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants