Skip to content

fix(select): slotted label content works with outline notch #27483

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

Merged
merged 29 commits into from
May 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
8bcfebb
feat(select): add label slot
liamdebeasi May 9, 2023
186d759
fix(select): label slot content is passed as default title for alert
liamdebeasi May 9, 2023
8acbec5
lint
liamdebeasi May 9, 2023
27d1cd1
fix(select): slotted content can truncate
liamdebeasi May 10, 2023
3bf0b70
test(select): add tests to verify prop/slot behavior
liamdebeasi May 12, 2023
45ea21d
test(select): verify alert header slot behavior
liamdebeasi May 12, 2023
1885823
fix(select): label is hidden if no content passed
liamdebeasi May 12, 2023
b1bb1f8
clarify getters
liamdebeasi May 12, 2023
3974407
test(select): verify header works with prop and slot
liamdebeasi May 12, 2023
99006c4
remove only
liamdebeasi May 12, 2023
02a430a
fix(select): pass correct value to aria-label
liamdebeasi May 12, 2023
9d5e82e
fix(select): floating label covers placeholder when when blurred (#27…
liamdebeasi May 12, 2023
c253b8f
add poc for notch
liamdebeasi May 15, 2023
c239572
add IO test
liamdebeasi May 15, 2023
0b39538
sync
liamdebeasi May 16, 2023
99bd9e1
Merge remote-tracking branch 'origin/FW-3532' into 3532-notch
liamdebeasi May 16, 2023
3cb5416
chore: clean up code, add comments
liamdebeasi May 16, 2023
6153c60
test(select): test notch
liamdebeasi May 16, 2023
5d8436a
test(select): test IO behavior too
liamdebeasi May 16, 2023
55d3286
revert old file change
liamdebeasi May 16, 2023
0f7b642
add ssr check for IO
liamdebeasi May 16, 2023
2e87f71
Update index.html
liamdebeasi May 16, 2023
ce272df
perf(select): avoid duplicate IOs
liamdebeasi May 16, 2023
122ffe1
chore(): add updated snapshots
Ionitron May 16, 2023
cb556c1
fix(select): still work if position: fixed
liamdebeasi May 16, 2023
53d764c
update comment
liamdebeasi May 16, 2023
13a6547
formatting
liamdebeasi May 22, 2023
714f8c4
perf(select): only call raf when needed
liamdebeasi May 23, 2023
2339727
remove log
liamdebeasi May 23, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 149 additions & 5 deletions core/src/components/select/select.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core';
import { Component, Element, Event, Host, Method, Prop, State, Watch, h, forceUpdate } from '@stencil/core';
import type { LegacyFormController } from '@utils/forms';
import { createLegacyFormController } from '@utils/forms';
import { findItemLabel, focusElement, getAriaLabel, renderHiddenInput, inheritAttributes } from '@utils/helpers';
import { findItemLabel, focusElement, getAriaLabel, renderHiddenInput, inheritAttributes, raf } from '@utils/helpers';
import type { Attributes } from '@utils/helpers';
import { printIonWarning } from '@utils/logging';
import { actionSheetController, alertController, popoverController } from '@utils/overlays';
import type { OverlaySelect } from '@utils/overlays-interface';
import { isRTL } from '@utils/rtl';
import { createColorClasses, hostContext } from '@utils/theme';
import { watchForOptions } from '@utils/watch-options';
import { win } from '@utils/window';
import { caretDownSharp, chevronExpand } from 'ionicons/icons';

import { getIonMode } from '../../global/ionic-global';
Expand Down Expand Up @@ -55,6 +56,8 @@ export class Select implements ComponentInterface {
private legacyFormController!: LegacyFormController;
private inheritedAttributes: Attributes = {};
private nativeWrapperEl: HTMLElement | undefined;
private notchSpacerEl: HTMLElement | undefined;
private notchVisibilityIO: IntersectionObserver | undefined;

// This flag ensures we log the deprecation warning at most once.
private hasLoggedDeprecationWarning = false;
Expand Down Expand Up @@ -665,13 +668,13 @@ export class Select implements ComponentInterface {
* was passed.
*/
private get labelText() {
const { el, label } = this;
const { label } = this;

if (label !== undefined) {
return label;
}

const labelSlot = el.querySelector('[slot="label"]');
const { labelSlot } = this;

if (labelSlot !== null) {
return labelSlot.textContent;
Expand Down Expand Up @@ -740,14 +743,155 @@ export class Select implements ComponentInterface {
);
}

componentDidRender() {
if (this.needsExplicitNotchWidth()) {
/**
* Run this the frame after
* the browser has re-painted the select.
* Otherwise, the label element may have a width
* of 0 and the IntersectionObserver will be used.
*/
raf(() => {
this.setNotchWidth();
});
}
}

/**
* Gets any content passed into the `label` slot,
* not the <slot> definition.
*/
private get labelSlot() {
return this.el.querySelector('[slot="label"]');
}

/**
* Returns `true` if label content is provided
* either by a prop or a content. If you want
* to get the plaintext value of the label use
* the `labelText` getter instead.
*/
private get hasLabel() {
return this.label !== undefined || this.el.querySelector('[slot="label"]') !== null;
return this.label !== undefined || this.labelSlot !== null;
}

private needsExplicitNotchWidth() {
if (
/**
* If the notch is not being used
* then we do not need to set the notch width.
*/
this.notchSpacerEl === undefined ||
/**
* If no label is being used, then we
* do not need to estimate the notch width.
*/
!this.hasLabel ||
/**
* If the label property is being used
* then we can render the label text inside
* of the notch and let the browser
* determine the notch size for us.
*/
this.label !== undefined
) {
return false;
}

return true;
}

/**
* When using a label prop we can render
* the label value inside of the notch and
* let the browser calculate the size of the notch.
* However, we cannot render the label slot in multiple
* places so we need to manually calculate the notch dimension
* based on the size of the slotted content.
*
* This function should only be used to set the notch width
* on slotted label content. The notch width for label prop
* content is automatically calculated based on the
* intrinsic size of the label text.
*/
private setNotchWidth() {
const { el, notchSpacerEl } = this;

if (notchSpacerEl === undefined) {
return;
}

if (!this.needsExplicitNotchWidth()) {
notchSpacerEl.style.removeProperty('width');
return;
}

const width = this.labelSlot!.scrollWidth;
if (
/**
* If the computed width of the label is 0
* and notchSpacerEl's offsetParent is null
* then that means the element is hidden.
* As a result, we need to wait for the element
* to become visible before setting the notch width.
*
* We do not check el.offsetParent because
* that can be null if ion-select has
* position: fixed applied to it.
* notchSpacerEl does not have position: fixed.
*/
width === 0 &&
notchSpacerEl.offsetParent === null &&
win !== undefined &&
'IntersectionObserver' in win
) {
/**
* If there is an IO already attached
* then that will update the notch
* once the element becomes visible.
* As a result, there is no need to create
* another one.
*/
if (this.notchVisibilityIO !== undefined) {
return;
}

const io = (this.notchVisibilityIO = new IntersectionObserver(
(ev) => {
/**
* If the element is visible then we
* can try setting the notch width again.
*/
if (ev[0].intersectionRatio === 1) {
this.setNotchWidth();
io.disconnect();
this.notchVisibilityIO = undefined;
}
},
/**
* Set the root to be the select
* This causes the IO callback
* to be fired in WebKit as soon as the element
* is visible. If we used the default root value
* then WebKit would only fire the IO callback
* after any animations (such as a modal transition)
* finished, and there would potentially be a flicker.
*/
{ threshold: 0.01, root: el }
));

io.observe(notchSpacerEl);
return;
}

/**
* If the element is visible then we can set the notch width.
* The notch is only visible when the label is scaled,
* which is why we multiply the width by 0.75 as this is
* the same amount the label element is scaled by in the
* select CSS (See $select-floating-label-scale in select.vars.scss).
*/
notchSpacerEl.style.setProperty('width', `${width * 0.75}px`);
}

/**
Expand All @@ -770,7 +914,7 @@ export class Select implements ComponentInterface {
<div class="select-outline-container">
<div class="select-outline-start"></div>
<div class="select-outline-notch">
<div class="notch-spacer" aria-hidden="true">
<div class="notch-spacer" aria-hidden="true" ref={(el) => (this.notchSpacerEl = el)}>
{this.label}
</div>
</div>
Expand Down
58 changes: 58 additions & 0 deletions core/src/components/select/test/fill/select.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ configs({ modes: ['md'] }).forEach(({ title, screenshot, config }) => {
const select = page.locator('ion-select');
expect(await select.screenshot()).toMatchSnapshot(screenshot(`select-fill-outline-label-floating`));
});

test('should not have visual regressions with shaped outline', async ({ page }) => {
await page.setContent(
`
Expand Down Expand Up @@ -167,3 +168,60 @@ configs({ modes: ['md'] }).forEach(({ title, screenshot, config }) => {
});
});
});

configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
test.describe(title('select: label slot'), () => {
test('should render the notch correctly with a slotted label', async ({ page }) => {
await page.setContent(
`
<style>
.custom-label {
font-size: 30px;
}
</style>
<ion-select
fill="outline"
label-placement="stacked"
value="apple"
>
<div slot="label" class="custom-label">My Label Content</div>
<ion-select-option value="apple">Apple</ion-select-option>
</ion-select>
`,
config
);

const select = page.locator('ion-select');
expect(await select.screenshot()).toMatchSnapshot(screenshot(`select-fill-outline-slotted-label`));
});
test('should render the notch correctly with a slotted label after the select was originally hidden', async ({
page,
}) => {
await page.setContent(
`
<style>
.custom-label {
font-size: 30px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting this style on a floating label causes the label to render too low when the select is closed:

weird placement

Not sure if this is best addressed in another PR, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is another tricky one. The problem here is we are translating based on %, which will be based on the height of the label element. If the label element grows in height so will the translation.

MDC "fixes" this by giving the label a fixed height. However, this causes the text to get cut off at larger font sizes. This also causes the label to no longer be centered in the container as the select increases in height, though I'm not 100% if that's by design.

We could also make the label height 100%, but then we're going to need to translate by seemingly arbitrary values that may also be wrong as the select changes in height.

Any thoughts on how best to address? I'll keep testing it. It's worth noting that Material Design doesn't seem to do a lot of customizations on floating/stacked labels beyond colors and such. Perhaps this is by design?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to get pretty close by changing the following styles on .label-text-wrapper (screenshots linked):

  • In the default styles, replace the current transform with top: 50% and transform: translateY(-50%) scale(1). The top: 50/translateY(-50) is a trick I've used to vertically center absolutely positioned items.
  • When floating, add top: auto.

The animation doesn't work right, but maybe there's a good way to fix it? An alternative could be to not do position: absolute at all and use flex styles to vertically center the label instead, maybe only when the label isn't floating if the position is necessary to make the float work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During refinement we discussed that this is the same problem as https://ionic-cloud.atlassian.net/browse/FW-4083, so we are going to handle this fix in a separate ticket.

}
</style>
<ion-select
fill="outline"
label-placement="stacked"
value="apple"
style="display: none"
>
<div slot="label" class="custom-label">My Label Content</div>
<ion-select-option value="apple">Apple</ion-select-option>
</ion-select>
`,
config
);

const select = page.locator('ion-select');

await select.evaluate((el: HTMLIonSelectElement) => el.style.removeProperty('display'));

expect(await select.screenshot()).toMatchSnapshot(screenshot(`select-fill-outline-hidden-slotted-label`));
});
});
});
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.