Last modified: 2014-09-24 01:00:35 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T56673, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 54673 - LESS compiler should preserve the position of CSSMin / CSSJanus annotations
LESS compiler should preserve the position of CSSMin / CSSJanus annotations
Status: RESOLVED WONTFIX
Product: MediaWiki
Classification: Unclassified
ResourceLoader (Other open bugs)
1.22.0
All All
: High normal (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks: remove-custom-less
  Show dependency treegraph
 
Reported: 2013-09-27 00:01 UTC by Ori Livneh
Modified: 2014-09-24 01:00 UTC (History)
13 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Ori Livneh 2013-09-27 00:01:19 UTC
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.
Comment 1 Bartosz Dziewoński 2013-09-27 00:13:33 UTC
(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.)
Comment 2 Ori Livneh 2013-09-29 21:26:33 UTC
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.
Comment 3 Ori Livneh 2013-09-29 21:39:38 UTC
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/>
Comment 4 Gerrit Notification Bot 2013-10-03 19:43:25 UTC
Change 86616 had a related patch set uploaded by Ori.livneh:
Update lessphp to 261f1bd28

https://gerrit.wikimedia.org/r/86616
Comment 5 Gerrit Notification Bot 2013-10-04 00:26:51 UTC
Change 86616 merged by jenkins-bot:
Update lessphp to 261f1bd28

https://gerrit.wikimedia.org/r/86616
Comment 6 Ori Livneh 2013-10-04 00:30:13 UTC
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.
Comment 7 Kevin Israel (PleaseStand) 2013-10-16 16:02:44 UTC
This is still not completely fixed. See bug 55779.
Comment 8 Andre Klapper 2013-12-07 02:37:51 UTC
[Bumping TM as MediaWiki 1.22.0 tarball was released today.]
Comment 9 Bartosz Dziewoński 2014-05-05 10:07:41 UTC
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.
Comment 10 Mark A. Hershberger 2014-06-21 19:58:03 UTC
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.
Comment 11 Matthew Flaschen 2014-07-26 01:36:22 UTC
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 ).
Comment 12 Bartosz Dziewoński 2014-09-18 15:20:33 UTC
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.
Comment 13 Matthew Flaschen 2014-09-24 00:59:21 UTC
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?

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links