[EXTERNAL] CssStyleHelper canReuseStyleHelper

[Dean Wookey]

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
>