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: Alerts override id assigned through htmlAttributes #29704

Closed
3 tasks done
mikelhamer opened this issue Jul 10, 2024 · 10 comments · Fixed by #29708 · 4 remaining pull requests
Closed
3 tasks done

bug: Alerts override id assigned through htmlAttributes #29704

mikelhamer opened this issue Jul 10, 2024 · 10 comments · Fixed by #29708 · 4 remaining pull requests
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@mikelhamer
Copy link
Contributor

mikelhamer commented Jul 10, 2024

Prerequisites

Ionic Framework Version

v8.x

Current Behavior

After upgrading from version 7 to 8, the ion-alerts created via the AlertController no longer have the custom ID attribute I set via the htmlAttributes property like so

alertController.create({
    htmlAttributes: {id: 'custom-id'}
    ...
});

Instead, it has an id of ion-overlay-1.

Expected Behavior

The ion-alert should have the custom ID

Steps to Reproduce

  1. Create a new alert via the AlertController, giving it a custom ID
  2. Observe that the underlying ion-alert does not have the custom ID

Code Reproduction URL

https://github.com/mikelhamer/ion8-alert-bug

Ionic Info

[WARN] Error loading @capacitor/ios package.json: Error: Cannot find module '@capacitor/ios/package.json'
       
       Require stack:
       - /opt/homebrew/lib/node_modules/@ionic/cli/lib/project/index.js
       - /opt/homebrew/lib/node_modules/@ionic/cli/lib/index.js
       - /opt/homebrew/lib/node_modules/@ionic/cli/index.js
       - /opt/homebrew/lib/node_modules/@ionic/cli/bin/ionic
[WARN] Error loading @capacitor/android package.json: Error: Cannot find module '@capacitor/android/package.json'
       
       Require stack:
       - /opt/homebrew/lib/node_modules/@ionic/cli/lib/project/index.js
       - /opt/homebrew/lib/node_modules/@ionic/cli/lib/index.js
       - /opt/homebrew/lib/node_modules/@ionic/cli/index.js
       - /opt/homebrew/lib/node_modules/@ionic/cli/bin/ionic

Ionic:

   Ionic CLI                     : 7.2.0 (/opt/homebrew/lib/node_modules/@ionic/cli)
   Ionic Framework               : @ionic/angular 8.2.5
   @angular-devkit/build-angular : 18.1.0
   @angular-devkit/schematics    : 18.1.0
   @angular/cli                  : 18.1.0
   @ionic/angular-toolkit        : 11.0.1

Capacitor:

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

Utility:

   cordova-res (update available: 0.15.4) : 0.15.3
   native-run                             : 2.0.1

System:

   NodeJS : v20.15.1 (/usr/local/bin/node)
   npm    : 9.7.1
   OS     : macOS Monterey

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Jul 10, 2024
@sean-perkins
Copy link
Contributor

@mikelhamer can you make the repository public so the Ionic team can review it? Thanks!

@mikelhamer
Copy link
Contributor Author

@mikelhamer can you make the repository public so the Ionic team can review it? Thanks!

Oops! Done

@sean-perkins
Copy link
Contributor

@mikelhamer awesome, thanks!

This behavior changed in this PR from me 😅

This is a bug. Ionic Framework's overlays need to be updated to additionally account for the id being passed through htmlAttributes, since the element will not have reflected the id at the time we are checking to set the incremental overlay id.

As a workaround you can manually assigning the attribute after the overlay is presented:

await alert.present();
await alert.setAttribute('id', 'custom-id');

@sean-perkins sean-perkins changed the title bug: AlertController not setting id from htmlAttributes bug: Overlays override id assigned through htmlAttributes Jul 11, 2024
@sean-perkins sean-perkins added package: core @ionic/core package type: bug a confirmed bug report labels Jul 11, 2024
@ionitron-bot ionitron-bot bot removed the triage label Jul 11, 2024
@mikelhamer
Copy link
Contributor Author

@sean-perkins woohoo! Thanks for confirming, and for the workaround! I wouldn't mind trying to take this on as a first contribution, but could use some insights into the impact of that PR and what to consider when applying a fix.

@sean-perkins
Copy link
Contributor

@mikelhamer awesome!

I believe the fix is straightforward, we need to opt-out of calling setOverlayId in the overlay components when the htmlAttributes has an id.

For example, something similar to:

componentWillLoad() {
  if (!this.htmlAttributes?.id) {
    setOverlayId(this.el);
  }
}

This should prevent Ionic from overriding the id that a developer passes in.

Ionic's tests can feel intimidating, but I promise they aren't too bad to get started with 👍 The documentation for how testing works in Ionic Framework is available here: https://github.com/ionic-team/ionic-framework/tree/main/docs/core/testing. You may be able to write a simple Stencil spec test instead of relying on something heavier like the Playwright e2e tests.

An example of a Stencil spec test is here: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/alert/test/alert.spec.ts

The test case should be:

  1. Render the overlay with your HTML attributes assigned
  2. Query the overlay element
  3. Validate the ID matches the attribute value assigned

Let me know if you run into any challenges or have questions!

@mikelhamer
Copy link
Contributor Author

@sean-perkins many thanks! I will dive in

@mikelhamer
Copy link
Contributor Author

@sean-perkins hmmmm, I am a bit lost. I've created this test which I thought should fail, but it's passing before making any changes to alert.tsx. I'm struggling to get this test to fail.

 it('should be assigned id from html attributes', async () => {
    // Given: An alert with an id specified in html attributes
    const page = await newSpecPage({
      components: [Alert],
      html: `<ion-alert is-open="true"></ion-alert>`
    });
    let alert: HTMLIonAlertElement = page.root as HTMLIonAlertElement;
    const htmlAttributes = {id: 'custom-id'};
    alert.htmlAttributes = htmlAttributes;
    await page.waitForChanges();

    // When: The alert is queried from the page
    alert = page.body.querySelector('ion-alert')!;

    // Then: The id should be set to the id specified in html attributes
    expect(alert).not.toBe(null);
    expect(alert.getAttribute('id')).toBe(htmlAttributes.id);
  });
});

@sean-perkins
Copy link
Contributor

@mikelhamer I would recommend renaming the alert.spec.ts to alert.spec.tsx so you can use JSX to assign the test template easier.

For example:

// src/components/alert/test/alert.spec.tsx

import { h } from '@stencil/core';
import { newSpecPage } from '@stencil/core/testing';

import { config } from '../../../global/config';
import { Alert } from '../alert';

describe('alert: custom html', () => {
  it('does not overwrite the id set in the htmlAttributes', async () => {
    const page = await newSpecPage({
      components: [Alert],
      template: () => <ion-alert htmlAttributes={{ id: 'my-custom-id' }} overlayIndex={-1}></ion-alert>,
    });

    const alert = page.body.querySelector('ion-alert')!;
    expect(alert.id).toBe('my-custom-id');
  });

  /* existing tests */
});

With it, I was able to confirm the test fails on main and passes with:

componentWillLoad() {
+  if (!this.htmlAttributes?.id) {
    setOverlayId(this.el);
+  }
  this.inputsChanged();
  this.buttonsChanged();
}

@mikelhamer
Copy link
Contributor Author

@sean-perkins thanks for the help! PR opened.

github-merge-queue bot pushed a commit that referenced this issue Jul 15, 2024
Issue number: resolves #29704

---------

<!-- 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?
Setting the id of an alert via htmlAttributes doesn't work due to it
being overwritten by the overlay id.

## What is the new behavior?
Setting the id of an alert via htmlAttributes works.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#footer
for more information.
-->


## Other information

First time PR, long time fan. Please let me know if I missed any of the
contributing guidelines, happy to change anything!
@sean-perkins sean-perkins changed the title bug: Overlays override id assigned through htmlAttributes bug: Alerts override id assigned through htmlAttributes Jul 15, 2024
Copy link

ionitron-bot bot commented Aug 14, 2024

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 Aug 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.