Media queries

[Dakror]

This is a big one.

This PR adds support for basic media queries in RCSS.

Usage & Features

• RCSS media-blocks to apply properties conditional to context properties:
  • viewport width & height given in pixels, supports range (min-/max-)
  • resolution, aka dpi (using the density_independent_pixel_ratio value), supports range (min-/max-)
  • aspect ratio, using ratio notation like 16/9, internally using it as a float, supports range (min-/max-)
  • orientation "landscape" / "portrait", comparing viewport width against height
• Creating composite queries using "and" operator

Example usage:

div {
	height: 48px;
	width: 48px;
}

@media (min-width: 640px) {
	div {
		height: 32px;
		width: 32px;
	}
}

@media (orientation: landscape) and (min-resolution: 90dpi) {
	div {
		height: 32px;
		width: 32px;
	}
}

@media (max-aspect-ratio: 4/3) {
	div {
		height: 64px;
		width: 64px;
	}
}

---

I've tried to adhere as closely as possible to the early CSS media query standards, by for instance inforcing fixed units like dpi for resolution, and pixels for width & height and reusing the CSS names for the various properties. The CSS feature of media distinction like "screen" etc. does not really apply in RCSS, therefore it was omitted. The current approach matches media blocks with an all or nothing approach, since only conjunctive chaining ("and") is allowed.

Internals

In order to assemble the correct style sheet for a given context configuration, a new top-level class above StyleSheet was introduced: the StyleSheetContainer. It maps a PropertyDictionary of the query-list properties to the contained sub-stylesheet.

A function UpdateMediaFeatures creates a new combined StyleSheet and stores it inside the container, giving external access through a non-owning pointer.

For parsing, new parsers needed to be introduced, namely for the aspect ratio notation, and others for the unit enforcing.

The style sheet parser received a new parsing method with its own state machine parsing the custom syntax of the media query list

The document re-compiles the stylesheet when it detects a context property change.

A unit test suite checking all properties is provided.

[Dakror]

Hey @mikke89 can you tell me why the context parameter inside ReloadStyleSheet for InstanceDocumentStream is nullptr? If i pass the context that the method is receiving anyway, no issue seems apparent but i resolve the additional error that i reported in #170 regarding the missing sprites.

EDIT: Ah i seem to have found a warning appear when the document gets destructed, "Could not retrieve element in view, was it destroyed?" related to some data element getting released with UB state perhaps?

[Dakror]

The issue with #170 and this, is that we need the context in the tmp document to compile a stylesheet.. what to do though..

[mikke89]

First of all, awesome and great work! You're learning the library fast :)
It's great to have this feature, I know other user have requested it as well.

I'll go through it in more detail later on.

Hey @mikke89 can you tell me why the context parameter inside ReloadStyleSheet for InstanceDocumentStream is nullptr?

Yeah, the main reason for this is that I wanted to ensure that the state of the current context is not changed when ReloadStyleSheet occurs. This can happen due to eg. events, and any state change from this function would be quite unexpected. Is there still an issue here after #171?

[Dakror]

Yeah since we can't compile the stylesheet in the context-less temporary document, some warnings arise, particularly when images look for sprites (found it in my game as a test case :D ) . While these warnings don't cause any real problem since only the stylesheetcontainer is used from the temporary document, we still need to quench those warnings somehow..

[mikke89]

Oh, I see. I'll have to think about that some more.

[mikke89]

I've taken a detailed look at this now. Thanks a lot, looking forward to merging this :) I'm also pleased to see the very nice PR description.

Please see the detailed review comments, as well as the following remarks.

• The dpi unit for resolution doesn't really make sense as dpi vs pixels are fixed in RmlUi. The unit is actually more like the CSS dppx (dots per pixel), but this also doesn't make sense in RmlUi, because one dot is always one pixel. What we really want is dp-ratio dpr, but this again may be confusing to someone coming from CSS. However, I see that CSS adds an x unit alias, and this unit would actually also make sense for us. So I suggest that we call it that. More long-term, we might consider adopting the CSS approach to pixels, so that one RCSS pixel can be scaled relative to device pixels.

