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

regression: properties from included decorators do not get propagated #9524

Open
level420 opened this Issue Apr 20, 2018 · 8 comments

Comments

Projects
None yet
2 participants
@level420
Member

level420 commented Apr 20, 2018

And it seems that the last merged PRs #9518 and/or #9509 introduced a regression:

I have the following decorator definition in my theme:

"input-base" : {
  style : {
    radius: 2,
    color : "border-input",
    width: 1,
    innerWidth: 1,
    innerColor: "#f5f5f5"
  }
},

"input" : {
  include: "input-base",
  style : {
    gradientStart : ["#eaeaea", 0],
    gradientEnd : ["#fbfbfb", 100]
  }
},

What I suppose to happen is, that the deocorator input inherits width and innerWidth from input-base, but it does not happen.

@Adolfrizer and @ronkie could you please check if your PRs broke something? Thank you.

@level420

This comment has been minimized.

Member

level420 commented Apr 20, 2018

@Adolfrizer It's definitely #9518 I've backed out the commit here locally and inheritance works again.

I'm not sure, but this may be due to the shortcut width: 1 where width is defined http://www.qooxdoo.org/devel/api/#qx.ui.decoration.MSingleBorder~setWidth!method_public as being an array, but it also seems to accept the shortcut with a single number x which presumably means [x, x, x, x]. But this has to be verified.

@Adolfrizer

This comment has been minimized.

Contributor

Adolfrizer commented Apr 23, 2018

I've just verified this decorator definition and got the correct result: the decorator input inherits width and innerWidth from input-base. I debugged the resolve method and took a look at properties by qooxdoo plugin for Chrome - inherited properties exist.

And my changes (#9518), as far as I understand, were not related to the shortcut and group properties - I tried to expand them in the previous PR (#9511) which wasn't merged.

@level420

This comment has been minimized.

Member

level420 commented Apr 23, 2018

@Adolfrizer thank you for investigating. It remains the fact that if I revert commit d4d3a35 the decorator above works again with my theme. But maybe the patch unveils a bug in my theme?

I'll let this issue open until I find the time to debug my theme. Let's see if there are other people running into this issue.

BTW: what do you mean with "qooxdoo plugin for Chrome"? Sounds interesting.

@Adolfrizer

This comment has been minimized.

Contributor

Adolfrizer commented Apr 23, 2018

@level420

This comment has been minimized.

Member

level420 commented Apr 23, 2018

@Adolfrizer The effect I'm seeing is when a input field gets the focus. Then it seems to jump a bit, because of its different styles attached. When the field gets the focus the decorator changes from input to input-focus which causes the css class qx-input being removed and qx-input-focus being added. The class qx-abstract-field remains in the class attribute, indifferent which state the input field gets. In the chrome dom inspector I'm getting the following css style definitions for those classes:

.qx-abstract-field {
    display: block;
    position: absolute;
    resize: none;
    border-width: initial;
    border-top-width: initial;
    border-right-width: initial;
    border-bottom-width: initial;
    border-left-width: initial;
    border-style: none;
    border-color: initial;
    border-image: initial;
    padding: 0px;
    margin: 0px;
    background: transparent;
    outline: none;
    border-radius: 0px;
}

.qx-input {
    border-bottom-left-radius: 2px;
    border-bottom-right-radius: 2px;
    border-top-left-radius: 2px;
    border-top-right-radius: 2px;
    box-shadow: rgb(245, 245, 245) 0px 0px 0px 1px inset;
    background: linear-gradient(rgb(234, 234, 234) 0%, rgb(251, 251, 251) 100%) padding-box;
    border-bottom: 1px solid rgb(209, 209, 209);
    border-left: 1px solid rgb(209, 209, 209);
    border-right: 1px solid rgb(209, 209, 209);
    border-top: 1px solid rgb(209, 209, 209);
}

.qx-input-focused {
    box-shadow: rgba(138, 182, 207, 0.8) 0px 0px 2px 1px;
    background: linear-gradient(rgb(234, 234, 234) 0%, rgb(251, 251, 251) 100%) padding-box;
}

where qx-input-focused is missing the border-xxx-width attributes.

Switching over to the Computed tab of the dom inspector in chrome shows for the unfocused state of the input element a border-xxx-width attribute being 1px while for the focused state it shows 0px thus causing the jumping effect.

@level420

This comment has been minimized.

Member

level420 commented Apr 23, 2018

and here are the class definitions which are present if d4d3a35 is reverted:

.qx-abstract-field {
    display: block;
    position: absolute;
    resize: none;
    border-width: initial;
    border-style: none;
    border-color: initial;
    border-image: initial;
    padding: 0px;
    margin: 0px;
    background: transparent;
    outline: none;
    border-radius: 0px;
}

.qx-input {
    border-bottom-left-radius: 2px;
    border-bottom-right-radius: 2px;
    border-top-left-radius: 2px;
    border-top-right-radius: 2px;
    box-shadow: rgb(245, 245, 245) 0px 0px 0px 1px inset;
    background: linear-gradient(rgb(234, 234, 234) 0%, rgb(251, 251, 251) 100%) padding-box;
    border-bottom: 1px solid rgb(209, 209, 209);
    border-left: 1px solid rgb(209, 209, 209);
    border-right: 1px solid rgb(209, 209, 209);
    border-top: 1px solid rgb(209, 209, 209);
}

.qx-input-focused {
    border-bottom-left-radius: 2px;
    border-bottom-right-radius: 2px;
    border-top-left-radius: 2px;
    border-top-right-radius: 2px;
    box-shadow: rgb(245, 245, 245) 0px 0px 0px 1px inset, rgba(138, 182, 207, 0.8) 0px 0px 2px 1px;
    background: linear-gradient(rgb(234, 234, 234) 0%, rgb(251, 251, 251) 100%) padding-box;
    border-bottom: 1px solid rgb(209, 209, 209);
    border-left: 1px solid rgb(209, 209, 209);
    border-right: 1px solid rgb(209, 209, 209);
    border-top: 1px solid rgb(209, 209, 209);
}

as you can see the qx-input-focus css class now has border-xxx property definitions with 1px width and the state change from non focused to focused does not cause the jumpt.

@level420

This comment has been minimized.

Member

level420 commented Apr 23, 2018

@Adolfrizer The recursion you've implemented in https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/theme/manager/Decoration.js#L223-L242 can't work this way.

As I understand the code function handleIncludeEntry starts with the current decorator, in my case the object input-focused which has the attribute input-focused.include with the string value "input". It then calls recursively handleIncludeEntry with the string parameter "input" here: https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/theme/manager/Decoration.js#L225

This ends recursion as the string "input" does not have the attribute include anymore and the else part comes to execution (https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/theme/manager/Decoration.js#L227-L238).

But the input-focus decorator includes the input decorator which itself includes the input-base decorator. The input-base decorator is never used itself in a widget, but only holds some common base properties.

So I presume that the recursive lookup down the include chain stops after one include.

The Real decorator chain which did not work is this one:

    "input-base" :  {
        style : {
          radius: 2,
          color : "border-input",
          width: 1,
          innerWidth: 1,
          innerColor: "#f5f5f5"
        }
    },

    "input": {
      include: "input-base",
      style : {
          gradientStart : ["#eaeaea", 0],
          gradientEnd : ["#fbfbfb", 100]
      }
    },

    "input-focused" : {
      include: "input",
      style : {
        shadowColor: "blue-shadow",
        shadowLength : 0,
        shadowBlurRadius : 2,
        shadowSpreadRadius: 1
      }
    },

where the input-focused decorator is supposed to get width and innerWidth from the decorator input-base.

As soon as I add width and innerWidth to input it starts working.

level420 added a commit to level420/qooxdoo that referenced this issue Apr 23, 2018

Fix for qooxdoo#9524: fix recursive included style propagation
Replaced the faulty recursive decorator include algorithm.
@level420

This comment has been minimized.

Member

level420 commented Apr 23, 2018

@Adolfrizer would you please check if PR #9525 still solves the original issue? Thank you.

johnspackman added a commit that referenced this issue Apr 26, 2018

Merge pull request #9525 from level420/level420-fix-9524
Fix for #9524: fix recursive included decorator style propagation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment