Skip to content
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

RCSS attribute selector support #240

Closed

Conversation

aquawicket
Copy link
Contributor

This PR adds .rcss attribute selector support to RmlUi. I've tested all 5 operators and they tested pretty well. lacking better error control in the Parser, and maybe some variables renamed. Too soon to merge, but it's available for review.

https://www.w3schools.com/cssref/css_selectors.asp
https://developer.mozilla.org/en-US/docs/Web/CSS/Attribute_selectors
https://drafts.csswg.org/selectors/#attribute-selectors

[attribute] [target] Selects all elements with a target attribute
[attribute=value] [target=_blank] Selects all elements with target="_blank"
[attribute~=value] [title~=flower] Selects all elements with a title attribute containing the word "flower"
[attribute|=value] [lang|=en] Selects all elements with a lang attribute value equal to "en" or starting with "en-"
[attribute^=value] a[href^="https"] Selects every element whose href attribute value begins with "https"
[attribute$=value] a[href$=".pdf"] Selects every element whose href attribute value ends with ".pdf"
[attribute*=value] a[href*="w3schools"] Selects every element whose href attribute value contains the substring "w3schools"

@mikke89 mikke89 added the enhancement New feature or request label Sep 30, 2021
@mikke89
Copy link
Owner

mikke89 commented Sep 30, 2021

Very cool, this is a welcome addition indeed!

One thing to keep in mind is that the applicable rules for an element should be updated when the attributes of the element change. At the same time, we want to minimize the number of times we need to recalculate the applicable nodes, otherwise we might end up with some really bad performance regressions. Eg. when the attributes are changed and no attribute rules are used at all, we should not have to recalculate applicable nodes. We should add some benchmarks to ensure we don't have performance regressions.

Looking forward to seeing this take shape, let me know when you feel it is ready to be reviewed!

@aquawicket
Copy link
Contributor Author

aquawicket commented Sep 30, 2021

Yea, i was wondering what that was refreshing,,.. so that's the main for streamline.. i noticed you are using robin hood as well.
. ok.. I'll dig in a little deeper and get everything in line. trying to keep the code base short as well.. So does calling applicable inject, trigger refresh ? store cache? You've added quite a bit to this library . lol. Just tell me which files to review and i'll be up to speed soon enough :) It's a lot easier to read the code since rocket. so i'm happy about that. thanks mike. PS. I'm not a trained programmer.. I learn what i can on my own, It's just a hobby that i love :)

@mikke89
Copy link
Owner

mikke89 commented Oct 1, 2021

You're doing good so far, and seems like you're figuring things out quickly, just keep it up :) And it's very nice to hear you find the code easier to read.

So we somehow need to dirty the element definition (ie. the list of applicable properties) when the attributes change. However we need to be smart about this, because dirtying the element definition is a slow process, so I think we need some kind of attribute volatility flag or similar. This flag could be set when there is some chance that a changed attribute could lead to a different definition. The tricky part about this is that the current element's definition could be changed when attributes in parents and other elements are changed.

Related files and functions are Element.cpp for attributes being applied, and also ElementStyle::UpdateDefinition() and ElementStyle::DirtyDefinition().

The element definition is cached during StyleSheet::GetElementDefinition() so we may need to change that as well.

Hopefully these pointers can be helpful, let me know if anything is unclear.

@mikke89
Copy link
Owner

mikke89 commented May 1, 2022

Hey. Just wondering if you intend to keep working on this. I can see that you keep it up to date with master which is nice. However, if there are no plans to integrate the previously discussed changes in the near feature it's time to close this PR.

With that said, if you or anyone else wants to take this on again at a later stage then I am all positive on doing that.

@aquawicket
Copy link
Contributor Author

I havn't had the time, I'll try and work on it soon and send a new PR. Thanks

@mikke89
Copy link
Owner

mikke89 commented May 3, 2022

I understand, no worries. Closing this one for the time being, feel free to either re-open or submit a new PR later.

@mikke89 mikke89 closed this May 3, 2022
mikke89 added a commit that referenced this pull request Aug 4, 2022
@mikke89
Copy link
Owner

mikke89 commented Aug 4, 2022

Hey. I recently figured I'd take up this work and ended up merging the result: a96164c. I've added you as a co-author on the commit. I started out with your changes, refactored it a bit, tried to speed it up where I could (mostly avoiding allocations), and fixed some issues. I am quite happy with the end result,

I did a lot of benchmarking and tried several different ways to speed up the lookup of the attribute selectors. It wasn't really that successful, anytime I got a substantial speedup there was a baseline slowdown on all lookups in general. So the end result is that they are quite slow compared to tag and class lookups, which are indexed for fast retrieval. So I let them be a bit slow and figured that is acceptable as they are an opt-in feature. Also, they can be combined with tags and classes for faster lookup. Maybe we can find better strategies in the future.

In any case, thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants