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.

Sign up for GitHub

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

Jump to bottom

Flash the pane dark when BEL is emitted and pane's appearance has a light background #13450

Closed
yunruse opened this issue Jul 7, 2022 · 4 comments · Fixed by #13707
Closed

Flash the pane dark when BEL is emitted and pane's appearance has a light background #13450

yunruse opened this issue Jul 7, 2022 · 4 comments · Fixed by #13707
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Disability-LowVision Accessibility tracking good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. InclusionBacklog-Windows TerminalWin32 Accessibility tracking Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Backlog

Comments

@yunruse
Copy link

yunruse commented Jul 7, 2022

Description of the new feature/enhancement

Pull request Add a setting to flash the pane when BEL is emitted #9270 implemented a visual BEL indictor on the working terminal pane. It's currently a white flash, which works well against a traditional dark-mode background. However, on a light appearance this basically only lightly flashes the text, making it a little hard to notice!
Is there a possibility this could be modified to work with higher contrast light-mode users? Thank you!

Proposed technical implementation details (optional)

I'd probably set (and store?) an isBackgroundLight bool somewhere iff the background color of the pane is "light". If it is, and the BEL handler has the window flag set, the pane would flash dark on a BEL instead of light.

A quick way to find isBackgroundLight may be to just see if the average of r, g and b is higher than 127. I can't imagine enough people have #888888-ish terminals for this to warrant being a manually-defined setting.
@yunruse yunruse added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jul 7, 2022
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jul 7, 2022
@zadjii-msft
Copy link
Member

That seems sensible to me!

@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. Priority-3 A description (P3) and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Jul 7, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 7, 2022
@zadjii-msft
Copy link
Member

(@carlos-zamora you think this is something that counts as an a11y bug?)

@carlos-zamora
Copy link
Member

(@carlos-zamora you think this is something that counts as an a11y bug?)

Yeah. It's basically a high-contrast bug, when you think about it (just not limited to high contrast mode itself). Similar to how we are expected to have a good enough contrast for text in SUI.

@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 7, 2022
@zadjii-msft zadjii-msft added this to the Backlog milestone Jul 13, 2022
@zadjii-msft zadjii-msft added InclusionBacklog-Windows TerminalWin32 Accessibility tracking Disability-LowVision Accessibility tracking good first issue This is a fix that might be easier for someone to do as a first contribution labels Jul 13, 2022
@Fyrebright Fyrebright mentioned this issue Aug 9, 2022
Merged
@ghost ghost added the In-PR This issue has a related PR label Aug 9, 2022
@ghost ghost closed this as completed in #13707 Aug 11, 2022
ghost pushed a commit that referenced this issue Aug 11, 2022
@Fyrebright
Flash the pane dark when BEL is emitted in a light terminal (#13707)
ea04823 
Adds a variable `_isBackgroundLight` that is updated when the background
color is changed. When it is `true`, the BEL indicator flash will darken
the screen instead of brightening.

`_isColorLight(bg)` returns `true` if the average of `r`, `g`, and `b`
is >127

I was unsure of an appropriate way to change the color of the
`CompositionLight` based on the background, so I changed it to always be
gray and adjusted the intensity values of the original animation to have
roughly the same visual effect as the white.

## Validation Steps Performed
* Tested the two flashes on the default color schemes and some custom
  background colors to see if they look consistent
* Used tracepoints and visual to check that the right animation is used
  (including multiple tabs, split windows with different themes, and
  changing settings while window is open)

References #9270
Closes #13450
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Aug 11, 2022
@ghost
Copy link

ghost commented Sep 13, 2022

🎉This issue was addressed in #13707, which has now been successfully released as Windows Terminal Preview v1.16.252.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Disability-LowVision Accessibility tracking good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. InclusionBacklog-Windows TerminalWin32 Accessibility tracking Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Flash the pane dark when BEL is emitted in a light terminal Fyrebright/terminal
3 participants
@carlos-zamora @yunruse @zadjii-msft