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

refactor!: update select to use new structure #2282

Merged
merged 12 commits into from
Sep 2, 2021
Merged

Conversation

web-padawan
Copy link
Member

@web-padawan web-padawan commented Jul 29, 2021

Description

This PR adds @vaadin/select base class, Lumo and Material theme, and visual tests.

Fixes #2197
Fixes #2198

Type of change

  • Breaking change

@fhdhsni
Copy link

fhdhsni commented Aug 10, 2021

Points 8 and 10 from this comment is applicable here too:

  • aria-owns and aria-controls on the button referring to the listbox.
  • having aria-labelledby(refering to the label)/aria-label attributes on the listbox

@web-padawan web-padawan requested review from vursen and removed request for fhdhsni August 30, 2021 12:14
@web-padawan web-padawan changed the title feat: add select using slotted button refactor!: update select to use new structure Aug 30, 2021
@web-padawan web-padawan force-pushed the feat/select branch 2 times, most recently from 87f3c92 to 22d8e16 Compare August 30, 2021 15:04
@web-padawan web-padawan added the a11y phase 1 First batch of accessibility fixes label Aug 30, 2021
packages/select/src/vaadin-select.js Outdated Show resolved Hide resolved
packages/select/src/vaadin-select.js Outdated Show resolved Hide resolved
packages/select/src/vaadin-select.js Outdated Show resolved Hide resolved
packages/select/src/vaadin-select.js Outdated Show resolved Hide resolved
packages/select/src/vaadin-select.js Outdated Show resolved Hide resolved
packages/select/src/vaadin-select.js Show resolved Hide resolved
packages/select/src/vaadin-select.js Outdated Show resolved Hide resolved
packages/select/test/accessibility.test.js Outdated Show resolved Hide resolved
packages/select/test/accessibility.test.js Outdated Show resolved Hide resolved
packages/select/test/scrollable-viewport.test.js Outdated Show resolved Hide resolved
packages/select/test/select.test.js Outdated Show resolved Hide resolved
packages/select/test/select.test.js Outdated Show resolved Hide resolved
packages/select/src/vaadin-select.js Outdated Show resolved Hide resolved
@vursen
Copy link
Contributor

vursen commented Sep 1, 2021

Observation: An unknown symbol appears in the announcement in Safari

OS: Mac OS Big Sur 11.5.2

Browser: Safari 14.1.2

When announcing an option in Safari, there appears an unknown symbol in the announcement, in the place of the selected icon (represented as the ::before pseudo-element in the DOM).

This symbol doesn't get announced on my computer so it is apparently not an issue.

image

Snippet:

<div id="select"></div>

<script type="module">
  import '@vaadin/select';

  import { html, render } from 'lit';

  render(html`
    <vaadin-select
      value="recent"
      label="Sort by"
      .renderer="${(root) =>
        render(
          html`
            <vaadin-list-box>
              <vaadin-item value="recent">Most recent first</vaadin-item>
              <vaadin-item value="rating-desc">Rating: high to low</vaadin-item>
              <vaadin-item value="rating-asc">Rating: low to high</vaadin-item>
              <vaadin-item value="price-desc">Price: high to low</vaadin-item>
              <vaadin-item value="price-asc">Price: low to high</vaadin-item>
            </vaadin-list-box>
          `,
          root
        )}"
    ></vaadin-select>
  `, document.querySelector('#select'))
</script>

@vursen
Copy link
Contributor

vursen commented Sep 1, 2021

Issue: The selected option doesn't get announced as selected in Chrome

OS: Mac OS Big Sur 11.5.2

Browser: Chrome 93.0.4577.63 (Official Build) (arm64)

There seems to be a bug in Chrome as the selected option doesn't get announced as selected after the focus has moved to another option once (see the recording).

Although, everything announces correctly in Safari and Firefox.

Screen.Recording.2021-09-01.at.11.08.48.mov

Snippet:

<div id="select"></div>

