feat(form-field): support native select element #12707

vivian-hu-zz · 2018-08-16T22:55:25Z

No description provided.

jelbourn · 2018-08-17T00:17:58Z

src/lib/form-field/form-field.md

@@ -8,6 +8,7 @@ In this document, "form field" refers to the wrapper component `<mat-form-field>

 The following Angular Material components are designed to work inside a `<mat-form-field>`:
 * [`<input matInput>` &amp; `<textarea matInput>`](https://material.angular.io/components/input/overview)
+* [`<select matInput>`](https://material.angular.io/components/select/overview)


Should this be <select matNativeSelect>?

(here and elsewhere)

Now that I'm thinking about it, we could deprecate matInput and make the selector matControl work for both... @mmalerba thoughts?

Maybe matFormControl for consistency with mat-form-field?

I'd like it to be something short, matInput,matSelect, and matControl are all fine by me

after team discussion, decided to go with matNativeControl

Still need to update these lines

jelbourn · 2018-08-17T00:19:52Z

src/lib/input/input.scss

+// Remove select button from IE11
+select::-ms-expand {
+  display: none;
+}


This will target all <select> elements; it should be restricted to a CSS class that's applied by the directive

jelbourn · 2018-08-17T00:20:05Z

src/lib/input/input.scss

+  display: none;
+}
+
+select[matInput] {


Prefer CSS class selectors to attribute selectors

jelbourn · 2018-08-17T00:20:36Z

src/lib/input/input.scss

+  -webkit-appearance: none;
+  position: relative;
+  background-color: transparent;
+  background-image: url('data:image/svg+xml;charset=utf8,%3Csvg%20width%3D%2210%22%20height%3D%225%22%20viewBox%3D%227%2010%2010%205%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%3E%3Cpath%20fill%3D%22%230%22%20fill-rule%3D%22evenodd%22%20opacity%3D%22.54%22%20d%3D%22M7%2010l5%205%205-5z%22%2F%3E%3C%2Fsvg%3E');


Can you extract the uri portion of this into a sass variable with a comment explaining what it is?

AFAIK we can't use data URIs in CSS. See #8898.

We couldn't think of another way to do this, so we checked directly with Google's security team. They said that they see no reason to ban data: URIs, so we can relax this restriction. I do want to add documentation that talks about our CSP support.

security team approved using data URI for image.

jelbourn · 2018-08-17T00:20:58Z

src/lib/input/input.scss

+  background-repeat: no-repeat;
+  display: inline-flex;
+  box-sizing: border-box;
+  background-position: right center;


Does this need to be flipped in RTL?

jelbourn · 2018-08-17T00:22:10Z

src/lib/input/input.ts

+    }
+  }
+
+  private shouldLabelFloatForSelect(selectElement:HTMLSelectElement): boolean {


Space after colon

jelbourn · 2018-08-17T00:22:44Z

src/lib/input/input.ts

+    }
+  }
+
+  private shouldLabelFloatForSelect(selectElement:HTMLSelectElement): boolean {


This method should be prefixed with an underscore (applies to any non-public api)

Maybe we should have another class only with the select functionality that extends MatInput? Otherwise we'll end up sprinkling if (isSelect) checks everywhere.

jelbourn · 2018-08-17T00:26:33Z

src/lib/select/select.md

@@ -1,8 +1,11 @@
+Angular material supports both native html select and `<mat-select>` to work inside a 


I would leave the original text for this file the same, and then add a new paragraph underneath that says something like

Angular Material also supports use of native the `<select>` element inside of `<mat-form-field>`. The native control has several performance, accessibility, and usability advantages. See [the documentation for form-field](https://material.angular.io/components/form-field) for more information.

jelbourn · 2018-08-17T00:27:21Z

src/lib/select/select.md

@@ -71,11 +74,13 @@ by setting the `multiple` property. This will allow the user to select multiple
 using the `<mat-select>` in multiple selection mode, its value will be a sorted list of all selected
 values rather than a single value.

+Multiple select with `<select>` is not recommended.


I would specifically use <select multiple> here

crisbeto · 2018-08-17T06:33:34Z

src/lib/form-field/form-field.md

@@ -8,6 +8,7 @@ In this document, "form field" refers to the wrapper component `<mat-form-field>

 The following Angular Material components are designed to work inside a `<mat-form-field>`:
 * [`<input matInput>` &amp; `<textarea matInput>`](https://material.angular.io/components/input/overview)
+* [`<select matInput>`](https://material.angular.io/components/select/overview)


Maybe matFormControl for consistency with mat-form-field?

crisbeto · 2018-08-17T06:36:23Z

src/lib/input/input.scss

+  -webkit-appearance: none;
+  position: relative;
+  background-color: transparent;
+  background-image: url('data:image/svg+xml;charset=utf8,%3Csvg%20width%3D%2210%22%20height%3D%225%22%20viewBox%3D%227%2010%2010%205%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%3E%3Cpath%20fill%3D%22%230%22%20fill-rule%3D%22evenodd%22%20opacity%3D%22.54%22%20d%3D%22M7%2010l5%205%205-5z%22%2F%3E%3C%2Fsvg%3E');


AFAIK we can't use data URIs in CSS. See #8898.

crisbeto · 2018-08-17T06:38:22Z

src/lib/input/input.spec.ts

@@ -572,6 +614,41 @@ describe('MatInput without forms', () => {
    expect(formFieldEl.classList).toContain('mat-form-field-should-float');
  }));

+  it('should floating labels when select has value', fakeAsync(() => {


"should floating" -> "should float"

crisbeto · 2018-08-17T06:39:18Z

src/lib/input/input.spec.ts

+    expect(formFieldEl.classList).toContain('mat-form-field-should-float');
+  }));
+
+  it('should not floating labels when select has no value, no option label, ' +


floating -> float

crisbeto · 2018-08-17T06:43:38Z

src/lib/input/input.ts

@@ -351,7 +350,26 @@ export class MatInput extends _MatInputMixinBase implements MatFormFieldControl<
   * Implemented as part of MatFormFieldControl.
   * @docs-private
   */
-  get shouldLabelFloat(): boolean { return this.focused || !this.empty; }
+  get shouldLabelFloat(): boolean {
+    if (this._elementRef.nativeElement.nodeName.toLowerCase() === 'select') {


Instead of doing this in a getter, the check can be done in the constructor since the element's nodeName won't change.

crisbeto · 2018-08-17T06:44:28Z

src/lib/input/input.ts

+    }
+  }
+
+  private shouldLabelFloatForSelect(selectElement:HTMLSelectElement): boolean {


Maybe we should have another class only with the select functionality that extends MatInput? Otherwise we'll end up sprinkling if (isSelect) checks everywhere.

crisbeto · 2018-08-17T06:46:39Z

src/lib/input/input.ts

+    if (selectElement.multiple) {
+      // For multi select, float mat input label
+      return true;
+    } else if (!this.empty || selectElement.options[0].label || selectElement.options[0].innerHTML) {


Since this is being called in a getter, it can be very expensive to keep checking innerHTML. We should either move away from checking the DOM or check textContent.

we can just use label to cover both. from the documentation "label
This attribute is text for the label indicating the meaning of the option. If the label attribute isn't defined, its value is that of the element text content."
And testing confirmed that.

I think this method can be simplified into one statement:

return selectedElement.multiple || this.focused || !this.empty || selectElement.options[0].label || selectElement.options[0].innerHTML;

crisbeto · 2018-08-17T06:48:14Z

src/lib/select/select.md

+Angular material supports both native html select and `<mat-select>` to work inside a 
+[`<mat-form-field>`] element.(https://material.angular.io/components/form-field/overview) element.
+
+To use native select inside `<mat-form-field>`, add `matInput` as a attribute to native html select. 


"as a" -> "as an"

vivian-hu-zz

addressed review comments.

After team discussion, decided to go with matNativeControl for input, textarea and select. Changed the existing input and textarea to use matNativeControl in a few places to make sure both matInput and matNativeControl works for input/textarea.

ngbot · 2018-08-20T21:35:01Z

Hi @vivian-hu! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

jelbourn · 2018-08-20T21:48:09Z

src/lib/input/input.scss

+  display: none;
+}
+
+//encoded material design select arrow svg


Comment should have a spacer after // and start with a capital letter:

// Drowndown arrow svg icon, url-encoded.

jelbourn · 2018-08-20T22:20:54Z

src/lib/input/input.scss

+$mat-native-select-arrow-svg: 'data:image/svg+xml;charset=utf8,%3Csvg%20width%3D%2210%22%20height%3D%225%22%20viewBox%3D%227%2010%2010%205%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%3E%3Cpath%20fill%3D%22%230%22%20fill-rule%3D%22evenodd%22%20opacity%3D%22.54%22%20d%3D%22M7%2010l5%205%205-5z%22%2F%3E%3C%2Fsvg%3E';
+/* stylelint-enable */
+
+select.mat-input-element {


Add a comment here that explains what this class is doing.

jelbourn · 2018-08-20T22:22:17Z

src/lib/input/input.ts

+      // For multi select, float mat input label
+      // If single select has value or has a option label or html, float mat input label to avoid
+      // mat input label to overlap with the select content.
+      const selectElement  = this._elementRef.nativeElement;


Extra space

jelbourn · 2018-08-20T22:23:18Z

src/lib/input/input.ts

+    if (this._isNativeSelect) {
+      // For multi select, float mat input label
+      // If single select has value or has a option label or html, float mat input label to avoid
+      // mat input label to overlap with the select content.


Line-break and capitalization is off. Also, should this say "For native select" instead of "For multi select"?

jelbourn · 2018-08-20T22:24:09Z

src/lib/input/input.ts

+      // mat input label to overlap with the select content.
+      const selectElement  = this._elementRef.nativeElement;
+      return selectElement.multiple || !this.empty || selectElement.options[0].label
+          || this.focused;


Prefer putting the operator on the previous line:

return selectElement.multiple || !this.empty || selectElement.options[0].label || this.focused;

jelbourn · 2018-08-20T22:24:30Z

src/lib/select/select.md

+and usability advantages. See [the documentation for
+form-field](https://material.angular.io/components/form-field) for more information.
+
+To use native select inside `<mat-form-field>`, add `matInput` as an attribute to native html select. 


Update selector here

jelbourn · 2018-08-20T22:26:15Z

src/lib/select/select.md

-The select component has `role="listbox"` and options inside select have `role="option"`.
+The `<mat-select>` component has `role="listbox"` and options inside select have `role="option"`.
+
+The `<select>` offers better accessibility by supporting all the native html accessibility capability.


I would rephrase this

The native `<select>` offers the best accessibility because it is supported directly by screen-readers.

vivian-hu-zz

addressed code review comments.

jelbourn · 2018-08-21T20:56:29Z

src/lib/form-field/form-field.md

@@ -8,6 +8,7 @@ In this document, "form field" refers to the wrapper component `<mat-form-field>

 The following Angular Material components are designed to work inside a `<mat-form-field>`:
 * [`<input matInput>` &amp; `<textarea matInput>`](https://material.angular.io/components/input/overview)
+* [`<select matInput>`](https://material.angular.io/components/select/overview)


Still need to update these lines

jelbourn · 2018-08-21T20:56:52Z

src/lib/form-field/form-field.md

@@ -41,7 +42,8 @@ want a floating label, add a `<mat-label>` to the `mat-form-field`.
 ### Floating label

 The floating label is a text label displayed on top of the form field control when
-the control does not contain any text. By default, when text is present the floating label
+the control does not contain any text or when `<select matInput>` does not show any option text. 


Still needs updated selector

jelbourn · 2018-08-21T20:58:53Z

src/lib/input/input.scss

+// Remove select button from IE11
+select.mat-input-element::-ms-expand {
+  display: none;
+}


You can move this into the rule below like this:

select.mat-input-element { &::-ms-expand { display: none; } -moz-appearance: none; ... }

jelbourn · 2018-08-21T21:01:57Z

src/lib/input/input.spec.ts

+
+    const formFieldEl =
+      fixture.debugElement.query(By.css('.mat-form-field')).nativeElement;
+    const inputEl = fixture.debugElement.query(By.css('select')).nativeElement;


inputEl -> selectElement (or selectEl) here and other places where it is a select element and not an input

jelbourn · 2018-08-21T21:11:48Z

src/lib/input/input.ts

+    if (this._isNativeSelect) {
+      // If multi select, float mat-label.
+      // If single select with value, with an option label or html, float mat-label to avoid
+      // it overlapping with select content.


How about

// For a single-selection `<select>`, the label should float when the selected option has a non-empty // display value. For a `<select multiple>`, the label *always* floats to avoid overlapping the label // with the options.

jelbourn · 2018-08-21T21:13:03Z

src/lib/select/select.md

+and usability advantages. See [the documentation for
+form-field](https://material.angular.io/components/form-field) for more information.
+
+To use native select inside `<mat-form-field>`, add `matNativeControl` as an attribute to native html select. 


To use a native select inside `<mat-form-field>`, add the `matNativeControl` attribute to the `<select>` element.

jelbourn · 2018-08-21T21:13:55Z

src/lib/select/select.md

@@ -45,7 +52,7 @@ In some cases that `<mat-form-field>` may use the placeholder as the label (see
 ### Disabling the select or individual options

 It is possible to disable the entire select or individual options in the select by using the
-disabled property on the `<mat-select>` and the `<mat-option>` components respectively.
+disabled property on the `<select>` or `<mat-select>` and the `<option>` or <mat-option>` components respectively.


Change components here to elements since <option> is not a component.

jelbourn · 2018-08-21T22:13:04Z

src/lib/select/select.md

@@ -71,11 +78,13 @@ by setting the `multiple` property. This will allow the user to select multiple
 using the `<mat-select>` in multiple selection mode, its value will be a sorted list of all selected
 values rather than a single value.

+Multiple select with `<select multiple>` is not recommended.


How about

Using multiple selection with a native select element (`<select multiple>`) is discouraged inside `<mat-form-field>`, as the inline listbox appearance is inconsistent with other Material Design components.

vivian-hu-zz

Update based on review feedback

jelbourn · 2018-08-22T20:29:23Z

src/lib/form-field/form-field.md

@@ -8,6 +8,7 @@ In this document, "form field" refers to the wrapper component `<mat-form-field>

 The following Angular Material components are designed to work inside a `<mat-form-field>`:
 * [`<input matInput>` &amp; `<textarea matInput>`](https://material.angular.io/components/input/overview)
+* [`<select matNativeControl>`](https://material.angular.io/components/select/overview)


Should also update the line above:
<input matNativeControl> and <textarea matNativeControl>

jelbourn · 2018-08-22T20:29:53Z

src/lib/input/input.spec.ts

+    fixture.detectChanges();
+
+    const formFieldEl =
+      fixture.debugElement.query(By.css('.mat-form-field')).nativeElement;


Just noticed this, but line overflow indent should be +4 rather than +2
(here and elsewhere)

jelbourn · 2018-08-22T20:31:03Z

src/lib/select/select.md

@@ -8,6 +8,14 @@ To add options to the select, add `<mat-option>` elements to the `<mat-select>`.
 has a `value` property that can be used to set the value that will be selected if the user chooses
 this option. The content of the `<mat-option>` is what will be shown to the user.

+Angular Material also supports use of native `<select>` element inside of


of native `<select>` element -> of the native `<select>` element

vivian-hu-zz

fixed indentation and updated selector name for input and textarea.

jelbourn

LGTM

jelbourn · 2018-08-22T23:41:29Z

Commit message should be:

feat(form-field): support native select element

ngbot · 2018-08-23T20:33:43Z

Hi @vivian-hu! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

vivian-hu-zz

changed the way to set "Readonly" attribute.

angular-automatic-lock-bot · 2019-09-10T10:03:03Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

vivian-hu-zz requested review from amcdnl, crisbeto, jelbourn and mmalerba as code owners August 16, 2018 22:55

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 16, 2018

vivian-hu-zz changed the title ~~List2~~ feat(input): add native select to be form input. Aug 16, 2018

jelbourn reviewed Aug 17, 2018

View reviewed changes

crisbeto reviewed Aug 17, 2018

View reviewed changes

josephperrott added the target: minor This PR is targeted for the next minor release label Aug 17, 2018

vivian-hu-zz force-pushed the list2 branch from 6241113 to ee775a8 Compare August 20, 2018 21:17

vivian-hu-zz commented Aug 20, 2018

View reviewed changes

vivian-hu-zz force-pushed the list2 branch from ee775a8 to 80f0f42 Compare August 20, 2018 21:36

jelbourn reviewed Aug 20, 2018

View reviewed changes

vivian-hu-zz force-pushed the list2 branch from 80f0f42 to 8f477c3 Compare August 21, 2018 18:14

vivian-hu-zz commented Aug 21, 2018

View reviewed changes

jelbourn reviewed Aug 21, 2018

View reviewed changes

vivian-hu-zz force-pushed the list2 branch from 8f477c3 to 46c2056 Compare August 22, 2018 18:49

vivian-hu-zz commented Aug 22, 2018

View reviewed changes

jelbourn reviewed Aug 22, 2018

View reviewed changes

vivian-hu-zz force-pushed the list2 branch 2 times, most recently from 75b57d4 to 47fcaf8 Compare August 22, 2018 21:39

vivian-hu-zz commented Aug 22, 2018

View reviewed changes

jelbourn approved these changes Aug 22, 2018

View reviewed changes

vivian-hu-zz changed the title ~~feat(input): add native select to be form input.~~ feat(form-field): support native select element Aug 23, 2018

vivian-hu-zz force-pushed the list2 branch from 47fcaf8 to 4117aa0 Compare August 27, 2018 18:17

Vivian Hu added 2 commits August 27, 2018 16:42

update dependency

21d4c42

feat(input): add native select to be form input.

4c8448a

vivian-hu-zz force-pushed the list2 branch 2 times, most recently from 63f07ca to 4e04c81 Compare August 27, 2018 23:55

feat(input): add native select to be form input

371a373

vivian-hu-zz force-pushed the list2 branch from 4e04c81 to 371a373 Compare August 28, 2018 01:06

vivian-hu-zz commented Aug 28, 2018

View reviewed changes

vivian-hu-zz added the P2 The issue is important to a large percentage of users, with a workaround label Aug 28, 2018

kwkelly mentioned this pull request Aug 28, 2018

feat(select): add mat-select-header component #7835

Closed

jelbourn removed the merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed label Aug 30, 2018

jelbourn merged commit 4e41985 into angular:master Aug 30, 2018

vivian-hu-zz mentioned this pull request Sep 4, 2018

Support native <select> in mat-form-field #11971

Closed

vivian-hu-zz deleted the list2 branch January 23, 2019 23:00

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019

		@@ -1,8 +1,11 @@
		Angular material supports both native html select and `<mat-select>` to work inside a

feat(form-field): support native select element #12707

feat(form-field): support native select element #12707

Conversation

vivian-hu-zz commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivian-hu-zz left a comment

Choose a reason for hiding this comment

ngbot bot commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivian-hu-zz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivian-hu-zz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivian-hu-zz left a comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn commented Aug 22, 2018

ngbot bot commented Aug 23, 2018

vivian-hu-zz left a comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 10, 2019