Skip to content

Commit

Permalink
fix: Conflict Resolutions with Media Queries.
Browse files Browse the repository at this point in the history
This fixes two bugs:

1. Conlict resolution was skipping resolution rules if the resolved
   selector was the same, but this missed the use case where the
   selector could be in a conditional at-rule. I removed the
   deduplication.  Fixing this uncovered the second issue.

2. We were only preserving the conditional at-rule context on the local
   side of the conflict resolution. If the conditional was on the remote
   side of the conflict that context was being lost. To fix this, we now
   clone the conditional at-rules that form the context of the remote
   selector into the local stylesheet around the resolution rule.

Closes #372.
  • Loading branch information
chriseppstein committed Dec 18, 2019
1 parent 488e23e commit c189613
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 5 deletions.
46 changes: 42 additions & 4 deletions packages/@css-blocks/core/src/BlockCompiler/ConflictResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ export class ConflictResolver {
}

// Crawl up inheritance tree of the other block and attempt to resolve the conflict at each level.
// XXX Should this really abort when it finds the first conflict?
let foundConflict = ConflictType.noConflict;
do {
foundConflict = this.resolveConflictWith(resolution.path, other, res);
Expand Down Expand Up @@ -244,7 +245,6 @@ export class ConflictResolver {

// Something to consider: when resolving against a sub-block that has overridden a property, do we need
// to include the base object selector(s) in the key selector as well?
const resolvedSelectors = new Set<string>();
const query = new QueryKeySelector(other);
const result = query.execute(root, other.block);
let foundConflict: ConflictType = ConflictType.noConflict;
Expand All @@ -261,9 +261,8 @@ export class ConflictResolver {

// avoid duplicate selector via permutation
let newSelStr = newSelectors.join(",\n");
if (resolvedSelectors.has(newSelStr)) { continue; }
resolvedSelectors.add(newSelStr);
let newRule = postcss.rule({ selector: newSelStr });
let newRuleContext = reproduceContext(s.rule, newRule);

// For every declaration in the other ruleset,
const remoteDecls: SimpleDecl[] = [];
Expand Down Expand Up @@ -311,7 +310,7 @@ export class ConflictResolver {
let parent = decl.parent.parent;
if (parent) {
let rule = decl.parent as postcss.Rule;
parent.insertAfter(rule, newRule);
parent.insertAfter(rule, newRuleContext);
}
}
}
Expand Down Expand Up @@ -440,3 +439,42 @@ export class ConflictResolver {
return sourceRange(this.config, block.stylesheet, blockPath, node);
}
}

interface InstanceOf<T> {
constructor: { new (): T };
}

function shallowClone(node: InstanceOf<postcss.Container>) {
let cloned = new node.constructor();

for (let i in node) {
if (!node.hasOwnProperty(i)) continue;
let value = node[i];
let type = typeof value;

if (i === "parent" && type === "object") {
// skip it
} else if (i === "source") {
cloned[i] = value;
} else if (value instanceof Array) {
// skip it
} else if (type === "object" && value !== null) {
// skip it
} else {
cloned[i] = value;
}
}

return cloned;
}

function reproduceContext(hasContext: postcss.Node, needsContext: postcss.Node): postcss.Node {
if (!hasContext.parent || hasContext.parent.type === "root") {
return needsContext;
}
let parent = hasContext.parent;
// The typings for postcss don't model the nodes as classes so we have to cast through unknown.
let newParent = shallowClone(<InstanceOf<postcss.Container>><unknown>parent);
newParent.append(needsContext);
return reproduceContext(parent, newParent);
}
2 changes: 1 addition & 1 deletion packages/@css-blocks/core/src/BlockTree/BlockClass.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ export class BlockClass extends Style<BlockClass, Block, Block, Attribute> {
*
* @param optionalRoot The root class is optional on root-level
* Attributes. So when these attributes are being used in conjunction
* with a Attributes, this value is set to true.
* with attributes, this value is set to true.
*/
public asSourceAttributes(optionalRoot = false): Attr[] {
if (!this._sourceAttribute) {
Expand Down
68 changes: 68 additions & 0 deletions packages/@css-blocks/core/test/resolution-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,74 @@ export class BlockInheritance extends BEMProcessor {
});
}

@test "resolves block roots in media queries"() {
let { imports, config } = setupImporting();
imports.registerSource(
"grid.block.css",
`:scope {
width: 1128px;
box-sizing: content-box;
padding: 0 30px;
display: block;
margin: auto;
position: relative;
}
@media (max-width: 1208px) {
:scope {
display: inline-block;
width: calc(100vw - 20px);
box-sizing: border-box;
}
}
@media (max-width: 976px) {
:scope {
padding: 0 18px;
}
}
`,
);

let filename = "header.css";
let inputCSS = `
@block grid from "./grid.block.css";
:scope {
background: blue;
}
.content {
display: flex;
display: resolve("grid");
height: 100%;
}
.content[width="fixed"] {
composes: grid;
}
.content[width="full"] {
margin: resolve("grid");
margin: 0 24px;
}
`;

return this.process(filename, inputCSS, config).then((result) => {
imports.assertImported("grid.block.css");
assert.deepEqual(
result.css.toString(),
".header { background: blue; }\n" +
".header__content { display: flex; height: 100%; }\n" +
".grid.header__content { display: block; }\n" +
"@media (max-width: 1208px) {\n" +
" .grid.header__content { display: inline-block; }\n" +
"}\n" +
".header__content--width-fixed { }\n" +
".header__content--width-full { margin: 0 24px; }\n" +
".grid.header__content--width-full { margin: 0 24px; }\n",
);
});
}

@test "resolves root states"() {
let { imports, config } = setupImporting();
imports.registerSource(
Expand Down

0 comments on commit c189613

Please sign in to comment.