[EXTERNAL] CssStyleHelper canReuseStyleHelper

[Dean Wookey]

It's also a problem with variables defined in css, as well as inherited
values like visibility (cursor seems to work differently), eg:
.a { col: red; }
.b { col: blue; }
.leaf { -fx-background-color: col}

Then move leaf from branch a to b.

Likewise
.a { visibility: collapse; }
.b { visibility: visible; }
.leaf { visibility: inherit;}

If you move a node from branch a to branch b, it stays invisible. Again
that one like fix solves all these problems.

I've created JDK-8237469 <https://bugs.openjdk.java.net/browse/JDK-8237469>.

On Fri, Jan 17, 2020 at 11:50 AM Dean Wookey <wookey.dean at gmail.com> wrote:

> Hi David,
>
> Thanks, I wrote that test as a graphics module test and it does fail.
> https://gist.github.com/DeanWookey/2624945e3ba037f00c39c0bfd0b22cef
>
> It passes if you move
>
> node.styleHelper.firstStyleableAncestor =
> findFirstStyleableAncestor(node); (line 134)
>
> to line 321. I imagine there is some performance impact if you do this.
> I'm not sure it affects anything other than fonts at the moment.
> Pseudoclasses recalculate the entire tree every time, so they appear to be
> fine.
>
> Dean
>
> On Thu, Jan 16, 2020 at 9:01 PM David Grieve <David.Grieve at microsoft.com>
> wrote:
>
>> I'd create a scenegraph
>>
>>                  R
>>          .-----+-----.
>>          A               B
>>    .----+----.
>>    C            D
>>
>> Where C and D are Labels. Then I'd set a font style on A and a different
>> font style on B.
>> C and D should pick up the font style of A. Then move D to B and see if
>> it still has A's
>> font style.
>>
>> It may be necessary to have a more deeply nested scene graph.
>>
>> The potential issue is that the node that got moved could pick up the old
>> set of styles
>> if the style helper is not renewed.
>>
>> Ultimately, canReuseStyleHelper will return false if the set of styles
>> for the
>> new parent (line 110 or so in CssStyleHelper.java) are different.
>>
>> > -----Original Message-----
>> > From: openjfx-dev <openjfx-dev-bounces at openjdk.java.net> On Behalf Of
>> > Dean Wookey
>> > Sent: Thursday, January 16, 2020 11:34 AM
>> > To: openjfx-dev at openjdk.java.net Mailing <openjfx-
>> > dev at openjdk.java.net>; David Grieve <david.grieve at oracle.com>
>> > Cc: aghaisas at openjdk.org
>> > Subject: [EXTERNAL] CssStyleHelper canReuseStyleHelper
>> >
>> > Hi Ajit, David,
>> >
>> > I came accross a potential issue introduced by JDK-8090462 (
>> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs
>> > .openjdk.java.net%2Fbrowse%2FJDK-
>> > 8090462&data=02%7C01%7Cdavid.grieve%40microsoft.com%7C235930d
>> > fab7f4f7f15ba08d79aa21774%7C72f988bf86f141af91ab2d7cd011db47%7C1%7
>> > C0%7C637147893881930150&sdata=awsMTPYkp7GmSrYsoy9I6M%2F6uj
>> > X7thgZhBVY9UKcjpU%3D&reserved=0), (
>> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
>> > ub.com%2Fopenjdk%2Fjfx%2Fcommit%2F834b0d05e7a2a8351403ec4a121ea
>> > b312d80fd24%23diff-
>> > 9ec098280fa1aeed53c70243347e76ab&data=02%7C01%7Cdavid.grieve%
>> > 40microsoft.com%7C235930dfab7f4f7f15ba08d79aa21774%7C72f988bf86f141
>> > af91ab2d7cd011db47%7C1%7C0%7C637147893881930150&sdata=ZhWS
>> > dmlJ9rLzIkqnohWTacJKPb8FNrG5yApF0fwP8g4%3D&reserved=0).
>> > The issue is in the canReuseStyleHelper method. canReuseStyleHelper is
>> > called as a result of changes to the scene graph and for other reasons.
>> The
>> > original code found the parent helper in the following way:
>> >
>> >         Styleable parent = node.getStyleableParent();
>> >         while (parent != null) {
>> >             if (parent instanceof Node) {
>> >                 parentHelper = ((Node) parent).styleHelper;
>> >                 if (parentHelper != null) break;
>> >             }
>> >             parent = parent.getStyleableParent();
>> >         }
>> >
>> > This gets the parent helper for the new parent of the node. The code
>> now (
>> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
>> > ub.com%2Fopenjdk%2Fjfx%2Fblob%2F20325e1c3ec4c4e81af74d3d43bf3a803
>> > dbe1a51%2Fmodules%2Fjavafx.graphics%2Fsrc%2Fmain%2Fjava%2Fjavafx%
>> > 2Fscene%2FCssStyleHelper.java%23L322&data=02%7C01%7Cdavid.griev
>> > e%40microsoft.com%7C235930dfab7f4f7f15ba08d79aa21774%7C72f988bf86f
>> > 141af91ab2d7cd011db47%7C1%7C0%7C637147893881930150&sdata=nJR
>> > 3ocKaIsf6HWNjHtseEbqLbEvbW7%2FzQS1B%2F39GVyI%3D&reserved=
>> > 0
>> > ):
>> >
>> >         CssStyleHelper parentHelper =
>> > getStyleHelper(node.styleHelper.firstStyleableAncestor);
>> >
>> > gets the helper of the previous parent of the node since
>> > firstStyleableAncestor hasn't been updated to reflect the current state
>> of the
>> > scene graph yet. This means if you move a node, it could keep its
>> > CssStyleHelper even if it's under completely new parents.
>> >
>> > I'm not sure if this is actually causing any problems. It would be
>> helpful if I
>> > knew the kind of things that could go wrong if you reuse a style helper
>> so
>> > that I could design a test that highlighted the issue.
>> >
>> > Can you perhaps think of cases where this could go badly?
>> >
>> > Dean
>>
>