leafo / lessphp

LESS compiler written in PHP
http://leafo.net/lessphp
Other
2.2k stars 528 forks source link

Calculated values precision #324

Open Ninir opened 12 years ago

Ninir commented 12 years ago

When calculating values with percentages (and probably other, didn't checked), it seems the calculation has not the precision given by lessjs.

Here is an example using twitter bootstrap for instance. To make a 100% row, we should do something like:

span3 - span9, which results in a span12 which should be 100%.

However, here are the measures I get when loading the same page with less JS or less PHP:

Calculations

span3 Less PHP width : 23,4042553191% Less JS width: 23,076923076923077% Less PHP margin left: 2,5641025641% Less JS margin left : 2,564102564102564%

span9 Less PHP width : 74,4680851064% Less JS width: 74,35897435897436% Less PHP margin left: 2,5641025641% Less JS margin left : 2,564102564102564%

Now, using the formula (we don't care about the left element margin-left as it is always 0, so here, we don't care about the span3 margin-left) span3 width + span9 width + span9 margin left = Result

Results

lessphp 23,4042553191 + 74,4680851064 + 2,5641025641 = 100,4364429896

lessjs 23,076923076923077 + 74,35897435897436 + 2,564102564102564 = 100

@leafo Is there a chance to fix this as soon as possible? or any hint on how to fix it please?

leafo commented 12 years ago

I don't know if there is anything I can do about the precision. I am using PHP's native numeric operators.

In any case, it doesn't look like a precision problem. The results for width on lessphp appear to be just wrong. What function (or what is the math) you are using to generate them. Both of them have a pretty big differences:

Less PHP width : 23.4042553191% 
Less JS width: 23.076923076923077%

A difference of 0.3 shouldn't just pop out of nowhere.

stof commented 12 years ago

@leafo This is the calculation done in Twitter Bootstrap with the default values for variables.

ezp-matt commented 11 years ago

I'm running into this issue as well, and it's a fairly serious problem because it breaks Bootstrap's responsive layouts. If this is merely an issue of precision, maybe a possible solution is to check to see if the server has the BC math extension installed, and use that for arbitrary precision calculations instead?

ezp-matt commented 11 years ago

ok, as a test I rewrote op_number_number() to use the bcmath functions, but the numbers still end up the same as PHP's native functions.. the numbers are off by quite a bit (as per leafo's example) so it does seem as though the calculations are possibly being done wrong somewhere. I'll tinker around and see what I can figure out.

ds125v commented 11 years ago

If you compare the bootstrap-responsive.css directly from Bootstrap project with one compiled with lessphp then the figures match exactly (well, php displays 11 digits after the . which all match with the js output, which also has an extra 3 digits, but we'll call that the same) all the way up to: @media (min-width: 768px) and (max-width: 979px) {

.rowfluid .span12 { width: 1.2345

and then from the next one onward

.row-fluid .span11 {

where the figures vary by between 0.1 and 0.4 difference.

until offset 12 which is correct again,

.row-fluid .offset12:first-child {

but then from the next one again:

row-fluid .offset11 { margin-left: 1.2345 This matches with my experience of the less bootstrap being broken between 768px and 979px and then working again between 980px to 1199px but it's not obvious why my layout falls apart again at 1200px and above

ds125v commented 11 years ago

Compiling this with the respective Bootstrap classes (I'm using the new 2.2 version):

@import "variables.less"; // Modify this for custom colors, font-sizes, etc @import "mixins.less"; @import "responsive-utilities.less"; @import "responsive-768px-979px.less";

gives the expected output that matches with the recess compiler.

Importing the 1200px responsive changes the numbers outputted for the responsive 768-979 .fluid grid:

@import "variables.less"; // Modify this for custom colors, font-sizes, etc @import "mixins.less"; @import "responsive-utilities.less"; @import "responsive-1200px-min.less"; @import "responsive-768px-979px.less";

Why? I have no idea. Those files call mixins.less and #grid > .fluid to do the maths but I can't figure out why it would get the right/same answer for the first call (12) and then get the rest wrong.

ds125v commented 11 years ago

Actually, a clue to why it works for the first one but not the others is possibly that the .spanX iterates by calling itself recursively to count down from 12 to 1 so that's probably something to check out.

ezp-matt commented 11 years ago

I've broken this down into the simplest test-case I could do, and it definitely seems to have something to do with the recursive function. Here we go:

.set (@width) {

  .spanX (@index) when (@index > 0) {
    (~".span@{index}") { width: @width; }
    .spanX(@index - 1);
  }

  .spanX (0) {}

  .row {
    .spanX (3);
  }
}

.set1 {
.set(10px);
}

.set2 {
.set(33px);
}

With lessphp, you'll see that in .set1 all the rows get a width of 10px, but in .set2, .span3 gets a width of 33px, and span1 and span2 get a width of 10px, whereas they should all get a width of 33px (and they do with lessc). Pretty weird. Even weirder, if we take out the .row nesting, like so:

.set (@width) {

  .spanX (@index) when (@index > 0) {
    (~".span@{index}") { width: @width; }
    .spanX(@index - 1);
  }

  .spanX (0) {}

  .spanX (3);
}

.set1 {
.set(10px);
}

.set2 {
.set(33px);
}

...it works fine. Also, if we take out the recursive functionality (and leave the .row nesting as in the first example):

.set (@width) {

  .spanX (@index) {
    (~".span@{index}") { width: @width; }
  }

  .spanX (0) {}

  .row {
    .spanX (3);
    .spanX (2);
    .spanX (1);
  }
}

.set1 {
.set(10px);
}

.set2 {
.set(33px);
}

...that works as expected as well. I'm at a loss as to where to even begin looking for a place to fix a problem like that, but hopefully that'll help someone else track it down ;)

P.S. the reason this breaks Bootstrap's responsive layouts is because the responsive media queries are calling the same mixin repeatedly, like the example above, so the subsequent media queries are incorrectly duplicating the values from the first media query. The correct values are fairly close, which is why it looks to be a precision issue at first glance.

dynamick commented 11 years ago

I've the same problem :-(

stof commented 11 years ago

@dynamick you can use my branch (see the PR linked above) which compiles BS responsive properly. Unforunately, it introduces some other bugs when using complex mixin constructs (and I haven't spent time on it yet as we considered the bug minor in our app compared to the broken layout)

dynamick commented 11 years ago

Your PR fix the problem! Thanks!

jozefspisiak commented 11 years ago

It worries me, that this issue is for over 5 months still open. It causes to break twitter bootstrap fluid grid system, which is used a lot. Could this be merged to master? Or did someone write test cases for the other bugs which this introduces?

Ninir commented 11 years ago

@jozefspisiak Please use this branch: https://github.com/Incenteev/lessphp/tree/recursion

You should however be aware that it introduces some issues as @stof wrote:

@dynamick you can use my branch (see the PR linked above) which compiles BS responsive properly. Unforunately, it introduces some other bugs when using complex mixin constructs (and I haven't spent time on it yet as we considered the bug minor in our app compared to the broken layout)

laoneo commented 10 years ago

Thanks you very much for this fix. Can't believe it is over a year old :-)