Page MenuHomePhabricator

LESS compiler should preserve the position of CSSMin / CSSJanus annotations
Closed, DeclinedPublic

Description

We have some fancy ideas for handling image embedding and flipping in LESS, but until they are realized, LESS should preserve CSS @embed and @noflip annotations.

The problematic bits in lessc.inc.php that need to be modified are in lines 280-286:

protected function compileProps($block, $out) {

		foreach ($this->sortProps($block->props) as $prop) {
			$this->compileProp($prop, $block, $out);
		}

		$out->lines = array_values(array_unique($out->lines));

}

Required changes:

  • Call to array_unique() should be replaced with something that exempts comments, because it is valid to have multiple /* @embed */ annotations in a single block. (Maybe we can just get rid of 'array_unique' -- it could be a nonstandard performance optimization.)
  • sortProps should preserve the position of @annotations relative to the rules below them.

Version: 1.22.0
Severity: normal
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=55779
https://bugzilla.wikimedia.org/show_bug.cgi?id=61941
https://bugzilla.wikimedia.org/show_bug.cgi?id=67368

Details

Reference
bz54673

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 2:19 AM
bzimport set Reference to bz54673.
bzimport added a subscriber: Unknown Object (MLST).

(Marking with 1.22 milestone – we really should find a way to make embedding and flipping work with LESS before the release, with this or otherwise.)

Patch available in my fork https://github.com/atdt/lessphp/commit/55040667a6620fa4b546c050219b3a49a40ac896. Going to test it a bit more and then submit it upstream if everything looks OK. Additional code-review / testing would be much appreciated.

To make my changes easier to test against MediaWiki code, I submitted my modifications as Gerrit change Ib6bc76736.

https://gerrit.wikimedia.org/r/#/c/86616/

Change 86616 had a related patch set uploaded by Ori.livneh:
Update lessphp to 261f1bd28

https://gerrit.wikimedia.org/r/86616

Change 86616 merged by jenkins-bot:
Update lessphp to 261f1bd28

https://gerrit.wikimedia.org/r/86616

Closing as fixed. I still do think we could handle embedding / flipping better via LESS constructs, but I'm glad we have time to work on it now.

This is still not completely fixed. See bug 55779.

[Bumping TM as MediaWiki 1.22.0 tarball was released today.]

To summarize:

This works correctly, if there is only one instance of each property in the block (a property might be repeated several times for browser hacks, see bug 61941):

.class {
    /* @noflip */
    margin-left: 1em;
    /* @noflip */
    margin-right: 2em;
}

This doesn't generally work, although sometimes it does due to pure chance:

/* @noflip */
.class {
    margin-left: 1em;
    margin-right: 2em;
}

See https://gerrit.wikimedia.org/r/#/c/90169/ / bug 55779 for an example workaround for this.

Removing target milestone that was in the past.

If you want this in a specific release, have a good reason AND you are willing to find resources to fix this bug, feel free to change it to something appropriate.

Blocks was correct. It blocks bug 67368 because (if we do that), this has to be done first (we have custom functions for working around the annotation problem at https://git.wikimedia.org/blob/mediawiki%2Fcore.git/362d7c1e83cf424acf2021dd4a4ce2ad11062e45/includes%2Fresourceloader%2FResourceLoaderLESSFunctions.php#L38 ).

For /* @embed */ annotations, it turns out it is actually possible to use them without lessc mucking about, using the "infix" syntax:

background-image: /* @embed */ url(file.png);

I3062346ed63272a1c22b5df27b4cc1de2a699d9a demonstrates an implementation and hopefully fixes/works-around the last bug we had because of this underlying lessc issue.

Considering Bartosz's patch (https://gerrit.wikimedia.org/r/#/c/161245/) solved this in a different way, and this is an upstream (lessphp) issue, should we WONTFIX this?