Improve PropertySpecification::ParsePropertyValues

- Allow splitting by comma
- No escaping inside parenthesis
- Enable quote mode inside parenthesis
- Add unit tests

Include/RmlUi/Core/PropertySpecification.h

@@ -136,9 +136,11 @@ private:
 	PropertyIdSet property_ids_inherited;
 	PropertyIdSet property_ids_forcing_layout;
 
-	bool ParsePropertyValues(StringList& values_list, const String& values, bool split_values) const;
+	enum class SplitOption { None, Whitespace, Comma };
+	bool ParsePropertyValues(StringList& values_list, const String& values, SplitOption split_option) const;
 
 	friend class Rml::StyleSheetSpecification;
+	friend class TestPropertySpecification;
 };
 
 } // namespace Rml

Source/Core/PropertySpecification.cpp

@@ -242,7 +242,7 @@ bool PropertySpecification::ParsePropertyDeclaration(PropertyDictionary& diction
 		return false;
 
 	StringList property_values;
-	if (!ParsePropertyValues(property_values, property_value, false) || property_values.size() == 0)
+	if (!ParsePropertyValues(property_values, property_value, SplitOption::None) || property_values.empty())
 		return false;
 
 	Property new_property;
@@ -255,15 +255,17 @@ bool PropertySpecification::ParsePropertyDeclaration(PropertyDictionary& diction
 
 bool PropertySpecification::ParseShorthandDeclaration(PropertyDictionary& dictionary, ShorthandId shorthand_id, const String& property_value) const
 {
-	StringList property_values;
-	if (!ParsePropertyValues(property_values, property_value, true) || property_values.size() == 0)
-		return false;
-
-	// Parse as a shorthand.
 	const ShorthandDefinition* shorthand_definition = GetShorthand(shorthand_id);
 	if (!shorthand_definition)
 		return false;
 
+	const SplitOption split_option =
+		(shorthand_definition->type == ShorthandType::RecursiveCommaSeparated ? SplitOption::Comma : SplitOption::Whitespace);
+
+	StringList property_values;
+	if (!ParsePropertyValues(property_values, property_value, split_option) || property_values.empty())
+		return false;
+
 	// Handle the special behavior of the flex shorthand first, otherwise it acts like 'FallThrough'.
 	if (shorthand_definition->type == ShorthandType::Flex)
 	{
@@ -346,30 +348,28 @@ bool PropertySpecification::ParseShorthandDeclaration(PropertyDictionary& dictio
 	}
 	else if (shorthand_definition->type == ShorthandType::RecursiveCommaSeparated)
 	{
-		StringList subvalues;
-		StringUtilities::ExpandString(subvalues, property_value);
-
 		size_t num_optional = 0;
 		for (auto& item : shorthand_definition->items)
 			if (item.optional)
 				num_optional += 1;
 
-		if (subvalues.size() + num_optional < shorthand_definition->items.size())
+		if (property_values.size() + num_optional < shorthand_definition->items.size())
 		{
 			// Not enough subvalues declared.
 			return false;
 		}
 
 		size_t subvalue_i = 0;
-		for (size_t i = 0; i < shorthand_definition->items.size() && subvalue_i < subvalues.size(); i++)
+		for (size_t i = 0; i < shorthand_definition->items.size() && subvalue_i < property_values.size(); i++)
 		{
 			bool result = false;
 
+			const String* subvalue = &property_values[subvalue_i];
 			const ShorthandItem& item = shorthand_definition->items[i];
 			if (item.type == ShorthandItemType::Property)
-				result = ParsePropertyDeclaration(dictionary, item.property_id, subvalues[subvalue_i]);
+				result = ParsePropertyDeclaration(dictionary, item.property_id, *subvalue);
 			else if (item.type == ShorthandItemType::Shorthand)
-				result = ParseShorthandDeclaration(dictionary, item.shorthand_id, subvalues[subvalue_i]);
+				result = ParseShorthandDeclaration(dictionary, item.shorthand_id, *subvalue);
 
 			if (result)
 				subvalue_i += 1;
@@ -448,25 +448,33 @@ String PropertySpecification::PropertiesToString(const PropertyDictionary& dicti
 	return result;
 }
 
-bool PropertySpecification::ParsePropertyValues(StringList& values_list, const String& values, bool split_values) const
+bool PropertySpecification::ParsePropertyValues(StringList& values_list, const String& values, const SplitOption split_option) const
 {
+	const bool split_values = (split_option != SplitOption::None);
+	const bool split_by_comma = (split_option == SplitOption::Comma);
+	const bool split_by_whitespace = (split_option == SplitOption::Whitespace);
+
 	String value;
 
-	enum ParseState { VALUE, VALUE_PARENTHESIS, VALUE_QUOTE };
+	auto SubmitValue = [&]() {
+		value = StringUtilities::StripWhitespace(value);
+		if (value.size() > 0)
+		{
+			values_list.push_back(value);
+			value.clear();
+		}
+	};
+
+	enum ParseState { VALUE, VALUE_PARENTHESIS, VALUE_QUOTE, VALUE_QUOTE_ESCAPE_NEXT };
 	ParseState state = VALUE;
 	int open_parentheses = 0;
-
 	size_t character_index = 0;
-	bool escape_next = false;
 
 	while (character_index < values.size())
 	{
 		const char character = values[character_index];
 		character_index++;
 
-		const bool escape_character = escape_next;
-		escape_next = false;
-
 		switch (state)
 		{
 		case VALUE:
@@ -480,37 +488,20 @@ bool PropertySpecification::ParsePropertyValues(StringList& values_list, const S
 					value.clear();
 				}
 			}
-			else if (StringUtilities::IsWhitespace(character))
+			else if (split_by_comma ? (character == ',') : StringUtilities::IsWhitespace(character))
 			{
 				if (split_values)
-				{
-					value = StringUtilities::StripWhitespace(value);
-					if (value.size() > 0)
-					{
-						values_list.push_back(value);
-						value.clear();
-					}
-				}
+					SubmitValue();
 				else
 					value += character;
 			}
 			else if (character == '"')
 			{
-				if (split_values)
-				{
-					value = StringUtilities::StripWhitespace(value);
-					if (value.size() > 0)
-					{
-						values_list.push_back(value);
-						value.clear();
-					}
-					state = VALUE_QUOTE;
-				}
+				state = VALUE_QUOTE;
+				if (split_by_whitespace)
+					SubmitValue();
 				else
-				{
-					value += ' ';
-					state = VALUE_QUOTE;
-				}
+					value += (split_by_comma ? '"' : ' ');
 			}
 			else if (character == '(')
 			{
@@ -524,97 +515,73 @@ bool PropertySpecification::ParsePropertyValues(StringList& values_list, const S
 			}
 		}
 		break;
-
 		case VALUE_PARENTHESIS:
 		{
-			if (escape_character)
+			if (character == '(')
 			{
-				if (character == ')' || character == '(' || character == '\\')
-				{
-					value += character;
-				}
-				else
-				{
-					value += '\\';
-					value += character;
-				}
+				open_parentheses++;
 			}
-			else
+			else if (character == ')')
 			{
-				if (character == '(')
-				{
-					open_parentheses++;
-					value += character;
-				}
-				else if (character == ')')
-				{
-					open_parentheses--;
-					value += character;
-					if (open_parentheses == 0)
-						state = VALUE;
-				}
-				else if (character == '\\')
-				{
-					escape_next = true;
-				}
-				else
-				{
-					value += character;
-				}
+				open_parentheses--;
+				if (open_parentheses == 0)
+					state = VALUE;
 			}
+			else if (character == '"')
+			{
+				state = VALUE_QUOTE;
+			}
+
+			value += character;
 		}
 		break;
-
 		case VALUE_QUOTE:
 		{
-			if (escape_character)
+			if (character == '"')
 			{
-				if (character == '"' || character == '\\')
+				if (open_parentheses == 0)
 				{
-					value += character;
+					state = VALUE;
+					if (split_by_whitespace)
+						SubmitValue();
+					else
+						value += (split_by_comma ? '"' : ' ');
 				}
 				else
 				{
-					value += '\\';
+					state = VALUE_PARENTHESIS;
 					value += character;
 				}
 			}
+			else if (character == '\\')
+			{
+				state = VALUE_QUOTE_ESCAPE_NEXT;
+			}
 			else
 			{
-				if (character == '"')
-				{
-					if (split_values)
-					{
-						value = StringUtilities::StripWhitespace(value);
-						if (value.size() > 0)
-						{
-							values_list.push_back(value);
-							value.clear();
-						}
-					}
-					else
-						value += ' ';
-					state = VALUE;
-				}
-				else if (character == '\\')
-				{
-					escape_next = true;
-				}
-				else
-				{
-					value += character;
-				}
+				value += character;
+			}
+		}
+		break;
+		case VALUE_QUOTE_ESCAPE_NEXT:
+		{
+			if (character == '"' || character == '\\')
+			{
+				value += character;
 			}
+			else
+			{
+				value += '\\';
+				value += character;
+			}
+			state = VALUE_QUOTE;
 		}
+		break;
 		}
 	}
 
 	if (state == VALUE)
-	{
-		value = StringUtilities::StripWhitespace(value);
-		if (value.size() > 0)
-			values_list.push_back(value);
-	}
+		SubmitValue();
 
 	return true;
 }

Tests/Source/UnitTests/PropertySpecification.cpp

@@ -31,12 +31,142 @@
 #include <RmlUi/Core/PropertyDefinition.h>
 #include <RmlUi/Core/PropertyDictionary.h>
 #include <RmlUi/Core/PropertySpecification.h>
+#include <RmlUi/Core/StyleSheetSpecification.h>
 #include <doctest.h>
 #include <limits.h>
 
+namespace Rml {
+class TestPropertySpecification {
+public:
+	using SplitOption = PropertySpecification::SplitOption;
+	TestPropertySpecification(const PropertySpecification& specification) : specification(specification) {}
+
+	bool ParsePropertyValues(StringList& values_list, const String& values, SplitOption split_option) const
+	{
+		return specification.ParsePropertyValues(values_list, values, split_option);
+	}
+
+private:
+	const PropertySpecification& specification;
+};
+} // namespace Rml
+
 using namespace Rml;
 
-TEST_CASE("PropertySpecification")
+static String Stringify(const StringList& list)
+{
+	String result = "[";
+	for (int i = 0; i < (int)list.size(); i++)
+	{
+		if (i != 0)
+			result += "; ";
+		result += list[i];
+	}
+	result += ']';
+	return result;
+}
+
+TEST_CASE("PropertySpecification.ParsePropertyValues")
+{
+	TestsSystemInterface system_interface;
+	TestsRenderInterface render_interface;
+	SetRenderInterface(&render_interface);
+	SetSystemInterface(&system_interface);
+	Rml::Initialise();
+
+	using SplitOption = TestPropertySpecification::SplitOption;
+	const TestPropertySpecification& specification = TestPropertySpecification(StyleSheetSpecification::GetPropertySpecification());
+
+	struct Expected {
+		Expected(const char* value) : values{String(value)} {}
+		Expected(std::initializer_list<String> list) : values(list) {}
+		StringList values;
+	};
+
+	auto Parse = [&](const String& test_value, const Expected& expected, SplitOption split = SplitOption::Whitespace) {
+		StringList parsed_values;
+		bool success = specification.ParsePropertyValues(parsed_values, test_value, split);
+		const String split_str[] = {"none", "whitespace", "comma"};
+
+		INFO("\n\tSplit:     ", split_str[(int)split], "\n\tInput:     ", test_value, "\n\tExpected: ", Stringify(expected.values),
+			"\n\tResult:   ", Stringify(parsed_values));
+		CHECK(success);
+		CHECK(parsed_values == expected.values);
+	};
+
+	Parse("red", "red");
+	Parse(" red ", "red");
+	Parse("inline-block", "inline-block");
+
+	Parse("none red", {"none", "red"});
+	Parse("none    red", {"none", "red"});
+	Parse("none\t \r \nred", {"none", "red"});
+
+	Parse("none red", "none red", SplitOption::None);
+	Parse(" none red ", "none red", SplitOption::None);
+	Parse("none    red", "none    red", SplitOption::None);
+	Parse("none\t \r \nred", "none\t \r \nred", SplitOption::None);
+	Parse("none,red", "none,red", SplitOption::None);
+	Parse(" \"none,red\" ", "none,red", SplitOption::None);
+
+	Parse("none,red", {"none,red"});
+	Parse("none, red", {"none,", "red"});
+	Parse("none , red", {"none", ",", "red"});
+	Parse("none   ,   red", {"none", ",", "red"});
+	Parse("none,,red", "none,,red");
+	Parse("none,,,red", "none,,,red");
+
+	Parse("none,red", {"none", "red"}, SplitOption::Comma);
+	Parse("none, red", {"none", "red"}, SplitOption::Comma);
+	Parse("none , red", {"none", "red"}, SplitOption::Comma);
+	Parse("none   ,   red", {"none", "red"}, SplitOption::Comma);
+	Parse("none,,red", {"none", "red"}, SplitOption::Comma);
+	Parse("none,,,red", {"none", "red"}, SplitOption::Comma);
+	Parse("none, ,  ,red", {"none", "red"}, SplitOption::Comma);
+
+	Parse("\"string with spaces\"", "string with spaces");
+	Parse("\"string with spaces\" two", {"string with spaces", "two"});
+	Parse("\"string with spaces\"two", {"string with spaces", "two"});
+	Parse("\"string with spaces\"two", "string with spaces two", SplitOption::None);
+
+	Parse("\"string (with) ((parenthesis\" two", {"string (with) ((parenthesis", "two"});
+	Parse("\"none,,red\" two", {"none,,red", "two"});
+
+	Parse("aa(bb( cc ) dd) ee", {"aa(bb( cc ) dd)", "ee"});
+	Parse("aa(\"bb cc ) dd\") ee", {"aa(\"bb cc ) dd\")", "ee"});
+	Parse("aa(\"bb cc \\) dd\") ee", {"aa(\"bb cc \\) dd\")", "ee"});
+	Parse("aa(\"bb cc \\) dd\") ee", "aa(\"bb cc \\) dd\") ee", SplitOption::Comma);
+
+	Parse("none(\"long string\"), aa, \"bb() cc\"", {"none(\"long string\"),", "aa,", "bb() cc"});
+	Parse("none(\"long string\"), aa, \"bb() cc\"", {"none(\"long string\")", "aa", "\"bb() cc\""}, SplitOption::Comma);
+	Parse("none(\"long string\"), aa, bb() cc", {"none(\"long string\")", "aa", "bb() cc"}, SplitOption::Comma);
+
+	Parse("tiled-horizontal( title-bar-l, title-bar-c, title-bar-r )", "tiled-horizontal( title-bar-l, title-bar-c, title-bar-r )");
+	Parse("tiled-horizontal( title-bar-l, title-bar-c,\n\ttitle-bar-r )", "tiled-horizontal( title-bar-l, title-bar-c,\n\ttitle-bar-r )");
+	Parse("tiled-horizontal( title-bar-l, title-bar-c )", "tiled-horizontal( title-bar-l, title-bar-c )", SplitOption::Comma);
+
+	Parse("linear-gradient(110deg, #fff, #000 10%) border-box, image(invader.png)",
+		{"linear-gradient(110deg, #fff, #000 10%)", "border-box,", "image(invader.png)"});
+	Parse("linear-gradient(110deg, #fff, #000 10%) border-box, image(invader.png)",
+		{"linear-gradient(110deg, #fff, #000 10%) border-box", "image(invader.png)"}, SplitOption::Comma);
+
+	Parse(R"(image( a\) b ))", {R"(image( a\))", "b", ")"});
+	Parse(R"(image( a\) b ))", R"(image( a\) b ))", SplitOption::Comma);
+
+	Parse(R"(image( ))", R"(image( ))");
+	Parse(R"(image( a\\b ))", R"(image( a\\b ))");
+	Parse(R"(image( a\\\b ))", R"(image( a\\\b ))");
+	Parse(R"(image( a\\\\b ))", R"(image( a\\\\b ))");
+	Parse(R"(image("a\)b"))", R"(image("a\)b"))");
+	Parse(R"(image("a\\)b"))", R"(image("a\)b"))");
+	Parse(R"(image("a\\b"))", R"(image("a\b"))");
+	Parse(R"(image("a\\\b"))", R"(image("a\\b"))");
+	Parse(R"(image("a\\\\b"))", R"(image("a\\b"))");
+
+	Rml::Shutdown();
+}
+
+TEST_CASE("PropertySpecification.string")
 {
 	TestsSystemInterface system_interface;
 	TestsRenderInterface render_interface;
@@ -89,13 +219,6 @@ TEST_CASE("PropertySpecification")
 	Parse(R"(image(a, "b"))", R"(image(a, "b"))");
 	Parse(R"V("image(a, \"b\")")V", R"V(image(a, "b"))V");
 
-	Parse(R"(image( ))", R"(image( ))");
-	Parse(R"(image( a\)b ))", R"(image( a)b ))");
-	Parse(R"(image("a\)b"))", R"(image("a)b"))");
-	Parse(R"(image( a\\b ))", R"(image( a\b ))");
-	Parse(R"(image( a\\\b ))", R"(image( a\\b ))");
-	Parse(R"(image( a\\\\b ))", R"(image( a\\b ))");
-
 	Rml::Shutdown();
 }
 
@@ -107,6 +230,7 @@ TEST_CASE("PropertyParser.Keyword")
 	SetSystemInterface(&system_interface);
 	Rml::Initialise();
 
+	// Test keyword parser. Ensure that the keyword values are correct.
 	PropertySpecification specification(20, 0);
 
 	auto Parse = [&](const PropertyId id, const String& test_value, int expected_value) {