Screen reader-friendly errors by JoaquinTrinanes · Pull Request #10122 · nushell/nushell

JoaquinTrinanes · 2023-08-26T00:53:53Z

Hopefully closes Add configuration parameter to turn off or change the type of border decorations on error messages and possibly other content like the table mode key in config.nu #10120

Description

This PR adds a new config item, error_style. It will render errors in a screen reader friendly mode when set to "plain". This is done using miette's own NarratableReportHandler, which seamlessly replaces the default one when needed.

Before:

Error: nu::shell::external_command

  × External command failed
   ╭─[entry #2:1:1]
 1 │ doesnt exist
   · ───┬──
   ·    ╰── executable was not found
   ╰────
  help: No such file or directory (os error 2)

After:

Error: External command failed
    Diagnostic severity: error
Begin snippet for entry #4 starting at line 1, column 1

snippet line 1: doesnt exist
    label at line 1, columns 1 to 6: executable was not found
diagnostic help: No such file or directory (os error 2)
diagnostic code: nu::shell::external_command

Things to be determined

~~Review naming. errors.style is not that consistent with the rest of the code. Menus use a style record, but table rendering mode is set via mode.~~ As it's a single config, we're using error_style for now.
Should this kind of setting be toggable with one single parameter? accessibility.no_decorations or similar, which would adjust the style of both errors and tables accordingly.

User-Facing Changes

No changes by default, errors will be rendered differently if error_style is set to simple.

Tests + Formatting

After Submitting

There's a PR updating the docs over here nushell/nushell.github.io#1026

amtoine

thanks a lot @JoaquinTrinanes for implementing this, it's a great step in the right direction for accessibility 🙏

i propose we simply wait for some feedback from @edhowland to make sure it's easier to read for them, and then it looks like we can land this 💪

kubouch · 2023-08-26T10:30:11Z

crates/nu-utils/src/sample_config/default_config.nu

    }

+    errors: {
+        style: "fancy" # "fancy" or "narratable" for screen reader-friendly error messages


Unless we have more error configs, I would not nest and instead use error_style: "fancy".

Just moved it outside to a single option in 281c1af.

I noticed that some config options are reconstructed after failing validation, while others (specially more simple fields like strings) aren't. I still do the reconstruction for this option according to the rules™️

// Config record (self) mutation rules: // * When parsing a config Record, if a config key error occurs, remove the key. // * When parsing a config Record, if a config value error occurs, replace the value // with a reconstructed Nu value for the current (unaltered) configuration for that setting. // For instance: // $env.config.ls.use_ls_colors = 2 results in an error, so // the current use_ls_colors config setting is converted to a Value::Boolean and inserted in the // record in place of the 2.

Happy to change it if I misunderstood :)

kubouch · 2023-08-26T10:36:32Z

Great, thanks! Looks good to me as well.

fdncred · 2023-08-26T12:17:15Z

crates/nu-utils/src/sample_config/default_config.nu

        header_on_separator: false # show header text on separator/border line
    }

+    error_style: "fancy"


There should be comments showing the available options for error_style so people know what the options are.

I'm fine with fancy as our default, but I don't really like the term narratable. Maybe one of these is better. I kind of lean toward "simple".

plain

without_borders

screen_reader

simple

bare

The term "narratable" might seem strange (not a native speaker), but I like that it conveys the purpose of the setting: to be narratable by screen readers. We could have simple or similar instead, but then it should be noted in the comment that it's mean to be screen reader--friendly.

I just added the comment back. It was there before but I accidentally deleted it in the last commit 😅

fancy refers to how miette itself calls this style, and narratable follows the same "logic" (they call the report handler NarratableReportHandler). Agree, it sounds weird 😬 I'll set it to simple for now if that's ok? I'm not sure about how these kind of decisions should be made

I'd sign off on it if edhowland agrees it's good. Since he's blind and uses nushell, I'd like to get his input before finalizing the name. I'll ask him in Discord #general https://discord.com/channels/601130461678272522/601130461678272524/1144979237707722752.

For those not following Discord, this is what edhowland said, "I read the PR and the comments and suggestions. I like either simple or plain. Prefer plain over simple. Reason: No truly hard and fast reason, but the 'bat' command (a replacement for 'cat' written in Rust) also implemented the default behaviour of using box drawing characters for the output for border decorations. To suppress this they added the [-p, --plain] option. So, screen reader users already have at least that one firmly planted in their memories. But I am only 1 dude. Also: plain <=> fancy seems like their are natural antonyms of each other. Thoughts?"

So, since there is a precedent with bat, I move that we maintain that precedent and name the two options fancy and plain for key error_style.

Are there further thoughts?

Renamed in 94295b8! :)

fdncred

I like it. It seems to work as expected. Except for the "plain" text. That doesn't seem right. "entry #6" is probably right, which you can see with view files, but seems like it should say something other than that because that won't be super helpful.

💡 When you do view files you get a list of file names, a span, and size. If you want to view the contents of one of these "file names" like "entry #6", you can use view span like this.

❯ view span 336112 336125
force_error 1

But now that I look at it, it doesn't say force_error anywhere. I wonder why?

I think something must be wrong

JoaquinTrinanes · 2023-08-26T23:55:48Z

@fdncred I'm confused 🥲

force_error appears on the second to last line:

snippet line 1: force_error 1

It looks like this when split in 2 lines:

Error: oh no!
    Diagnostic severity: error
Begin snippet for entry #15 starting at line 1, column 1

snippet line 1: (force_error
snippet line 2: 1)
    label at line 2, column 1: here's the error

I don't know what could be done about the entry #N thing, as that appears for both error reporters and it's a value explicitly set for the call as the filename:

nushell/crates/nu-cli/src/repl.rs

Line 590 in 5ac5b90

&format!("entry #{entry_num}"),

That will show the name of the file that errors when evaluating a file. I did a quick experiment replacing the filename type with Option and passing none to the eval_source function, and it will default to source (Begin snippet for source starting at line...)

fdncred · 2023-08-27T00:27:33Z

ya, you're right. maybe it's ok. thanks for looking into it. it just seems not as helpful. maybe it's as good as we can get with miette?

JoaquinTrinanes · 2023-08-27T03:06:10Z

maybe it's as good as we can get with miette?

Not necessarily, it would be a matter of removing the default source from here and passing None instead of Entry #N. Looking at the miette source not having a filename should render Begin snippet starting at....

However, I briefly tried and couldn't find an easy way to do it without the need to change the parser quite a bit. I'll be happy to further look into this but I think something of that scope should be another PR 😄

And there's always the option of creating our own report handler of course 😌

fdncred · 2023-08-27T11:54:12Z

ok, let's move forward with this. It's not perfect but provides a feature that's better than we had before for our screen reader community. Thanks!

- Hopefully closes nushell#10120 # Description This PR adds a new config item, `error_style`. It will render errors in a screen reader friendly mode when set to `"simple"`. This is done using `miette`'s own `NarratableReportHandler`, which seamlessly replaces the default one when needed. Before: ``` Error: nu::shell::external_command × External command failed ╭─[entry nushell#2:1:1] 1 │ doesnt exist · ───┬── · ╰── executable was not found ╰──── help: No such file or directory (os error 2) ``` After: ``` Error: External command failed Diagnostic severity: error Begin snippet for entry nushell#4 starting at line 1, column 1 snippet line 1: doesnt exist label at line 1, columns 1 to 6: executable was not found diagnostic help: No such file or directory (os error 2) diagnostic code: nu::shell::external_command ``` ## Things to be determined - ~Review naming. `errors.style` is not _that_ consistent with the rest of the code. Menus use a `style` record, but table rendering mode is set via `mode`.~ As it's a single config, we're using `error_style` for now. - Should this kind of setting be toggable with one single parameter? `accessibility.no_decorations` or similar, which would adjust the style of both errors and tables accordingly. # User-Facing Changes No changes by default, errors will be rendered differently if `error_style` is set to `simple`. # Tests + Formatting  # After Submitting There's a PR updating the docs over here nushell/nushell.github.io#1026

JoaquinTrinanes added 4 commits August 26, 2023 02:04

Add 'errors' config record

82eaacc

Change report handler based on errors.style config

a14d57b

Update default config file

0fdfdc2

Add error style tests

a377e5f

JoaquinTrinanes force-pushed the feature/narratable-errors branch from 1af5181 to a377e5f Compare August 26, 2023 01:29

JoaquinTrinanes mentioned this pull request Aug 26, 2023

Error style section nushell/nushell.github.io#1026

Merged

amtoine reviewed Aug 26, 2023

View reviewed changes

amtoine added the A:accessibility An issue to make Nushell more accessible to anyone label Aug 26, 2023

kubouch reviewed Aug 26, 2023

View reviewed changes

JoaquinTrinanes added 3 commits August 26, 2023 13:29

Fix typo

f622dd9

Make error_style a single config item

281c1af

Fix default config

ac64559

fdncred reviewed Aug 26, 2023

View reviewed changes

JoaquinTrinanes added 3 commits August 26, 2023 14:28

Add error_style comment explanation back

af6355e

Rename narratable option to simple

363f550

Rename simple to plain

94295b8

fdncred previously approved these changes Aug 26, 2023

View reviewed changes

fdncred merged commit cc805f3 into nushell:main Aug 27, 2023

fdncred added the notes:mention Include the release notes summary in the "Hall of Fame" section label Aug 27, 2023

JoaquinTrinanes deleted the feature/narratable-errors branch August 27, 2023 12:03

Conversation

JoaquinTrinanes commented Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Things to be determined

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

amtoine left a comment

Choose a reason for hiding this comment

Uh oh!

kubouch Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

JoaquinTrinanes Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

kubouch commented Aug 26, 2023

Uh oh!

fdncred Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

kubouch Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

JoaquinTrinanes Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

fdncred Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdncred Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

JoaquinTrinanes Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

fdncred left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoaquinTrinanes commented Aug 26, 2023

Uh oh!

fdncred commented Aug 27, 2023

Uh oh!

JoaquinTrinanes commented Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fdncred commented Aug 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JoaquinTrinanes commented Aug 26, 2023 •

edited

Loading

fdncred Aug 26, 2023 •

edited

Loading

fdncred left a comment •

edited

Loading

JoaquinTrinanes commented Aug 27, 2023 •

edited

Loading