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

Improve compatibility with compound selectors #126

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions lib/is-node-under-scope.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
var isUnderScope = require('./is-under-scope');
Copy link
Owner

Choose a reason for hiding this comment

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

I actually have to admit that I'm not using this plugin any more due to being able to drop IE11 support.

It was used in production and I cannot really remember any issues.

As it's been so long since I made the changes I know exactly as much as you on how and why they were made. : )

#126 (comment)

@siilike No worries, I understand. I might just add the comments to the tests where I pointed it out and merge this.

Seems like a decent improvement regardless of the code and us trying to understand it better.

var generateScopeList = require('./generate-scope-list');

var isNodeUnderScope = function(node, scopeNode, /*optional*/ignorePseudo) {
var isNodeUnderScope = function(node, scopeNode, /*optional*/ignorePseudo, /*optional*/keepPseudo) {
var nodeScopeList = generateScopeList(node, true);
var scopeNodeScopeList = generateScopeList(scopeNode, true);

return isUnderScope(nodeScopeList, scopeNodeScopeList, ignorePseudo);
return isUnderScope(nodeScopeList, scopeNodeScopeList, ignorePseudo, keepPseudo);
};

module.exports = isNodeUnderScope;
39 changes: 34 additions & 5 deletions lib/is-under-scope.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
var log = require('debug')('postcss-css-variables:is-under-scope');
Copy link
Owner

Choose a reason for hiding this comment

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

👍 on debug


var escapeStringRegexp = require('escape-string-regexp');

var isPieceAlwaysAncestorSelector = require('./is-piece-always-ancestor-selector');
Expand All @@ -14,26 +16,34 @@ function getScopeMatchResults(nodeScopeList, scopeNodeScopeList) {
var currentPieceOffset;
var scopePieceIndex;

log('######### getScopeMatchResults', nodeScopeList, scopeNodeScopeList);

// Check each comma separated piece of the complex selector
var doesMatchScope = scopeNodeScopeList.some(function(scopeNodeScopePieces) {
return nodeScopeList.some(function(nodeScopePieces) {

//console.log('sp', scopeNodeScopePieces);
//console.log('np', nodeScopePieces);

log(nodeScopePieces, scopeNodeScopePieces);

currentPieceOffset = null;
var wasEveryPieceFound = true;
for(scopePieceIndex = 0; scopePieceIndex < scopeNodeScopePieces.length; scopePieceIndex++) {
var scopePiece = scopeNodeScopePieces[scopePieceIndex];
var pieceOffset = currentPieceOffset || 0;

log('###### LOOP1', scopePiece, pieceOffset);

var foundIndex = -1;
// Look through the remaining pieces(start from the offset)
var piecesWeCanMatch = nodeScopePieces.slice(pieceOffset);
for(var nodeScopePieceIndex = 0; nodeScopePieceIndex < piecesWeCanMatch.length; nodeScopePieceIndex++) {
var nodeScopePiece = piecesWeCanMatch[nodeScopePieceIndex];
var overallIndex = pieceOffset + nodeScopePieceIndex;

log('### LOOP2', nodeScopePiece, overallIndex);

// Find the scope piece at the end of the node selector
// Last-occurence
if(
Expand All @@ -42,6 +52,7 @@ function getScopeMatchResults(nodeScopeList, scopeNodeScopeList) {
// scopePiece `.bar` matches node `.foo + .bar`
new RegExp(escapeStringRegexp(scopePiece) + '$').test(nodeScopePiece)
) {
log('break 1', scopePiece, nodeScopePiece);
foundIndex = overallIndex;
break;
}
Expand All @@ -54,15 +65,19 @@ function getScopeMatchResults(nodeScopeList, scopeNodeScopeList) {
if(isPieceAlwaysAncestorSelector(scopePiece) || isPieceAlwaysAncestorSelector(nodeScopePiece)) {
foundIndex = overallIndex;

log('break 2', scopePiece, isPieceAlwaysAncestorSelector(scopePiece), nodeScopePiece, isPieceAlwaysAncestorSelector(nodeScopePiece));
break;
}


// Handle any direct descendant operators in each piece
var directDescendantPieces = generateDirectDescendantPiecesFromSelector(nodeScopePiece);

// Only try to work out direct descendants if there was the `>` combinator, meaning multiple pieces
if(directDescendantPieces.length > 1) {

log('directDescendantPieces', directDescendantPieces);

var ddNodeScopeList = [].concat([directDescendantPieces]);
// Massage into a direct descendant separated list
var ddScopeList = [].concat([
Expand All @@ -83,6 +98,13 @@ function getScopeMatchResults(nodeScopeList, scopeNodeScopeList) {
scopePieceIndex += result.scopePieceIndex - 1;
}

log('break 3', result.doesMatchScope);

// if(!result.doesMatchScope)
// {
// wasEveryPieceFound = false;
// }

break;
}
}
Expand All @@ -95,14 +117,19 @@ function getScopeMatchResults(nodeScopeList, scopeNodeScopeList) {
// Mimicing a `[].every` with a for-loop
wasEveryPieceFound = wasEveryPieceFound && isFurther;
if(!wasEveryPieceFound) {
log('break 4');
break;
}
}

log('###### FOUND', wasEveryPieceFound, scopePiece, nodeScopePiece);

return wasEveryPieceFound;
});
});

log('######### RETURN', doesMatchScope, nodeScopeList, scopeNodeScopeList);

return {
doesMatchScope: doesMatchScope,
nodeScopePieceIndex: currentPieceOffset - 1,
Expand Down Expand Up @@ -133,11 +160,13 @@ var stripPseudoSelectorsFromScopeList = function(scopeList) {
// Another way to think about it: Can the target scope cascade properties to the node?
//
// For scope-lists see: `generateScopeList`
var isUnderScope = function(nodeScopeList, scopeNodeScopeList, /*optional*/ignorePseudo) {
// Because we only care about the scopeNodeScope matching to the nodeScope
// Remove the pseudo selectors from the nodeScope so it can match a broader version
// ex. `.foo:hover` can resolve variables from `.foo`
nodeScopeList = stripPseudoSelectorsFromScopeList(nodeScopeList);
var isUnderScope = function(nodeScopeList, scopeNodeScopeList, /*optional*/ignorePseudo, /*optional*/keepPseudo) {
Copy link
Owner

Choose a reason for hiding this comment

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

Can we document this dichotomy between ignorePseudo and keepPseudo? I'm not quite wrapping my head around why we need both options and them being opposite default states. Seems like they should both be ignoreX or keepX and maybe need better descriptors.

For example, just from an initial look, seems like ignorePseudo can cover keepPseudo by being ignorePseudo=false. But I understand they are probably tackling different use cases. So the parameter name should probably be different.

if(!keepPseudo) {
// Because we only care about the scopeNodeScope matching to the nodeScope
// Remove the pseudo selectors from the nodeScope so it can match a broader version
// ex. `.foo:hover` can resolve variables from `.foo`
nodeScopeList = stripPseudoSelectorsFromScopeList(nodeScopeList);
}

if(ignorePseudo) {
scopeNodeScopeList = stripPseudoSelectorsFromScopeList(scopeNodeScopeList);
Expand Down
188 changes: 172 additions & 16 deletions lib/resolve-decl.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@

var log = require('debug')('postcss-css-variables:resolve-decl');

var resolveValue = require('./resolve-value');
var generateScopeList = require('./generate-scope-list');
var gatherVariableDependencies = require('./gather-variable-dependencies');
Expand All @@ -9,16 +12,91 @@ var shallowCloneNode = require('./shallow-clone-node');
var findNodeAncestorWithSelector = require('./find-node-ancestor-with-selector');
var cloneSpliceParentOntoNodeWhen = require('./clone-splice-parent-onto-node-when');

var parser = require('postcss-selector-parser');
Copy link
Owner

@MadLittleMods MadLittleMods Apr 7, 2021

Choose a reason for hiding this comment

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

Fantastic! I started trying to use this in https://github.com/MadLittleMods/postcss-selector-scope-utility to replace all of the logic here but never got it over the line.


function equals(a, b) {
return a && b && a.type !== undefined && a.value !== undefined && a.type === b.type && a.value === b.value;
}

function compatible(a, b) {
var af = a.filter(c => !b.find(d => equals(d, c)));
var bf = b.filter(c => !a.find(d => equals(d, c)));
var ac = a.find(c => c.type == 'combinator');
var bc = b.find(c => c.type == 'combinator');

if(bf.length == 0 && ((!ac && !bc) || equals(ac, bc)))
{
return true;
}

return false;
}

function compatible2(a, b) {
var af = a.filter(c => !b.find(d => equals(d, c)));
var bf = b.filter(c => !a.find(d => equals(d, c)));

if(bf.length == 0 && af.length != 0)
{
return true;
}

return false;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add a comment describing how compatible and compatible2 are different and when you should use either


function isSpace(a) {
return a && a.type === 'combinator' && a.value.trim() === '';
}

function findIndex(s, fn, start)
{
if(!start) {
start = 0;
}
else {
s = s.slice(start);
}

var pos = s.findIndex(fn);

return pos === -1 ? -1 : s.findIndex(fn) + start;
}

function parseSelector(selector) {
var ret;

parser(selectors => {
ret = selectors.first.split(selector => selector.type === 'combinator').map(a => a.filter(b => !isSpace(b)));
}).processSync(selector);

return ret;
}

function removeWildcard(selector) {
return parser(selectors => {
selectors.first.nodes[selectors.first.nodes.length-1].remove();
}).processSync(selector, { updateSelector: false });
}

function eachMapItemDependencyOfDecl(variablesUsedList, map, decl, cb) {
log('>>>>>>>>> DECL ' + decl.parent.selector + ' / ' + decl.prop + ' = ' + decl.value);

var declSelector = decl.parent.selector ? parseSelector(decl.parent) : null;

log(declSelector);

// Now find any at-rule declarations that pertains to each rule
// Loop through the variables used
variablesUsedList.forEach(function(variableUsedName) {

log('>>>>>> VAR '+variableUsedName);

// Find anything in the map that corresponds to that variable
gatherVariableDependencies(variablesUsedList, map).deps.forEach(function(mapItem) {
gatherVariableDependencies(variablesUsedList, map).deps.forEach(function(mapItem, i) {

log('>>> DEP '+i+' / ' + mapItem.parent.selector + ' / ' + mapItem.prop + ' = ' + mapItem.calculatedInPlaceValue);

var state = 0;
var mimicDecl;
if(mapItem.isUnderAtRule) {

Expand All @@ -41,29 +119,107 @@ function eachMapItemDependencyOfDecl(variablesUsedList, map, decl, cb) {
//console.log(generateScopeList(mapItem.parent, true));
//console.log('amd isNodeUnderScope', isNodeUnderScope(mimicDecl.parent, mapItem.parent), mapItem.decl.value);
}
// TODO: use regex from `isUnderScope`
else if(isUnderScope.RE_PSEUDO_SELECTOR.test(mapItem.parent.selector)) {
// Create a detached clone
var ruleClone = shallowCloneNode(decl.parent);
ruleClone.parent = decl.parent.parent;

// Add the declaration to it
mimicDecl = decl.clone();
ruleClone.append(mimicDecl);

var lastPseudoSelectorMatches = mapItem.parent.selector.match(new RegExp(isUnderScope.RE_PSEUDO_SELECTOR.source + '$'));
var lastPseudoSelector = lastPseudoSelectorMatches ? lastPseudoSelectorMatches[2] : '';

ruleClone.selector += lastPseudoSelector;
else if(mapItem.parent.selector !== decl.parent.selector && declSelector) {
var s = parseSelector(mapItem.parent);

log(s);

var append = null;
var idx = 0;
var process = s.length > 1;
for(var i = 0; i < declSelector.length; i++) {
var a = declSelector[i];
var b = findIndex(s, c => compatible(c, a), idx);
process |= s.findIndex(c => compatible2(c, a), idx) != -1;

if(b < idx) {
// no match: already compatible or wildcard
if(i === 0) {
state = 1;
Copy link
Owner

Choose a reason for hiding this comment

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

Can these states be nice string label constants or symbols? Reasoning about these arbitrary state numbers seems mind bending 🤯

break;
}
// invalid unless last item
else if(i != declSelector.length-1) {
state = 2;
break;
}

log('append', a);
append = a;
}
else {
// check if the element following the combinator is compatible
if(s[b].find(a => a.type === 'combinator') && !equals(declSelector[i+1][0], s[b+1][0])) {
state = 6;
break;
}

idx = b;
}
}

// #foo li:hover a @ compound16.css
if(state === 0 && !append && idx != s.length-1) {
state = 3;
}

// already compatible
if(state === 0 && !process) {
state = 4;
}

// compound20.css
if(state === 1 && s.length != 1 && s[s.length-1].length === 1 && s[s.length-1][0].type === 'universal') {
state = 5;
}

log("STATE", state);

if(state === 0 || state === 5) {
// Create a detached clone
var ruleClone = shallowCloneNode(decl.parent);
ruleClone.parent = decl.parent.parent;

// Add the declaration to it
mimicDecl = decl.clone();

ruleClone.append(mimicDecl);

if(state === 0) {
ruleClone.selector = mapItem.parent.selector;

// append last element of selector where needed
if(append) {
ruleClone.selector += ' ';
append.forEach(a => ruleClone.selector += String(a));
}
}
else if(state === 5) {
ruleClone.selector = removeWildcard(mapItem.parent.selector).trim() + ' ' + decl.parent.selector;
}
else {
throw new Error("Invalid state: "+state);
}
}
}

// If it is under the proper scope,
// we need to check because we are iterating over all map entries
if(mimicDecl && isNodeUnderScope(mimicDecl, mapItem.parent, true)) {
var valid = state === 5 || (mimicDecl && isNodeUnderScope(mimicDecl, mapItem.parent, true));

if(valid) {
cb(mimicDecl, mapItem);
}

log('SELECTOR '+(mimicDecl ? mimicDecl.parent.selector : null));
log('VALID? '+valid);
log('<<< DEP '+i+' / ' + mapItem.parent.selector + ' / ' + mapItem.prop + ' = ' + mapItem.calculatedInPlaceValue);
});

log('<<<<<< VAR '+variableUsedName);
});

log('<<<<<<<<< DECL ' + decl.parent.selector + ' / ' + decl.prop + ' = ' + decl.value);
}


Expand Down
Loading