<script type="module">
  import '@vaadin/select';

  import { html, render } from 'lit';

  render(html`
    <vaadin-select
      value="recent"
      label="Sort by"
      .renderer="${(root) =>
        render(
          html`
            <vaadin-list-box>
              <vaadin-item value="recent">Most recent first</vaadin-item>
              <vaadin-item value="rating-desc">Rating: high to low</vaadin-item>
              <vaadin-item value="rating-asc">Rating: low to high</vaadin-item>
              <vaadin-item value="price-desc">Price: high to low</vaadin-item>
              <vaadin-item value="price-asc">Price: low to high</vaadin-item>
            </vaadin-list-box>
          `,
          root
        )}"
    ></vaadin-select>
  `, document.querySelector('#select'))
</script>

@vursen
Copy link
Contributor

vursen commented Sep 1, 2021

Issue: The list box doesn't get the correct position in Firefox

OS: Mac OS Big Sur 11.5.2

Browser: Firefox 91.0.2

When opening the list box, it is shown at the bottom-left corner of the page and isn't positioned relative to the select element in Firefox (see the screenshot).

image

Snippet:

<div id="select"></div>

<script type="module">
  import '@vaadin/select';

  import { html, render } from 'lit';

  render(html`
    <vaadin-select
      value="recent"
      label="Sort by"
      .renderer="${(root) =>
        render(
          html`
            <vaadin-list-box>
              <vaadin-item value="recent">Most recent first</vaadin-item>
              <vaadin-item value="rating-desc">Rating: high to low</vaadin-item>
              <vaadin-item value="rating-asc">Rating: low to high</vaadin-item>
              <vaadin-item value="price-desc">Price: high to low</vaadin-item>
              <vaadin-item value="price-asc">Price: low to high</vaadin-item>
            </vaadin-list-box>
          `,
          root
        )}"
    ></vaadin-select>
  `, document.querySelector('#select'))
</script>

@web-padawan
Copy link
Member Author

Observation: An unknown symbol appears in the announcement in Safari

Created a separate issue to handle this in vaadin-item properly: #2429

Issue: The list box doesn't get the correct position in Firefox

This appears to be an edge case I couldn't reproduce. Reported #2430

@sonarcloud
Copy link

sonarcloud bot commented Sep 1, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@web-padawan
Copy link
Member Author

Regarding #2282 (comment)

aria-owns and aria-controls on the button referring to the listbox.

This is not mentioned in the ARIA practices example that we use as a reference, for now I will skip this part.

having aria-labelledby(refering to the label)/aria-label attributes on the listbox

This does not seem to have any effect in VoiceOver and it will only work if vaadin-select is in global scope.

@web-padawan
Copy link
Member Author

There seems to be a bug in Chrome as the selected option doesn't get announced as selected

I think this is related to vaadin-list-box and it has other issues. I will create a separate issue for it tomorrow.

@vursen
Copy link
Contributor

vursen commented Sep 2, 2021

Changes in the Lumo theme

diff --git a/packages/select/theme/lumo/vaadin-select-styles.js b/packages/select/theme/lumo/vaadin-select-styles.js
index 050f81d1d..73de1d218 100644
--- a/packages/select/theme/lumo/vaadin-select-styles.js
+++ b/packages/select/theme/lumo/vaadin-select-styles.js
@@ -1,43 +1,35 @@
+/**
+ * @license
+ * Copyright (c) 2021 Vaadin Ltd.
+ * This program is available under Apache License Version 2.0, available at https://vaadin.com/license/
+ */
 import { registerStyles, css } from '@vaadin/vaadin-themable-mixin/register-styles.js';
 import '@vaadin/vaadin-lumo-styles/sizing.js';
-import '@vaadin/vaadin-lumo-styles/spacing.js';
 import '@vaadin/vaadin-lumo-styles/style.js';
 import '@vaadin/vaadin-lumo-styles/font-icons.js';
 import '@vaadin/vaadin-lumo-styles/mixins/menu-overlay.js';
-import '@vaadin/vaadin-lumo-styles/mixins/field-button.js';
-import '@vaadin/vaadin-text-field/theme/lumo/vaadin-text-field.js';
-import '@vaadin/vaadin-item/theme/lumo/vaadin-item.js';
-import '@vaadin/vaadin-list-box/theme/lumo/vaadin-list-box.js';
+import '@vaadin/text-field/theme/lumo/vaadin-input-field-shared-styles.js';
 
 registerStyles(
   'vaadin-select',
   css`
-    :host {
-      outline: none;
-      -webkit-tap-highlight-color: transparent;
-    }
-
-    [selected] {
-      padding-left: 0;
-      padding-right: 0;
-      flex: auto;
+    :host(:not([theme*='align'])) ::slotted([slot='value']) {
+      text-align: start;
     }
 
-    :host([theme~='small']) [selected] {
-      padding: 0;
-      min-height: var(--lumo-size-s);
+    [part='input-field'] ::slotted([slot='value']) {
+      font-weight: 500;
     }
 
-    :host([theme~='align-left']) [selected] {
-      text-align: left;
+    /* placeholder styles */
+    :host(:not([has-value])) [part='input-field'] ::slotted([slot='value']) {
+      color: inherit;
+      transition: opacity 0.175s 0.1s;
+      opacity: 0.5;
     }
 
-    :host([theme~='align-right']) [selected] {
-      text-align: right;
-    }
-
-    :host([theme~='align-center']) [selected] {
-      text-align: center;
+    :host([has-value]) [part='input-field'] ::slotted([slot='value']) {
+      color: var(--lumo-body-text-color);
     }
 
     [part='toggle-button']::before {
@@ -49,51 +41,27 @@ registerStyles(
       color: var(--lumo-contrast-80pct);
     }
   `,
-  { include: ['lumo-field-button'], moduleId: 'lumo-select' }
+  { moduleId: 'lumo-select', include: ['lumo-input-field-shared-styles'] }
 );
 
 registerStyles(
-  'vaadin-select-text-field',
+  'vaadin-select-value-button',
   css`
-    :host([theme~='align-center']) ::slotted([part~='value']) {
-      --_lumo-text-field-overflow-mask-image: linear-gradient(to left, transparent 0.25em, #000 1.5em);
-    }
-
-    :host([theme~='align-center']) ::slotted([part~='value']) {
-      --_lumo-text-field-overflow-mask-image: none;
-    }
-
-    :host([theme~='align-right']) ::slotted([part~='value']) {
-      --_lumo-text-field-overflow-mask-image: linear-gradient(to right, transparent 0.25em, #000 1.5em);
-    }
-
-    [part='input-field'] {
-      cursor: default;
-    }
-
-    [part='input-field'] ::slotted([part='value']) {
-      display: flex;
-    }
-
-    [part='input-field']:focus {
-      outline: none;
-    }
-
-    /* RTL specific styles */
-    :host([theme~='align-left'][dir='rtl']) ::slotted([part~='value']) {
-      --_lumo-text-field-overflow-mask-image: linear-gradient(to left, transparent 0.25em, #000 1.5em);
+    :host {
+      min-height: var(--lumo-size-m);
+      padding: 0 0.25em;
     }
 
-    :host([theme~='align-center'][dir='rtl']) ::slotted([part~='value']) {
-      --_lumo-text-field-overflow-mask-image: none;
+    :host::before,
+    :host::after {
+      display: none;
     }
 
-    :host([dir='rtl']) ::slotted([part~='value']),
-    :host([theme~='align-right'][dir='rtl']) ::slotted([part~='value']) {
-      --_lumo-text-field-overflow-mask-image: linear-gradient(to right, transparent 0.25em, #000 1.5em);
+    :host([focus-ring]) {
+      box-shadow: none;
     }
   `,
-  { moduleId: 'lumo-select-text-field' }
+  { moduleId: 'lumo-select-value-button' }
 );
 
 registerStyles(

Changes in the Material theme

diff --git a/packages/select/theme/material/vaadin-select-styles.js b/packages/select/theme/material/vaadin-select-styles.js
index ccc5c3bf8..0f369752d 100644
--- a/packages/select/theme/material/vaadin-select-styles.js
+++ b/packages/select/theme/material/vaadin-select-styles.js
@@ -1,11 +1,12 @@
+/**
+ * @license
+ * Copyright (c) 2021 Vaadin Ltd.
+ * This program is available under Apache License Version 2.0, available at https://vaadin.com/license/
+ */
 import { registerStyles, css } from '@vaadin/vaadin-themable-mixin/register-styles.js';
-import '@vaadin/vaadin-item/theme/material/vaadin-item.js';
-import '@vaadin/vaadin-list-box/theme/material/vaadin-list-box.js';
-import '@vaadin/vaadin-text-field/theme/material/vaadin-text-field.js';
-import '@vaadin/vaadin-material-styles/color.js';
 import '@vaadin/vaadin-material-styles/font-icons.js';
 import '@vaadin/vaadin-material-styles/mixins/menu-overlay.js';
-import '@vaadin/vaadin-material-styles/mixins/field-button.js';
+import '@vaadin/text-field/theme/material/vaadin-input-field-shared-styles.js';
 
 registerStyles(
   'vaadin-select',
@@ -15,6 +16,17 @@ registerStyles(
       -webkit-tap-highlight-color: transparent;
     }
 
+    /* placeholder styles */
+    :host(:not([has-value])) [part='input-field'] ::slotted([slot='value']) {
+      color: var(--material-disabled-text-color);
+      transition: opacity 0.175s 0.1s;
+      opacity: 1;
+    }
+
+    :host([has-value]) [part='input-field'] ::slotted([slot='value']) {
+      color: var(--material-body-text-color);
+    }
+
     [part='toggle-button']::before {
       content: var(--material-icons-dropdown);
     }
@@ -27,31 +39,24 @@ registerStyles(
       pointer-events: none;
     }
   `,
-  { include: ['material-field-button'], moduleId: 'material-select' }
+  { moduleId: 'material-select', include: ['material-input-field-shared-styles'] }
 );
 
 registerStyles(
-  'vaadin-select-text-field',
+  'vaadin-select-value-button',
   css`
     :host {
-      width: 100%;
-    }
-
-    :host([disabled]) [part='input-field'],
-    [part='input-field'],
-    [part='value'] {
-      cursor: default;
-    }
-
-    [part='input-field']:focus {
-      outline: none;
+      font: inherit;
+      letter-spacing: normal;
+      text-transform: none;
     }
 
-    ::slotted([part='value']) {
-      display: flex;
+    :host::before,
+    :host::after {
+      display: none;
     }
   `,
-  { moduleId: 'material-select-text-field' }
+  { moduleId: 'material-select-value-button' }
 );
 
 registerStyles(

@web-padawan
Copy link
Member Author

Confirmed that Flow component ITs pass with the following change in SelectElement implementation:

diff --git a/vaadin-select-flow-parent/vaadin-select-testbench/src/main/java/com/vaadin/flow/component/select/testbench/SelectElement.java b/vaadin-select-flow-parent/vaadin-select-testbench/src/main/java/com/vaadin/flow/component/select/testbench/SelectElement.java
index 1917ff160..3f73b48d6 100644
--- a/vaadin-select-flow-parent/vaadin-select-testbench/src/main/java/com/vaadin/flow/component/select/testbench/SelectElement.java
+++ b/vaadin-select-flow-parent/vaadin-select-testbench/src/main/java/com/vaadin/flow/component/select/testbench/SelectElement.java
@@ -108,11 +108,7 @@ public class SelectElement extends TestBenchElement
     }
 
     public ItemElement getSelectedItem() {
-        TestBenchElement textFieldElement = $("vaadin-select-text-field")
-                .first();
-        TestBenchElement itemSlotElement = wrapElement(
-                textFieldElement.findElement(By.xpath("div[@part=\"value\"]")),
-                getCommandExecutor());
-        return itemSlotElement.$(ItemElement.class).first();
+        TestBenchElement valueElement = $("vaadin-select-value-button").first();
+        return valueElement.$(ItemElement.class).first();
     }
 }

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 22.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y phase 1 First batch of accessibility fixes a11y Accessibility issue Released with Platform 22.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate vaadin-select to use new base class Add select base class using new mixins
4 participants