apache / apache/royale-asjs

Improve SimpleCSSValuesImpl

Open
#1,171 3 comments 0 reactions 0 assignees View on GitHub
Dominant language
ActionScript
Stars
380
Forks
120
PR merge metrics
No merged PRs in 30d

Description

Currently SimpleCSSValuesImpl gets style values like so:
1. returns the value from the underlying style if it exists.
2. Walks up the parent tree and returns the parent value if it's inherited.*
3. I uses a lookup for either css class names or Class names to get assigned style info
4. The Class names are saved in compiled MXML and added to the SimpleCSSValuesImpl by a call to `ValuesManager.valuesImpl.init(this)` in any class which implements `IMXMLDocument`
5. If the object is a class, it checks the qualified name and uses that to look up the styles.
6. When it doesn't find the styles, it will walk up the super-tree and does the same for each of the super-classes.

This has multiple issues:
1. Getting styles can be fairly expensive. As mentioned by @estanglerbm https://lists.apache.org/thread/qb1x76ohszmllkflzrq3mg70gy20y6tv The style lookups can be made much more efficient.
2. Requiring lookups based on qualified class names requires keeping these names around in minified code for every class in the application.
3. There's no way to cache known values to make lookups easier. (related to 1)
4. The code is pretty convoluted and can be made cleaner.

Additonally there's some issues identified here:
https://github.com/apache/royale-asjs/issues/641#issuecomment-573836029

An example of a compiled MXML lookup is this:
```
0,
1,
"org.apache.royale.core.Application",
function() {this["MozUserSelect"] = "none";
this["MsUserSelect"] = "none";
this["WebkitUserSelect"] = "none";
this["margin"] = 0.0;
this["padding"] = 0.0;
this["userSelect"] = "none"},
0,
1,
"org.apache.royale.html.Container",
function() {this["alignItems"] = "flex-start";
this["iBeadLayout"] = org.apache.royale.html.beads.layouts.BasicLayout;
this["iBeadView"] = org.apache.royale.html.beads.ContainerView;
this["iViewport"] = org.apache.royale.html.supportClasses.OverflowViewport;
this["iViewportModel"] = org.apache.royale.html.beads.models.ViewportModel},
```

Here's a potential improvement:
1. Instead of using `"org.apache.royale.html.Container"`, the value can be written unquoted to refer to the class directly like we do for bead references. This will enable to compiler to minimize the references normally.
2. This means that you can't add the class names to a hash in the `valuesImpl`.
3. Instead, we can add to `IStyleableObject` `setStyle()` and `getStyle()` That would put the logic of saving the style information and retrieving it in the class itself.
4. `setStyle()` and `getStyle()` would be instance methods, but it would save the information to the class so it's written once.
5. Additonally, getStyle can cache info it gets from the super-class so subsequent lookups would not need to walk up the hierarchy again. **
6. We might have additional methods for `saveCacheValue` and `getCacheValue` for optimizing lookups when walking up parents if it's known that it's safe to do so. These values would need to be saved in instances for it to work. The right balance of memory usage and speed optimization would need to be found for this.

* It looks like there's an inefficiency in step 2 where it will walk up the tree multiple times if the value is not set anywhere up the tree. That's because both `getValue` and `getInheritedValue` is called recursively in `getInheritedValue`. The current parent should probably be saved as an instance variable to prevent having to walk up the tree many times.
* * need to figure out if caching the super-values might be an issue for modules.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by reading SimpleCSSValuesImpl and the ValuesManager.valuesImpl.init(this) path used by IMXMLDocument implementations, then review IStyleableObject and the referenced issue discussion. Done means the style lookup design addresses qualified-name retention, repeated parent and superclass walks, caching, and module safety without leaving the current behavior undefined.

Written by the indexing model from the issue text.

Assessment

Tech stack
css
Domain
frontend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.