• I can see several RCSS problems introduced with this change. Eg. the 'demo' sample is not able to override the 'max-width' property. The VisualTests demo looks very broken. For some strange reaon, warnings are often given when tables are present, see eg. table_01 in VisualTests. Some of these problems may be due to specificity, eg. make sure all the rules are applied in the order they appear. Would be nice to have some unit tests for this.

• There is a substantial performance regression when loading documents, more than double the loading time compared to master. Haven't looked into it in any more detail, but we need to figure this out, it should not be measurably slower when not using media queries. I suspect there is a lot of duplicate work going on, the merging in particular should be looked at.

Here are some more notes for future changes. We can merge this PR before we work on any of this, but leaving these notes here for now.

• Need to test keyframes and sprite sheets in media block. I suspect eg. sprites won't properly updated as is. Any element/function that uses Element::GetStyleSheet should be investigated in particular.

• Other logic operations: Comma (OR), not... Maybe Media Queries level 4 syntax eg. (100px < width <= 1200px)? If we do implement that, then we may discard the min/max-width properties as the other operators are so much nicer.

• Instead of dirtying style sheets and definitions every time context dimensions and dp-ratio change, add flags to style sheet containers that tell whether they need to be dirtied when that value changes. So that we don't need to do anything if media queries are not used.

[Dakror]

Visual tests are fixed in 98bcc50

[Dakror]

Do you have any heavy hitting test for the performance issues you mentioned? I can't seem to detect a noticeable difference, looking for instance at unit test run duration.

[mikke89]

Do you have any heavy hitting test for the performance issues you mentioned? I can't seem to detect a noticeable difference, looking for instance at unit test run duration.

Yeah, if you run the benchmarks sample, take a look at the ElementDocument:LoadDocument benchmark, that is what I used.

[Dakror]

I've investigated the performance increase, and it purely stems from the heavy hitting DirtyDefinition call in SetStyleSheetContainer upon ProcessHeader call in ElementDocument. This adds a solid 5-10ms where normally only 5ms total were needed (on my system). Mitigating this would require adding all the relevant flags for invalidating only what's necessary and i think this is beyond me or this PR.

[mikke89]

Alright, thanks for looking into it. As long as we can fix the performance later on, I'm good with it for now.

[Dakror]

Working on a unit test i discovered that yeah indeed sprite updates are not updated, specifically i tested wether an ElementImage would update, but there is no method reacting to a change of that sort in that class. What do you think is the best approach to notify every element of relevant changes of this sort? I feel like this PR is getting a bit too big...

[Dakror]

I would say sprites + keyframes get a second PR? I would appreciate some help

[mikke89]

Yeah, I had the same issue in the high_dpi branch, which is why I expected it here as well. I had it half-working there, but now that we have media queries we probably want to make changes to the approach taken there.

Anyway, I'm fine with leaving that part out of this PR, we can work on that later.

[mikke89]

Thanks, looking good, just a few minor comments here.

So, after we merge this PR, we have the following high priority things to solve:

• The lifetime issue / potential crashes described here.
• The performance regression when loading documents.

I consider these two issues release blocking.

Additional things to work on, but lower priority (taken from previous post).

• Support for keyframes and sprite sheets.
• Expanded query syntax.
• Dirty flags.

[Dakror]

From my point of view, these issues would all be solvable using dirty flags because:

• Performance regression comes from the blunt DirtyDefinition which is super expensive
• Sprites and Keyframes aren't properly invalidated currently anyway
• Lifetimes could be monitored more closely and also tested against using dirty flags

[mikke89]

Yeah, dirty flags sounds like a good idea. I had a closer look at the lifetime issue, and it looks like we're not storing any related references around from the style sheet. So it should be all good, at least for now, and I feel a lot more comfortable merging it as is. But we should try to make it future proof as well.

Thank you for the hard work, cheers!