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

[BUGFIX] Wrong selector extraction from minified CSS #69

Merged
merged 1 commit into from
Mar 5, 2014
Merged

[BUGFIX] Wrong selector extraction from minified CSS #69

merged 1 commit into from
Mar 5, 2014

Conversation

OzzyCzech
Copy link

Emogrifier have problem with minified CSS code

First character from first selector is lost:

html,h1,.class{color:red;} => tml,h1,.class {color:red;}

public function emogrifyMinifiedCss() {
$html = self::HTML5_DOCUMENT_TYPE . self::LF . '<html><p></p></html>' . self::LF;
$this->subject->setHtml($html);
$this->subject->setCss('html{color:blue;}html,h1,.class{color:red;}');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you reduce the CSS test data to a minimum that still has the problem? Currently, I do not see what the exact problem is: two selectors, or no spaces, or two rules for the same property?

Maybe you can also change the test name to show what the exact case is.

@oliverklee
Copy link
Contributor

In the commit message: css -> CSS

@OzzyCzech
Copy link
Author

See var_dump of $matches on line 308

array(2) {
  [0] =>
  array(3) {
    [0] =>  string(17) "html{color:blue;}"
    [1] => string(4) "html"
    [2] => string(11) "color:blue;"
  }
  [1] =>
  array(3) {
    [0] => string(16) "html{color:red;}"
    [1] =>  string(3) "tml"
    [2] => string(10) "color:red;"
  }
}

Second selector it's just tml

$this->subject->setCss('html{color:blue;}html{color:red;}');

$this->assertContains(
'style="color:red;"',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to have the element name here in the assertContains.

@OzzyCzech
Copy link
Author

done

public function emogrifyCanMatchMinifiedCss() {
$html = self::HTML5_DOCUMENT_TYPE . self::LF . '<html><p></p></html>' . self::LF;
$this->subject->setHtml($html);
$this->subject->setCss('html{color:blue;}html{color:red;}');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to have two different properties here (or even better: two different element names) so that we don't test attribute overriding ("the last one wins") or "two attributes for one element" here.

Copy link
Contributor

Choose a reason for hiding this comment

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

And let's have two tests: One for the first selector and one for the second/last selector.

@OzzyCzech
Copy link
Author

we need there have two elements in minified CSS - that's the point first one match well second not match at all

what about

p{color:blue;}html{color:red;}

Will be enough?

@oliverklee
Copy link
Contributor

Yes, that would be perfect.

@OzzyCzech
Copy link
Author

it's there

@oliverklee oliverklee merged commit 2732016 into MyIntervals:master Mar 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants