bug: Alerts override id assigned through htmlAttributes

Prerequisites

I have read the Contributing Guidelines.
I agree to follow the Code of Conduct.
I have searched for existing issues that already report this problem, without success.

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

Create a new alert via the AlertController, giving it a custom ID
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

@mikelhamer

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

@mikelhamer

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

Oops! Done

@mikelhamer

@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 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.

@mikelhamer

@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:

Render the overlay with your HTML attributes assigned
Query the overlay element
Validate the ID matches the attribute value assigned

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

@sean-perkins

@sean-perkins many thanks! I will dive in

@sean-perkins

@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);
  });
});

@mikelhamer

@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();
}

fix(alert): dont overwrite id set in htmlAttributes

@sean-perkins

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

fix(alert): do not overwrite id set in htmlAttributes (#29708)

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.

Navigation Menu

Repository navigation

bug: Alerts override id assigned through htmlAttributes #29704

Prerequisites

Ionic Framework Version

Current Behavior

Expected Behavior

Steps to Reproduce

Code Reproduction URL

Ionic Info

Additional Information

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

bug: Alerts override id assigned through htmlAttributes #29704

Description

Prerequisites

Ionic Framework Version

Current Behavior

Expected Behavior

Steps to Reproduce

Code Reproduction URL

Ionic Info

Additional Information

Activity

sean-perkins commented on Jul 11, 2024

mikelhamer commented on Jul 11, 2024

sean-perkins commented on Jul 11, 2024

mikelhamer commented on Jul 11, 2024

sean-perkins commented on Jul 11, 2024

mikelhamer commented on Jul 12, 2024

mikelhamer commented on Jul 12, 2024

sean-perkins commented on Jul 12, 2024

mikelhamer commented on Jul 12, 2024

ionitron-bot commented on Aug 14, 2024

Metadata

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Issue actions