material icon indicating copy to clipboard operation
material copied to clipboard

dialog: theme not inheriting properly when changing themes

Open Arns opened this issue 7 years ago • 6 comments

Bug, feature request, or proposal:

Bug

What is the expected behavior?

The dialog will take on the theme that is set on its parent element (or somewhere in the ascendants).

What is the current behavior?

If changing the theme, the dialog still uses the previous theme.

CodePen and steps to reproduce the issue:

CodePen Demo which shows your issue: https://codepen.io/anon/pen/WzmmKx
Detailed Reproduction Steps:
  1. set a theme
  2. open dialog WITHOUT targetEvent set with an md-primary button and view theme
  3. switch theme using theme provider/service
  4. open dialog and see that theme did not update

What is the use-case or motivation for changing an existing behavior?

It essentially renders theme changing useless since the interim elements don't respect the change.

Which versions of AngularJS, Material, OS, and browsers are affected?

AngularJS Material >= 1.1.2 AngularJS >= 1.6.0

Is there anything else we should know? Stack Traces, Screenshots, etc.

Likely something in the following changes that broke it: https://github.com/angular/material/pull/9762/files#diff-58cbee594b2fabf2c78b6d283750e535R806

Arns avatar Apr 11 '18 17:04 Arns

Originally I did not realize this, but it appears a work around is setting the targetEvent when opening a dialog via the $mdDialog service. Not sure why/how this is related, and it is not documented anywhere that I've found.

Arns avatar Apr 11 '18 17:04 Arns

I updated the CodePen to 1.1.8 and I still see the issue. Thank you for reporting this and for creating the CodePen.

Splaktar avatar Apr 17 '18 19:04 Splaktar

Originally I did not realize this, but it appears a work around is setting the targetEvent when opening a dialog via the $mdDialog service. Not sure why/how this is related, and it is not documented anywhere that I've found.

Another workaround – just set the theme explicitly, like so:

let alert = $mdDialog.alert({
  title: 'Themed',
  textContent: 'This dialog is correctly themed!',
  ok: 'Close',
  theme: $scope.theme
});
$mdDialog.show(alert);

$mdDialogPreset#theme is documented under the preset creators, e.g. $mdDialog.alert().

This workaround also works when you can't set targetEvent (this was how I found it out), and it's more stable as it doesn't rely on the not well understood connection with targetEvent, which might disappear in next versions of AngularJS Material.

tbitai avatar Jul 19 '20 11:07 tbitai

$mdDialog's Theme Inheritance Demo uses the targetEvent API, which is required for theme inheritance.

However, while this API is documented, it isn't documented that this API is required for theme inheritance. A note about this needs to be added to the $mdDialog service docs. I don't think that it makes sense directly on the targetEvent API. We should probably add a dialog theming example to these docs and a note about specifying targetEvent being required.

We should also have a brief example of theming dialogs using the theme API w/o inheritance (as mentioned in the previous comment).

Splaktar avatar Jul 21 '20 18:07 Splaktar

So do I understand correctly that theme inheritance works like theme is inherited from the element which corresponds to the DOMClickEvent passed as targetEvent?

Anyway, information about dialogs' theme inheritance mechanism would be nice in the theming docs as well, as for example for me it was surprising that I set an md-theme on my body and it didn't propagate to the dialogs, despite that they're children of body.

(The logic of my app is so that it doesn't make sense for my dialogs to have a targetEvent. If I understand correctly, in that case theme inheritance is not defined according to AngularJS Material's system, so I have to explicitly set the theme, which is fine, just it would be nice if this was documented.)

As I think about it, theme inheritance via targetEvent is actually reasonable, as there might be multiple themes present among the elements of the page, and then $mdDialog wouldn't know which theme the dialog should inherit. Although perhaps it could be set to fall back to parent's theme in case targetEvent is not set, and then the explicit setting I mentioned above wouldn't be necessary.

tbitai avatar Jul 23 '20 14:07 tbitai

So do I understand correctly that theme inheritance works like theme is inherited from the element which corresponds to the DOMClickEvent passed as targetEvent?

Yes, see https://github.com/angular/material/blob/a13722ee69ab01babefe1f0bcc5af77f55a3f76c/src/components/dialog/dialog.js#L825-L839

Agreed that it should be mentioned with a note/callout in the theming guide.

Splaktar avatar Jul 23 '20 19:07 Splaktar