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

Appearance property "base" works incorrect, if appearance from parent theme has string value. #9429

Open
mfedyashev opened this Issue Nov 27, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@mfedyashev

mfedyashev commented Nov 27, 2017

For example, appearance for "datefield" from theme "qx.theme.modern.Appearance" has value:

"datefield" : "combobox",

If create custom theme and extend "qx.theme.modern.Appearance", then in custom theme create apearance with the same name "datefield" and use property "base: true":

"datefield": { base: true, style: function (states) { return { textColor: "red" }; } }

In this case the result appearance does not have properties from parent theme.

Qooxdoo Playground example: http://tinyurl.com/y8hj6b82

@cajus

This comment has been minimized.

Show comment
Hide comment
@cajus

cajus Nov 27, 2017

Contributor

Didn't you mean to change the textColor of datefield/textfield?

Contributor

cajus commented Nov 27, 2017

Didn't you mean to change the textColor of datefield/textfield?

@mfedyashev

This comment has been minimized.

Show comment
Hide comment
@mfedyashev

mfedyashev Nov 28, 2017

No, I didn't mean textColor. You can use empty style function, for example:
style: function (states) { return { }; }
I mean, that datefield appearance in custom theme does not inherite properties from parent theme.

mfedyashev commented Nov 28, 2017

No, I didn't mean textColor. You can use empty style function, for example:
style: function (states) { return { }; }
I mean, that datefield appearance in custom theme does not inherite properties from parent theme.

@cajus

This comment has been minimized.

Show comment
Hide comment
@cajus

cajus Nov 28, 2017

Contributor

I can't find a problem in your description. What I ment with my comment was that there is no textColor on the level of datefield that is used somewhere, so it will not have any effect.

Maybe the problem is different: base: true does not work if the parent class has no own top level styling. In the modern theme, datefield is just directly aliased to combobox. So you might need to add an include: 'combobox' to your definition to make it work.

Contributor

cajus commented Nov 28, 2017

I can't find a problem in your description. What I ment with my comment was that there is no textColor on the level of datefield that is used somewhere, so it will not have any effect.

Maybe the problem is different: base: true does not work if the parent class has no own top level styling. In the modern theme, datefield is just directly aliased to combobox. So you might need to add an include: 'combobox' to your definition to make it work.

@mfedyashev

This comment has been minimized.

Show comment
Hide comment
@mfedyashev

mfedyashev Nov 28, 2017

Problem is not about textColor, I know that textColor on the level datefield does not have any effect. It just was bad example.
Issue is the Date field with custom theme does not have border like the Date field with moderh theme.
So, as User, I set "base: true" and expect that custom datefield will include all property from Modern datefield. But, actually, it does not work.

Yes, include: 'combobox' can help in this case, but, I think, it just workaround.

mfedyashev commented Nov 28, 2017

Problem is not about textColor, I know that textColor on the level datefield does not have any effect. It just was bad example.
Issue is the Date field with custom theme does not have border like the Date field with moderh theme.
So, as User, I set "base: true" and expect that custom datefield will include all property from Modern datefield. But, actually, it does not work.

Yes, include: 'combobox' can help in this case, but, I think, it just workaround.

@cajus

This comment has been minimized.

Show comment
Hide comment
@cajus

cajus Nov 28, 2017

Contributor

It reacts like the comments in the appearance manager suggest. So that was done this way by intention of the original authors. I don't know why they did it this way.

Contributor

cajus commented Nov 28, 2017

It reacts like the comments in the appearance manager suggest. So that was done this way by intention of the original authors. I don't know why they did it this way.

@mfedyashev

This comment has been minimized.

Show comment
Hide comment
@mfedyashev

mfedyashev Nov 28, 2017

The function qx.theme.manager.Appearance.styleFrom uses common this.__aliasMap for all themes. For my custom theme datefield appearance is Entry, so when styleFrom calculates this appearnce, item "datefield": "datefield" is added to alias map. Then styleFrom calculates datefield appearance from Modern theme, but it does not resolve new id, it just gets existing alias from map. But alias "datefield": "datefield" is not correct for Modern theme. For Modern theme it should be "datefield": "combobox"

mfedyashev commented Nov 28, 2017

The function qx.theme.manager.Appearance.styleFrom uses common this.__aliasMap for all themes. For my custom theme datefield appearance is Entry, so when styleFrom calculates this appearnce, item "datefield": "datefield" is added to alias map. Then styleFrom calculates datefield appearance from Modern theme, but it does not resolve new id, it just gets existing alias from map. But alias "datefield": "datefield" is not correct for Modern theme. For Modern theme it should be "datefield": "combobox"

@ronkie

This comment has been minimized.

Show comment
Hide comment
@ronkie

ronkie Mar 9, 2018

Contributor

I've slightly modified an example:

https://tinyurl.com/yafwjr97

If we want to style text color of a textfield as subcontrol of a datefield, the issue is also reproducible.
You can notice that even though only text color is styled, datefield/textfield also loses it's paddings (they are not picked up anymore from the parent appearance).

Compare this with styling combobox/datefield (it is commented out in the example) - in this case paddings and other stylings are not lost, because they are still picked up from the parent theme.

Contributor

ronkie commented Mar 9, 2018

I've slightly modified an example:

https://tinyurl.com/yafwjr97

If we want to style text color of a textfield as subcontrol of a datefield, the issue is also reproducible.
You can notice that even though only text color is styled, datefield/textfield also loses it's paddings (they are not picked up anymore from the parent appearance).

Compare this with styling combobox/datefield (it is commented out in the example) - in this case paddings and other stylings are not lost, because they are still picked up from the parent theme.

ronkie added a commit to ronkie/qooxdoo that referenced this issue Mar 15, 2018

Appearance property "base" works incorrect, if appearance from parent…
… theme has string value. qooxdoo#9429

Always resolve alias as parent theme appearance can have a different one
@ronkie

This comment has been minimized.

Show comment
Hide comment
@ronkie

ronkie Mar 16, 2018

Contributor

So, it looks like "base" call for an appearance that overrides appearance with alias set through "string form" (e.g. "datefield/textfield": "combobox/textfield") does not do it's job, because not all the style properties of parent appearance are picked up.

We can put in the parent theme, instead of string alias:

"datefield/textfield": {
alias: "combobox/textfield",
include: "combobox/textfield",
style: function() {
return {};
}
}

In this case everything will be mapped correctly, and also in inherited appearance all the styles will be picked up correctly.

The "aliases" section of docs:

http://www.qooxdoo.org/current/pages/desktop/ui_appearance.html#aliases

Says "There is no type of implicit inheritance." So does this mean in particular that properties that are obtained through alias will not be inherited?

Contributor

ronkie commented Mar 16, 2018

So, it looks like "base" call for an appearance that overrides appearance with alias set through "string form" (e.g. "datefield/textfield": "combobox/textfield") does not do it's job, because not all the style properties of parent appearance are picked up.

We can put in the parent theme, instead of string alias:

"datefield/textfield": {
alias: "combobox/textfield",
include: "combobox/textfield",
style: function() {
return {};
}
}

In this case everything will be mapped correctly, and also in inherited appearance all the styles will be picked up correctly.

The "aliases" section of docs:

http://www.qooxdoo.org/current/pages/desktop/ui_appearance.html#aliases

Says "There is no type of implicit inheritance." So does this mean in particular that properties that are obtained through alias will not be inherited?

ronkie added a commit to ronkie/qooxdoo that referenced this issue Mar 20, 2018

Appearance property "base" works incorrect, if appearance from parent…
… theme has string value. qooxdoo#9429

Store aliases and styles per theme, as in different themes aliases can point to different appearances

level420 added a commit that referenced this issue Apr 17, 2018

Appearance property "base" works incorrect, if appearance from parent…
… theme has string value. #9429 (#9509)

* Appearance property "base" works incorrect, if appearance from parent theme has string value. #9429
Always resolve alias as parent theme appearance can have a different one

* Appearance property "base" works incorrect, if appearance from parent theme has string value. #9429
Store aliases and styles per theme, as in different themes aliases can point to different appearances

johnspackman added a commit to johnspackman/qooxdoo that referenced this issue Apr 20, 2018

Merge commit 'd4d3a359c84cbe9a4262d64dab9a8a57ba4dc953'
* commit 'd4d3a359c84cbe9a4262d64dab9a8a57ba4dc953':
  Add decoration entries in another order (qooxdoo#9518)
  Appearance property "base" works incorrect, if appearance from parent theme has string value. qooxdoo#9429 (qooxdoo#9509)
  Pseudo doc change to re-trigger a fresh travis run
  Removed @zaucker from CODOWNERS with his agreeement
  disable safari again because of failing tests
  add safari and run lint and coverage within one stage
  Really separate the coverage stage from the Edge stage
  Separate the coverage stage from the Edge stage
  Use stages to separate the test runs
  attempt to split the travis testrunner tests into 3 separate tests
  Improved __defaultModel jsDoc
  allow to set font weight
  scale 20 may lead to the travis unit test to run into a timeout. downscaling to 4
  Disable embedded audio/video event test on travis and fix macro definitions for travis generator run
  Improved PR based on comments
  Improved jsDoc documentation
  [New] allow drop-downs to be wider than their parent
  ~ Fix Bug when breaking target property chain qooxdoo#9506
  Fix non working non strict mode compatibility in qx.ui.table.model.Simple
  Promise support in Events (addition to qooxdoo#9491) (qooxdoo#9498)

# Conflicts:
#	application/uitests/source/class/uitests/DragAndDropApplication.js
#	framework/source/class/qx/event/Registration.js
#	framework/source/class/qx/event/Utils.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment