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

Don't combine selectors if the selector contains an escaped colon #233

Merged
merged 2 commits into from
Jul 30, 2015

Conversation

src-code
Copy link
Contributor

Escaped colons in a classname break the entire declaration in IE < 8, so we can't combine selectors that contain them with other selectors if we want the other selectors to work properly in old IE.

Example:

Input:

D(ib)
More:h_D(ib)
IbBox
LineClamp(2,3em)

Output before fix:

.D\(ib\), .More:hover .More\:h_D\(ib\), .IbBox, a[class*=LineClamp], a[class*=LineClamp]:after {
  display: inline-block;
}
.D\(ib\), .More:hover .More\:h_D\(ib\), .IbBox, a[class*=LineClamp] {
  zoom: 1;
  *display: inline;
}
.IbBox {
  vertical-align: top;
}
[class*=LineClamp] {
  display: -webkit-box;
  -webkit-box-orient: vertical;
}
[class*=LineClamp], a[class*=LineClamp]:after {
  overflow: hidden;
}
a[class*=LineClamp] {
  display : -webkit-box;
}
a[class*=LineClamp]:after {
  content: ".";
  font-size: 0;
  visibility: hidden;
  height: 0;
  width: 0;
}
.LineClamp\(2\) {
  -webkit-line-clamp: 2;
  max-height: ;
}

Output after fix:

.D\(ib\), .IbBox, a[class*=LineClamp] {
  display: inline-block;
  zoom: 1;
  *display: inline;
}
.More:hover .More\:h_D\(ib\) {
  display: inline-block;
  zoom: 1;
  *display: inline;
}
.IbBox {
  vertical-align: top;
}
[class*=LineClamp] {
  display: -webkit-box;
  -webkit-box-orient: vertical;
  overflow: hidden;
}
a[class*=LineClamp] {
  display : -webkit-box;
}
a[class*=LineClamp]:after {
  content: ".";
  font-size: 0;
  visibility: hidden;
  display: inline-block;
  overflow: hidden;
  height: 0;
  width: 0;
}
.LineClamp\(2\,3em\) {
  -webkit-line-clamp: 2;
  max-height: 3em;
}

@src-code
Copy link
Contributor Author

I'll need to fix our tests to expect this changed output, but want to get eyes on this first before I do that.

fyi @renatoi @thierryk @3den @dnewcome

for(var j = i + 1; j < l; j += 1) {
// If this selector has an escaped colon, we can't safely combine it
// with another selector since it will break in IE < 8
if (extracts[j].selector && extracts[j].selector.indexOf('\:') > -1) { continue; }
Copy link

Choose a reason for hiding this comment

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

do we really need the nested loop? If we can use a single loop it would increase the performance, also line 83 and 87 are pretty much the same i am wondering if we can remove this duplication.

Problem: extracted seems to be an array of objects, the challenge we have is to find the keys that map to the same value in order to create the selector and to do that we take current O(n^2).

Suggestion: we can think about this as an inverted hash where the key is the style we are applying and the value is an array of selectors that match that value every time you find a new selector we just push it to the corresponding style, the object could look something like:

{
   'display: inline-block;': ['.D\(ib\)', '.IbBox', 'a[class*=LineClamp]']
}

ps: I don't know if that suggestion would work better so fell free to ignore it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to open a new issue to track this performance issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@src-code
Copy link
Contributor Author

@3den I agree the code performance could be much improved. Right now we need this check in two places because of the nested loop (eg, we have to test the current node AND the node being considered for merging, because if either has an escaped colon, it's a no-go). I'm not familiar enough with this JSS lib to know if it's realistic to simplify that data structure but I think it's worth investigating outside the scope of this pull request.

@3den
Copy link

3den commented Jul 30, 2015

looks good 👍

@src-code src-code added this to the 3.2.0 milestone Jul 30, 2015
src-code added a commit that referenced this pull request Jul 30, 2015
Don't combine selectors if the selector contains an escaped colon
@src-code src-code merged commit aae6f28 into master Jul 30, 2015
@src-code src-code deleted the no-combine branch July 30, 2015 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants