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

Can't create elliptical border-radius #146

Closed
marioestrada opened this issue Nov 18, 2010 · 28 comments
Closed

Can't create elliptical border-radius #146

marioestrada opened this issue Nov 18, 2010 · 28 comments

Comments

@marioestrada
Copy link

@marioestrada marioestrada commented Nov 18, 2010

With the revised CSS3 specification it's possible to create elliptical border radius with the following syntax:
-webkit-border-radius: 40px/10px;
-moz-border-radius: 40px/10px;
border-radius: 40px/10px;

But less parses this and it calculates 40/10 so it actually becomes 5, like so:
-webkit-border-radius: 5px;
-moz-border-radius: 5px;
border-radius: 5px;

There should be a way to write this syntax without Less parsing it.

@neonstalwart
Copy link
Contributor

@neonstalwart neonstalwart commented Nov 22, 2010

i wanted to add a similar/related problem which happens with the font property when declaring a line-height

.class {
    font: 13px/1.231 arial, helvetica, clean, sans-serif;
} 

it should remain unchanged but it becomes:
.class {
font:10.560519902518276px arial,helvetica,clean,sans-serif;
}

@lukeapage
Copy link
Member

@lukeapage lukeapage commented Sep 14, 2011

dotless also has this issue (dotless/dotless#16). It would be nice to come to some agreement how to proceed in both.

@ttfkam
Copy link
Contributor

@ttfkam ttfkam commented Sep 14, 2011

The simplest solution that works out of the box today is to wrap it in a string literal.

 font: ~"13px/1.231 arial, helvetica, clean, sans-serif";

or even

 font: ~"13px/1.231" arial, helvetica, clean, sans-serif;

@ChrisCinelli
Copy link

@ChrisCinelli ChrisCinelli commented Sep 15, 2011

I understood that one of the goals of less was to be compatible with normal CSS syntax. The work around break this rule.

@ttfkam
Copy link
Contributor

@ttfkam ttfkam commented Sep 15, 2011

I understand your frustration. I can't see an easy solution between Less's arithmetic and specific CSS properties. For example, what if you actually want to specify that the font is half the size, i.e.

 @example: 24pt;
 .foo {
     font: normal @example/2 sans-serif;
 }

Something has to compromise. Admittedly, you could alter Less so that any conversions are wrapped.

 font: normal (@example/2) sans-serif;

but that would break and make more verbose any existing Less stylesheets for all cases just to avoid cases which aren't as common for font and border-radius shorthand declarations. Another option if you are worried about existing CSS, you can import any existing CSS into your Less file:

 @import "legacy.css";

In this case, the import succeeds, but does not change or convert the CSS.

Again I fully understand your concern that Less is not a strict superset of CSS, but I lack any obviously better ideas. Do you have any potential solutions to this syntax problem?

@mlms13
Copy link

@mlms13 mlms13 commented Apr 11, 2012

👍 on this issue from me, but I'm also not sure of an elegant way to solve it that doesn't require either escaping valid CSS or requiring parentheses to trigger calculations.

@dmcass
Copy link
Contributor

@dmcass dmcass commented Aug 16, 2012

@agatronic @cloudhead I'm working on a fix for this, but I would love to get your input on how you think this should be handled before I take it any further. SASS and Stylus both handle this by only allowing division inside parentheses, all other mathematical operations can be done outside parens. This method will prevent any future implementations of the slash by W3C from breaking as well, and remove the need for the Ratio and Shorthand parsers. This is how I was first inclined to fix the issue. Is this a reasonable solution for both of you?

Note that this bug also breaks CSS3 background shorthand which uses the slash to separate background-position (which can be keywords or dimensions) from background-size. For example:

.foo {
    background: url(image.png) center / 1px 100px;
}

Also pinging @MatthewDL @Synchro

@lukeapage
Copy link
Member

@lukeapage lukeapage commented Aug 17, 2012

A previous suggestion was that ' / ' divides and '/' is a ratio. It would be good to get input from @cloudhead because though it would be really nice to get sorted, its pretty scary if it breaks existing behaviour...

@dmcass
Copy link
Contributor

@dmcass dmcass commented Aug 17, 2012

@cloudhead responded via twitter, somewhat minimally: "the paren solution seems decent"

The problem with the spaces solution is that still breaks perfectly valid CSS. Unfortunately the only way to address this effectively without breaking existing behavior is to continue to allow plain old CSS to fail parsing correctly.

@lukeapage
Copy link
Member

@lukeapage lukeapage commented Aug 17, 2012

how about an "temporary" option so that people can enjoy both for a grace period?

or

we should do a release just before merging this fix in and go up a major version number to 1.4 (and hopefully add in variables in includes and interpolation of property names etc.

@dmcass
Copy link
Contributor

@dmcass dmcass commented Aug 17, 2012

I think this definitely would be a fix that would have to be part of a more major release, since it would break existing behavior. I'm not sure a temporary solution (if we're talking about spaces) is even reasonable considering that it could still break existing code, and it doesn't provide any kind of clean transition into the permanent solution. This seems like a situation where pulling the trigger on a release is the best option, and people can be warned through the Change Log and Docs.

@lukeapage
Copy link
Member

@lukeapage lukeapage commented Aug 17, 2012

I meant flag to keep existing behaviour.. but I agree with you.

Do you have commit rights? Even if so might be best as a pull request so we
can coordinate with releases (we have no plans I know of)

@matthew-dean
Copy link
Member

@matthew-dean matthew-dean commented Aug 18, 2012

Personally, I think we should swallow the pill and begin listing non-parenthesized math operations as a deprecated feature. There's too many cases where it conflicts with valid CSS. @agatronic - It's worth a Skype discussion with @cloudhead (or elsewhere), and I hate to ruin anyone's LESS libraries, but LESS should not be ripping out and breaking CSS that the author did not intend. The LESS parser should be provided clear signals that a math operation is needed, and parentheses are a good way to do that. In parentheses: DO MATH. Not in parentheses: LEAVE IT ALONE.

As a phase 1 to lessen the blow, we could STOP doing math ONLY in cases where the division is ambiguous e.g. in places where a slash "/" is a valid token in that value. So, in the case of border radius, if LESS is not sure, it leaves it alone. If the author is sure they want division between the two numbers, they can override the default behavior with adding parentheses.

But, as it stands right now, LESS is documented as "valid CSS = valid LESS", and with this bug, that's not the case. It's falsely re-writing valid CSS, which makes it a clear bug. The problem is that it's ALSO a clearly documented math operation. The two are in conflict, and it seems clear to me that the second case is the one that should be altered, with CSS preserved.

@matthew-dean
Copy link
Member

@matthew-dean matthew-dean commented Aug 18, 2012

I see @ttfkam and @mlms13 said much the same thing. I think their instinct is correct, and I would strongly vote that LESS begin a slow migration to parentheses being a requirement for operations.

@lukeapage
Copy link
Member

@lukeapage lukeapage commented Aug 18, 2012

@dmcass
Copy link
Contributor

@dmcass dmcass commented Aug 20, 2012

@agatronic: I don't have commit rights, however I agree that this should be done through a PR anyways for code review and potential release coordination.

@MatthewDL: For the purposes of my PR, I've started work on only allowing division within parentheses. Consider it a stop-gap measure to prevent LESS from breaking valid CSS. It is not limited to specific rules, so anywhere a slash is encountered and it's not wrapped in parens, it will be output as a literal dilimiter. Once that's done I'll look a bit more at how to restrict all operations to only be done within parens.

@matthew-dean
Copy link
Member

@matthew-dean matthew-dean commented Aug 20, 2012

Okay, I've discussed this with @cloudhead on Skype. He's in agreement, so here is the plan:

  1. On the next release, the documentation will be updated that math outside of parentheses is deprecated. The documentation will demonstrate math operations as being in parentheses. However, non-division math outside of parentheses should still compile (for that release).

  2. On the subsequent release after step #1, ALL math operations will require parentheses. The documentation will change from "deprecated" to "not supported" (or something like that - how we communicate it can be refined).

Sound good? This means that we should start planning for what the milestones are for the next 2 releases, but that's outside the scope of this thread here.

@matthew-dean
Copy link
Member

@matthew-dean matthew-dean commented Aug 20, 2012

Actually, to clarify, the documentation can be updated NOW to say that math outside of parentheses is deprecated, since math IN parentheses works fine. So that's really Step 0: We update docs now to tell people a) Math should be in parentheses, and to not do so may break in the future, b) Demonstrate all math in parentheses.

@dmcass
Copy link
Contributor

@dmcass dmcass commented Aug 20, 2012

Works for me! 👍

@lukeapage
Copy link
Member

@lukeapage lukeapage commented Aug 21, 2012

so presumably @dmcass should do a pull request against
https://github.com/cloudhead/lesscss.org

and then we should hassle @cloudhead to commit or give us commit rights to that project?

@matthew-dean
Copy link
Member

@matthew-dean matthew-dean commented Aug 21, 2012

Oh, right. Yes, I'll try to remember to bug him today about it.

On 2012-08-21, at 5:19 AM, Luke Page notifications@github.com wrote:

so presumably @dmcass https://github.com/dmcass should do a pull request
against
https://github.com/cloudhead/lesscss.org

and then we should hassle @cloudhead https://github.com/cloudhead to
commit or give us commit rights to that project?


Reply to this email directly or view it on
GitHubhttps://github.com//issues/146#issuecomment-7899194.

@dmcass
Copy link
Contributor

@dmcass dmcass commented Aug 22, 2012

I've opened a PR to update the docs at less/old-lesscss.org#29

@lukeapage
Copy link
Member

@lukeapage lukeapage commented Feb 2, 2013

Fixed on master for 1.4.0

@lukeapage lukeapage closed this Feb 2, 2013
@lukeapage
Copy link
Member

@lukeapage lukeapage commented Feb 25, 2013

It will be like that in 1.4.0

Check out the alpha on less2css.org

@FellowshipAgency
Copy link

@FellowshipAgency FellowshipAgency commented Sep 6, 2013

This bug still happens in 1.4.1 if you use the following CSS:
border-radius:0 0 100% 100% / 0 0 24px 24px;

It outputs:
border-radius: 0 0 100% Infinity% 0 24px 24px;

(I tried it on http://less2css.org )

@ttfkam
Copy link
Contributor

@ttfkam ttfkam commented Sep 6, 2013

@Rubious did you turn on strict math in the options menu? It is off by default and so gives the old behavior.

@FellowshipAgency
Copy link

@FellowshipAgency FellowshipAgency commented Sep 6, 2013

OK that works, however I'm using LiveReload and this option does not seem to be switched on there.

@lukeapage
Copy link
Member

@lukeapage lukeapage commented Sep 6, 2013

Sorry thats the only way to fix it.. either turn on strict maths so all maths is only done if it is parenthesis OR

border-radius:0 0 100% 100% ~"/" 0 0 24px 24px;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants