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

bug: select, long text does not truncate #27081

Closed
3 tasks done
matthew2564 opened this issue Apr 1, 2023 · 38 comments · Fixed by #27125
Closed
3 tasks done

bug: select, long text does not truncate #27081

matthew2564 opened this issue Apr 1, 2023 · 38 comments · Fixed by #27125
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@matthew2564
Copy link

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

Ionic now suggests adding a label or aria-label to tags such as ion-select which is highlighted via this console warning

[Ionic Warning]: ion-select now requires providing a label with either the "label" property or the "aria-label" attribute. To migrate, remove any usage of "ion-label" and pass the label text to either the "label" property or the "aria-label" attribute.

Example: <ion-select label="Favorite Color">...</ion-select>
Example with aria-label: <ion-select aria-label="Favorite Color">...</ion-select>

Upon adding an aria-label attribute to the ion-select, the positioning of the up / down carets on the right hand side comes inwards. Here is a basic code example

<ion-header [translucent]="true">
  <ion-toolbar>
    <ion-title>
      Blank
    </ion-title>
  </ion-toolbar>
</ion-header>

<ion-content [fullscreen]="true">
    <ion-row style="border: 3px solid black;">

      <ion-col class="ion-align-self-center" style="border: 3px solid red;" size="4">
        <label>Fruit</label>
      </ion-col>

      <ion-col>
        <ion-select placeholder="Select a fruit" fill="outline" aria-label="Fruit" style="border: 3px solid blue;">
          <ion-select-option value="apple">Apple</ion-select-option>
          <ion-select-option value="banana">Banana</ion-select-option>
          <ion-select-option value="orange">Orange</ion-select-option>
        </ion-select>
      </ion-col>

    </ion-row>
</ion-content>

The result of that being...

Screenshot 2023-04-01 at 15 58 18

Expected Behavior

I would expect the position of the carets to remain the same as without the aria-label attribute.

In the below example, the carets remain in the position expected.

<ion-header [translucent]="true">
  <ion-toolbar>
    <ion-title>
      Blank
    </ion-title>
  </ion-toolbar>
</ion-header>

<ion-content [fullscreen]="true">
    <ion-row style="border: 3px solid black;">

      <ion-col class="ion-align-self-center" style="border: 3px solid red;" size="4">
        <label id="fruit-label">Fruit</label>
      </ion-col>

      <ion-col>
        <ion-select placeholder="Select a fruit" fill="outline" style="border: 3px solid blue;">
          <ion-select-option value="apple">Apple</ion-select-option>
          <ion-select-option value="banana">Banana</ion-select-option>
          <ion-select-option value="orange">Orange</ion-select-option>
        </ion-select>
      </ion-col>

    </ion-row>
</ion-content>

The result of that being...

Screenshot 2023-04-01 at 15 58 27

Steps to Reproduce

  1. Add snippet provided above in the Current Behaviour section

Code Reproduction URL

No response

Ionic Info

Ionic:

   Ionic CLI                     : 6.20.8 (/opt/homebrew/lib/node_modules/@ionic/cli)
   Ionic Framework               : not installed
   @angular-devkit/build-angular : 15.2.4
   @angular-devkit/schematics    : 15.2.4
   @angular/cli                  : 15.2.4
   @ionic/angular-toolkit        : 9.0.0

Capacitor:

   Capacitor CLI      : 4.7.3
   @capacitor/android : not installed
   @capacitor/core    : 4.7.3
   @capacitor/ios     : not installed

Utility:

   cordova-res : not installed globally
   native-run  : 1.7.2

System:

   NodeJS : v18.10.0 (/Users/matt/.n/bin/node)
   npm    : 8.19.2
   OS     : macOS Unknown

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Apr 1, 2023
@judsonmusic
Copy link

judsonmusic commented Apr 2, 2023

@matthew2564 I was struggling with this today. I had to do a workaround by adding labelPlacement="stacked" hope this helps.

image

Fixed:

image

@matthew2564
Copy link
Author

matthew2564 commented Apr 2, 2023

Thanks for the workaround @judsonmusic, however there are further issues off the back of this.

<ion-content [fullscreen]="true">
    <ion-row style="border: 3px solid black;">
      <ion-col class="ion-align-self-center" style="border: 3px solid red;" size="4">
        <label id="fruit-label">Fruit</label>
      </ion-col>

      <ion-col size="3">
        <ion-select
          placeholder="Select a fruit"
          aria-label="fruit"
          labelPlacement="stacked"
          multiple="true"
          fill="outline"
          style="border: 3px solid blue;"
        >
          <ion-select-option value="apple">Apple</ion-select-option>
          <ion-select-option value="banana">Banana</ion-select-option>
          <ion-select-option value="orange">Orange</ion-select-option>
        </ion-select>
      </ion-col>
    </ion-row>
</ion-content>

This achieves the desired result mentioned above with the output of
Screenshot 2023-04-02 at 10 01 56

However, if the length of the selected value exceeds that of the ion-select area, there is no longer an ellipsis / ... to truncate the value, this is also a problem when you have multiple selectable options. See below (following code snippet is the same as the one above).

<ion-content [fullscreen]="true">
    <ion-row style="border: 3px solid black;">
      <ion-col class="ion-align-self-center" style="border: 3px solid red;" size="4">
        <label id="fruit-label">Fruit</label>
      </ion-col>

      <ion-col size="3">
        <ion-select
          placeholder="Select a fruit"
          aria-label="fruit"
          labelPlacement="stacked"
          multiple="true"
          fill="outline"
          style="border: 3px solid blue;"
        >
          <ion-select-option value="apple">Apple</ion-select-option>
          <ion-select-option value="banana">Banana</ion-select-option>
          <ion-select-option value="orange">Orange</ion-select-option>
        </ion-select>
      </ion-col>
    </ion-row>
</ion-content>

When selecting all 3 items, this is how it is rendered (where the carets overlay the selected values)
Screenshot 2023-04-02 at 10 01 50

With only the removal of the aria-label attribute or the labelPlacement="stacked", the ellipsis is restored like so
Screenshot 2023-04-02 at 10 02 21

In regards to the ion-select itself, it is frustrating that the aria-labelledby does not suppress the warning. As a result, I'm now having to re-label each ion-select instead of pointing it at the label associated.

@liamdebeasi liamdebeasi self-assigned this Apr 3, 2023
@liamdebeasi
Copy link
Contributor

Thanks for the report. Adding aria-label or label opts in to the new form control syntax which has a different architecture under the hood. The big difference is ion-select contains both the label and the form control. So in this case, you should be using the label property on ion-select instead of using a separate <label> and an aria-label.

Here is an example: https://codepen.io/liamdebeasi/pen/yLxmqdR

You can control the order of the label/control using labelPlacement. You can also control how the label and control are packed on a line by using the justify property.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Apr 3, 2023
@liamdebeasi liamdebeasi removed their assignment Apr 3, 2023
@ionitron-bot ionitron-bot bot removed the triage label Apr 3, 2023
@judsonmusic
Copy link

@liamdebeasi can you provide an example using the justify property?

Also, my company uses position stacked for everything so it might be worthwhile making sure the docs are updated for this as well. I created a ticket based on the fact that the stacked label for the ion select is off by a few pixels. Just FYI compared to the ion input.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Apr 3, 2023
@liamdebeasi
Copy link
Contributor

Sure, we have some playgrounds that show how the justify property works: https://ionicframework.com/docs/api/select#justification

Also, my company uses position stacked for everything so it might be worthwhile making sure the docs are updated for this as well. I created a ticket based on the fact that the stacked label for the ion select is off by a few pixels. Just FYI compared to the ion input.

This is #27086, correct?

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Apr 3, 2023
@ionitron-bot ionitron-bot bot removed the triage label Apr 3, 2023
@judsonmusic
Copy link

judsonmusic commented Apr 3, 2023 via email

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Apr 3, 2023
@liamdebeasi
Copy link
Contributor

Those issues have been labeled as bugs, but they have not been fixed yet. However, once the fixes merge we will post on each thread.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Apr 3, 2023
@ionitron-bot ionitron-bot bot removed the triage label Apr 3, 2023
@judsonmusic
Copy link

@liamdebeasi yes, that is correct. I have two other bugs file that are very similar and it looks like they were indeed marked as bugs. At what point will these be released or how will I know so I can pull latest on ionic? Also, I don't know if you'd be willing to do it but I have been doing ionic since it came out. I'm an architect. I was wondering if it's possible to screen share and show you a couple of things before I actually post potential bugs for them. Let me know if that's a possibility you can email me at [email protected] I am on LinkedIn under Judson Terrell.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Apr 3, 2023
@liamdebeasi
Copy link
Contributor

We don't provide public timelines on bug fixes since those timelines sometimes change. However, once a bug fix has been merged into main it is usually (but not always) released in the next patch release. We do patch releases once a week.

I'm fairly active on Discord if you wanted to ask your questions there: https://discord.com/invite/UPYYRhtyzp. Otherwise, feel free to open separate issues here on GitHub.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Apr 3, 2023
@ionitron-bot ionitron-bot bot removed the triage label Apr 3, 2023
@judsonmusic
Copy link

@liamdebeasi I joined the discord. What is your discord username? I was going to ping you minors under Judson all capital letters. If you want to add me I'll ping you when I'm in front of my desk in a little bit.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Apr 3, 2023
@matthew2564
Copy link
Author

Thanks for the report. Adding aria-label or label opts in to the new form control syntax which has a different architecture under the hood. The big difference is ion-select contains both the label and the form control. So in this case, you should be using the label property on ion-select instead of using a separate <label> and an aria-label.

Here is an example: https://codepen.io/liamdebeasi/pen/yLxmqdR

You can control the order of the label/control using labelPlacement. You can also control how the label and control are packed on a line by using the justify property.

Does this not invalidate any custom HTML composition?
The reason we are using label in a different context to the select/input itself is due to positioning on a page. i.e. ion-col's spacing elements apart.

If we do not have true flexibility between separating the label to the element then it feels like this shouldn’t be a warning and more of an error.

On another note, why does ‘aria-label’ have any impact on the positioning of the carets themselves? That query is more from inquisitive perspectiveto try and understand the underlying logic/tech more.

Another point mentioned above is re the 'aria-labelledby', why is this not sufficient to suppress the warning produced via the element when using 'ion-select'?

@matthew2564
Copy link
Author

What version of Chrome are you using?

Im using Brave, which uses Chromium

Version 1.49.132 Chromium: 111.0.5563.147 (Official Build) (arm64)

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Apr 5, 2023
@liamdebeasi
Copy link
Contributor

Are you able to reproduce this issue in an Ionic starter app and provide a link to the repo? I'm also using Brave with Chromium 111, and the text is truncated correctly.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Apr 5, 2023
@ionitron-bot ionitron-bot bot removed the triage label Apr 5, 2023
@matthew2564
Copy link
Author

matthew2564 commented Apr 5, 2023

Are you able to reproduce this issue in an Ionic starter app and provide a link to the repo? I'm also using Brave with Chromium 111, and the text is truncated correctly.

https://github.com/matthew2564/ion-select-testing

I can also see a similar affect when building to the iPad 9th gen iOS simulator, although this repo has a lot more in it which is why I started the above to check if there were other factors

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Apr 5, 2023
@liamdebeasi
Copy link
Contributor

Thanks! I can reproduce this issue now. The problem is related to stacked labels in ion-select. We incorrectly assume that the selected text can take up 100% of the ion-select width when using stacked labels. However, this is not true because we need to account for the width of the select icon.

@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels Apr 5, 2023
@ionitron-bot ionitron-bot bot removed triage labels Apr 5, 2023
@matthew2564
Copy link
Author

Thanks! I can reproduce this issue now. The problem is related to stacked labels in ion-select. We incorrectly assume that the selected text can take up 100% of the ion-select width when using stacked labels. However, this is not true because we need to account for the width of the select icon.

Brilliant, and thank you!

If you need any more info/examples etc, then just reach out.

Thanks!

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Apr 5, 2023

Here is a dev build if you are interested in trying a proposed fix: 7.0.2-dev.11680713209.14d9bc55

@liamdebeasi liamdebeasi removed their assignment Apr 5, 2023
@matthew2564
Copy link
Author

matthew2564 commented Apr 6, 2023

Truncation is working fine when specifying aria-label and no labelPlacement
Screenshot 2023-04-06 at 08 47 41

But the caret is not locked to the right hand side (when not using labelPlacement), it appears to be reactive to the length the value in the ion-select

Screenshot 2023-04-06 at 08 48 38

Screenshot 2023-04-06 at 08 47 30

Truncation does not work when specifying both aria-label and labelPlacement="stacked"

Screenshot 2023-04-06 at 08 52 23

Committed and pushed the new version (7.0.2-dev.11680713209.14d9bc55) to https://github.com/matthew2564/ion-select-testing

@liamdebeasi
Copy link
Contributor

Truncation does not work when specifying both aria-label and labelPlacement="stacked"

I'm not able to reproduce this using the dev build and the following code:

<ion-select
  placeholder="Select a fruit"
  aria-label="fruit"
  multiple="true"
  fill="outline"
  style="border: 3px solid blue;"
  labelPlacement="stacked"
  [value]="['apple','banana','orange']"
>
  <ion-select-option value="apple">Apple</ion-select-option>
  <ion-select-option value="banana">Banana</ion-select-option>
  <ion-select-option value="orange">Orange</ion-select-option>
</ion-select>

Just FYI your package-lock.json is still trying to install a nightly build of Ionic which does not have the proposed fix: https://github.com/matthew2564/ion-select-testing/blob/0093096c2bfc44a0f14f517c5f1009ae86d60b8c/package-lock.json#L2981

That could be why you are not seeing any improvements.

@matthew2564
Copy link
Author

@liamdebeasi Ah yes nice spot, it now appears to be working my side using 7.0.2-dev.11680713209.14d9bc55, thanks!

liamdebeasi added a commit that referenced this issue Apr 12, 2023
<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

<!-- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation
for details. -->

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->


<!-- Issues are required for both bug fixes and features. -->
Issue URL: resolves #27081

When using stacked/floating labels the icon needs to be centered with
the entire component, not just the placeholder/selected text. As a
result, we set `position: absolute`. However, this causes the long
selected texts to overlap the icon. This is not happening with
non-stacked/floating labels because the icon is `position: relative` and
follows the normal document flow.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Moved the icon sizes to sass variables
- Added code to set the .native-wrapper width to 100% minus the width of
the icon _and_ the additional margin that .select-icon adds

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

---------

Co-authored-by: ionitron <[email protected]>
Co-authored-by: Brandy Carney <[email protected]>
@matthew2564
Copy link
Author

Hey @liamdebeasi, just confirming if the fix made it into 7.0.2?

Tried to bump my boilerplate project and the fix seems to not be there...? matthew2564/ion-select-testing#1

@liamdebeasi
Copy link
Contributor

The PR for this was merged a few hours after Ionic 7.0.2 went out. You can check the changelog here: https://github.com/ionic-team/ionic-framework/releases/tag/v7.0.2

This fix should be in the next release though. (We release every week)

liamdebeasi added a commit that referenced this issue Apr 17, 2023
<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

<!-- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation
for details. -->

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->


<!-- Issues are required for both bug fixes and features. -->
Issue URL: resolves #27081

When using stacked/floating labels the icon needs to be centered with
the entire component, not just the placeholder/selected text. As a
result, we set `position: absolute`. However, this causes the long
selected texts to overlap the icon. This is not happening with
non-stacked/floating labels because the icon is `position: relative` and
follows the normal document flow.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Moved the icon sizes to sass variables
- Added code to set the .native-wrapper width to 100% minus the width of
the icon _and_ the additional margin that .select-icon adds

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

---------

Co-authored-by: ionitron <[email protected]>
Co-authored-by: Brandy Carney <[email protected]>
liamdebeasi added a commit that referenced this issue Apr 19, 2023
<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

<!-- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation
for details. -->

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->


<!-- Issues are required for both bug fixes and features. -->
Issue URL: resolves #27081

When using stacked/floating labels the icon needs to be centered with
the entire component, not just the placeholder/selected text. As a
result, we set `position: absolute`. However, this causes the long
selected texts to overlap the icon. This is not happening with
non-stacked/floating labels because the icon is `position: relative` and
follows the normal document flow.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Moved the icon sizes to sass variables
- Added code to set the .native-wrapper width to 100% minus the width of
the icon _and_ the additional margin that .select-icon adds

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

---------

Co-authored-by: ionitron <[email protected]>
Co-authored-by: Brandy Carney <[email protected]>
@ionitron-bot
Copy link

ionitron-bot bot commented May 13, 2023

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators May 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants