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

[Lens] Focus Fixes for v8 Theme #106634

Merged
merged 8 commits into from
Jul 30, 2021

Conversation

MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis commented Jul 23, 2021

Summary

This PR fixes broken focus states for field item and dimension item components when using the latest v8 (Amsterdam) theme. Closes #97421.

CCing @andreadelrio and @ryankeairns.

Before Screenshots

image

image

image

After Screenshots

image

image

image

Checklist

Delete any items that are not applicable to this PR.

@MichaelMarcialis MichaelMarcialis added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed v7.15.0 labels Jul 23, 2021
@MichaelMarcialis MichaelMarcialis requested a review from a team as a code owner July 23, 2021 04:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@mbondyra
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Chrome and Safari and noticed that on Safari there's no border-radius:
Screenshot 2021-07-26 at 10 24 55
Screenshot 2021-07-26 at 10 25 03
and the color of the 'ghost dnd' element is still blue:
Screenshot 2021-07-26 at 10 25 41

On FF, every focus is blue, but I think it's expected, because it's the same across all the Kibana:

Screenshot 2021-07-26 at 10 26 52

@andreadelrio
Copy link
Contributor

Tested on Chrome and Safari and noticed that on Safari there's no border-radius:
Screenshot 2021-07-26 at 10 24 55
Screenshot 2021-07-26 at 10 25 03

I was looking into this and it seems that's the expected behavior as per the notes here.

image

and the color of the 'ghost dnd' element is still blue:
Screenshot 2021-07-26 at 10 25 41

In Safari, the outline of the ghost element while being dragged is blue because we're applying outline-style: auto; as seen here:

.lnsDragDrop_ghost {
  @include kbnThemeStyle('v8') {
    outline: $euiFocusRingSize solid currentColor; // Safari & Firefox
    outline-style: auto; // Chrome
  }
}

However, we usually only apply outline-style: auto; inside :focus-visible (as seen in the screenshot above) in order to be able to target Chrome only. In this particular case the element we're dragging gets the class lnsDragDrop_ghost, but this element doesn't get focus (as can be checked through document.activeElement) so there's not way to use :focus and :focus-visible. @mbondyra do you have any ideas on how to work around this?

@mbondyra
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.6MB 1.6MB +1.4KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mbondyra
Copy link
Contributor

Hi @andreadelrio thanks for detailed explanation. I tried to find a way to avoid the blue focus for Safari, but was not able to do so without overcomplicating the code. Overall, I think it's a minor detail so I am ok with leaving it like this.

Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mbondyra
Copy link
Contributor

I'll merge this PR to avoid potential future conflicts. We can follow up if anything surfaces.

@mbondyra mbondyra merged commit 4317dcb into elastic:master Jul 30, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jul 30, 2021
* set correct focus ring for each theme

* fix field and dimension button span faux focus

* fix dimension button focus

* add mixin to remove euiFocusRing

* clean up the mess

Co-authored-by: Kibana Machine <[email protected]>
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jul 30, 2021
* set correct focus ring for each theme

* fix field and dimension button span faux focus

* fix dimension button focus

* add mixin to remove euiFocusRing

* clean up the mess

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.14
7.x

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jul 30, 2021
* set correct focus ring for each theme

* fix field and dimension button span faux focus

* fix dimension button focus

* add mixin to remove euiFocusRing

* clean up the mess

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Michael Marcialis <[email protected]>
kibanamachine added a commit that referenced this pull request Jul 30, 2021
* set correct focus ring for each theme

* fix field and dimension button span faux focus

* fix dimension button focus

* add mixin to remove euiFocusRing

* clean up the mess

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Michael Marcialis <[email protected]>
@MichaelMarcialis MichaelMarcialis deleted the lens/97421/focus-fixes branch August 2, 2021 18:47
streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021
* set correct focus ring for each theme

* fix field and dimension button span faux focus

* fix dimension button focus

* add mixin to remove euiFocusRing

* clean up the mess

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] v8 eui theme adjustments
5 participants