search

SAP / openui5

OpenUI5 lets you build enterprise-ready web applications, responsive to all devices, running on almost any browser of your choice.
http://openui5.org
Apache License 2.0
2.95k stars 1.23k forks source link

sap.m.MessagePage: main icon's CSS rule should not affect all icons #3145

Closed boghyon closed 3 years ago

boghyon commented 3 years ago

Steps to reproduce the problem:

  1. Go to https://openui5nightly.hana.ondemand.com/entity/sap.m.MessagePage/sample/sap.m.sample.MessagePageWithButtons
  2. Add an icon (e.g. sap-icon://message-warning) to one of the buttons.

What is the expected result?

The CSS rule for the main icon in the MessagePage is specific to the main icon only and does not affect other icons.

What happens instead?

icon-sap m Button-within-sap m MessagePage

The icon in e.g. sap.m.Button is affected due to: https://github.com/SAP/openui5/blob/a2e209dc78ea1da73396dc97f52f033d5fc79510/src/sap.m/src/sap/m/themes/base/MessagePage.less#L49-L56

unazko commented 3 years ago

Hello @boghyon,

Thank you for sharing this finding. I've created an internal incident 2180085202. The status of the issue will be updated here in GitHub.

Best Regards, Boyan

yanaminkova commented 3 years ago

Hello @boghyon,

Thank you for sharing this finding. The issue is now fixed with: 6f6dab2

Best regards, Yana

boghyon commented 3 years ago

The commit https://github.com/SAP/openui5/commit/6f6dab2c0df8e0e3ecb543e4d4b2510327e96874 unfortunately has some issues.

  1. It only covers sap.m.Button explicitly. Future or custom controls, which extend from sap.m.MessagePage, would have the same issue again if they include an icon somewhere outside of a button.

  2. The hardcoded font-size: 1rem; may only work for sap_fiori_3 in compact density mode, but not in other settings. Here is the button icon, for example, in sap_belize_plus cozy mode after the commit: Button-icon-within-sap m MessagePage-in-Belize-themes

  3. Each one of the themes applies also color: fade(@sapUiContentNonInteractiveIconColor, <themeDependentValue>) to the icon. The commit https://github.com/SAP/openui5/commit/6f6dab2c0df8e0e3ecb543e4d4b2510327e96874, however, handles only the size of the button icon ignoring the fade in each theme.

I proposed a solution in this PR: https://github.com/SAP/openui5/pull/3146.

It leverages the sapMMessagePageIcon CSS class which is available only to the main icon of the sap.m.MessagePage: https://github.com/SAP/openui5/blob/091ecbf11538e311c86b520aaca6827d26c91270/src/sap.m/src/sap/m/MessagePage.js#L321 I think it's a good opportunity to make use of sapMMessagePageIcon to increase the specificity of the main icon so that the CSS rules do not affect any other icons inside of sap.m.MessagePage.

Please consider reverting https://github.com/SAP/openui5/commit/6f6dab2c0df8e0e3ecb543e4d4b2510327e96874 and reopening this issue.

alexandar-mitsev commented 3 years ago

Hi,

Based on the comments and on the open PR #3146 I am reopening the issue. It will be processed further with the internal incident 2180085202 and progress will be updated here.

Regards, Alexandar Mitsev

boghyon commented 1 year ago

For future readers: the control sap.m.MessagePage is deprecated since https://github.com/SAP/openui5/commit/eef51496841f732dab5d66e0ffc7e36b3009855e (UI5 1.112). Use sap.m.IllustratedMessage instead: https://sdk.openui5.org/entity/sap.m.IllustratedMessage