Skip to content

Add support for grid line names #132

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 13, 2018
Merged

Add support for grid line names #132

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98c4b3a
Add support for grid line names
raxbg Jul 11, 2018
92abffa
Apply improvement suggestions by Sabberworm
raxbg Jul 12, 2018
c95610b
User parseIdentifier() for line name parsing
raxbg Jul 12, 2018
46e80a9
Apply suggestions by Sabberworm
raxbg Jul 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions lib/Sabberworm/CSS/Parser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use Sabberworm\CSS\Value\Color;
use Sabberworm\CSS\Value\URL;
use Sabberworm\CSS\Value\CSSString;
use Sabberworm\CSS\Value\LineName;
use Sabberworm\CSS\Rule\Rule;
use Sabberworm\CSS\Parsing\UnexpectedTokenException;
use Sabberworm\CSS\Comment\Comment;
Expand Down Expand Up @@ -447,6 +448,8 @@ private function parsePrimitiveValue() {
$oValue = $this->parseStringValue();
} else if ($this->comes("progid:") && $this->oParserSettings->bLenientParsing) {
$oValue = $this->parseMicrosoftFilter();
} else if ($this->comes("[")) {
$oValue = $this->parseLineNameValue();
} else {
$oValue = $this->parseIdentifier(true, false);
}
Expand Down Expand Up @@ -480,6 +483,23 @@ private function parseNumericValue($bForColor = false) {
return new Size(floatval($sSize), $sUnit, $bForColor, $this->iLineNo);
}

private function parseLineNameValue() {
$this->consume('[');
$sName = '';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use consumeWhitespace after [ and before ].

Copy link
Contributor Author

@raxbg raxbg Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be okay now. Please check.

try {
do {
$this->consumeWhiteSpace();
$sName .= $this->parseIdentifier(false, true) . ' ';
} while (!$this->comes(']'));
} catch (UnexpectedTokenException $e) {
if(!$this->oParserSettings->bLenientParsing && (!$sName || !$this->comes(']'))) {// This handles constructs like this [ linename ] in non lenient mode
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lenient mode has always tried to mimic what the browser does when parsing. I think if the browser encounters an unparseable value, it throws away the whole rule so we should do the same. Since there is a handler for UnexpectedTokenException already in place that does that, we don’t need to catch it in lenient mode.

Then again I’m not sure what the other half of the if (!$sName || !$this->comes(']')) is for. The comment does not enlighten me much. Shouldn’t [ linename ] parse correctly without throwing an exception? There’s something I don’t understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lenient mode has always tried to mimic what the browser does when parsing. I think if the browser encounters an unparseable value, it throws away the whole rule so we should do the same.

[] is parsed as [] in Chrome. If the UnexpectedTokenException is not handled here the parser will drop the value for the rule.

Then again I’m not sure what the other half of the if (!$sName || !$this->comes(']')) is for.

On the second iteration of the loop, parseIdentifier() will try to parse the ], so it will throw an exception. However this should not be considered an error because $sName will not be empty (it will have the value of 'linename '). The exception should be forwarded only if $sName is empty and lenient mode is disabled. This will be the case if parsing [].

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, you have convinced me with the lenient mode exception handling.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think using exception handling for control flow should be avoided. Having read your comments, shouldn’t the following also work?:

private function parseLineNameValue() {
	$this->consume('[');
	$this->consumeWhiteSpace();
	$sName = '';
	do {
		if($this->oParserSettings->bLenientParsing) {
			try {
				$sName .= $this->parseIdentifier(false, true) . ' ';
			} catch(UnexpectedTokenException $e) {}
		} else {
			$sName .= $this->parseIdentifier(false, true) . ' ';
		}
		$this->consumeWhiteSpace();
	} while (!$this->comes(']'));
	$this->consume(']');
	return new LineName(rtrim($sName), $this->iLineNo);
}

Having said that: now that I know there can be multiple identifiers in line name value brackets, I think LineName should extend ValueList with $sSeparator = ' ' and thus the code would become:

private function parseLineNameValue() {
	$this->consume('[');
	$this->consumeWhiteSpace();
	$aNames = array();
	do {
		if($this->oParserSettings->bLenientParsing) {
			try {
				$aNames[] = $this->parseIdentifier(false, true);
			} catch(UnexpectedTokenException $e) {}
		} else {
			$aNames[] = $this->parseIdentifier(false, true);
		}
		$this->consumeWhiteSpace();
	} while (!$this->comes(']'));
	$this->consume(']');
	return new LineName($aNames, $this->iLineNo);
}

which would allow you to shorten LineName to the following:

class LineName extends ValueList {
	public function __construct($aComponents = array(), $iLineNo = 0) {
		parent::__construct($aComponents, ' ', $iLineNo);
	}

	public function __toString() {
		return $this->render(new \Sabberworm\CSS\OutputFormat());
	}

	public function render(\Sabberworm\CSS\OutputFormat $oOutputFormat) {
		return '['.parent::render(\Sabberworm\CSS\OutputFormat::createCompact()).']';
	}
}

Optionally, you could create OutputFormat options on whether or not white-space should be output after [ and before ].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it ❤️

throw $e;
}
}
$this->consume(']');
return new LineName(rtrim($sName), $this->iLineNo);
}

private function parseColorValue() {
$aColor = array();
if ($this->comes('#')) {
Expand Down
29 changes: 29 additions & 0 deletions lib/Sabberworm/CSS/Value/LineName.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?php

namespace Sabberworm\CSS\Value;

class LineName extends PrimitiveValue {
private $sName;

public function __construct($sName, $iLineNo = 0) {
parent::__construct($iLineNo);
$this->sName = $sName;
}

public function setName($sName) {
$this->sName = $sName;
}

public function getName() {
return $this->sName;
}

public function __toString() {
return $this->render(new \Sabberworm\CSS\OutputFormat());
}

public function render(\Sabberworm\CSS\OutputFormat $oOutputFormat) {
return "[{$this->sName}]";
}

}
19 changes: 19 additions & 0 deletions tests/Sabberworm/CSS/ParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,25 @@ function testCalcInFile() {
$this->assertSame($sExpected, $oDoc->render());
}

function testGridLineNameInFile() {
$oDoc = $this->parsedStructureForFile('grid-linename', Settings::create()->withMultibyteSupport(true));
$sExpected = "div {grid-template-columns: [linename] 100px;}\nspan {grid-template-columns: [linename1 linename2] 100px;}";
$this->assertSame($sExpected, $oDoc->render());
}

function testEmptyGridLineNameLenientInFile() {
$oDoc = $this->parsedStructureForFile('empty-grid-linename');
$sExpected = '.test {grid-template-columns: [] 100px;}';
$this->assertSame($sExpected, $oDoc->render());
}

/**
* @expectedException Sabberworm\CSS\Parsing\UnexpectedTokenException
*/
function testLineNameFailure() {
$this->parsedStructureForFile('-empty-grid-linename', Settings::create()->withLenientParsing(false));
}

/**
* @expectedException Sabberworm\CSS\Parsing\UnexpectedTokenException
*/
Expand Down
1 change: 1 addition & 0 deletions tests/files/-empty-grid-linename.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.test { grid-template-columns: [] 100px; }
1 change: 1 addition & 0 deletions tests/files/empty-grid-linename.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.test { grid-template-columns: [] 100px; }
2 changes: 2 additions & 0 deletions tests/files/grid-linename.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
div { grid-template-columns: [ linename ] 100px; }
span { grid-template-columns: [ linename1 linename2 ] 100px; }