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

Let users specify the position of the gradient's center #13

Closed
wants to merge 6 commits into from

Conversation

kumarharsh
Copy link
Contributor

This PR lets users specify the position of the gradient's center.

Currently, it supports these units:
px | %
and the values supported by background-image/transform-origin properties, namely top|bottom|left|right|center.

Also, care is taken so that these two result in the same position:
(at top left, ...)
and
(at left top, ...)

There is a limitation of the current architecture, which makes the position values to not be relative to the element on which the gradient is applied, but to an arbitary canvas element. This is because the source element is not accessible while the gradient is being painted. This is also the reason that em is not supportable right now.

@LeaVerou
Copy link
Owner

Hi there,

Thanks for the contribution! Can I test this online somewhere? Judging from the code, it doesn't seem to draw the gradient correctly (the center point argument is not just about moving it), but I can't tell for sure without testing it.

@kumarharsh
Copy link
Contributor Author

Hi, here's a test page:
http://kumarharsh.github.io/conic-gradient/center.html

@LeaVerou
Copy link
Owner

Hi, I was referring to the main page of the polyfill, but with your version, since that has editable examples and it would be easy for me to try multiple cases.
When I try to visit http://kumarharsh.github.io/conic-gradient/index.html it's broken due to the broken references to prefixfree & incrementable…

@kumarharsh
Copy link
Contributor Author

Oh, alright. I'll fix that...
update: It's done. check it out now.

@kumarharsh
Copy link
Contributor Author

@LeaVerou I've added comments in the code for more clarity into the process of calculating the centers. Let me know if there's something more that can be done :)

@LeaVerou
Copy link
Owner

First try, failed.

background: conic-gradient(at 20% 20%, #f06 40%, gold 0);

@LeaVerou
Copy link
Owner

Also, you seem to just be moving the gradient based on the center. The center point is not just about moving the gradient, otherwise I would have implemented it too. It changes how the gradient is drawn.

@kumarharsh
Copy link
Contributor Author

@LeaVerou Hi. I'm not sure I understand the reason for center point correctly. It is used to shift the center, right? Like so:
center-point moved

I realize that there is still work to be done on extending the gradient all the way to the edges, like so:
center-point moved and gradient extended

I hope I'm on the right track, but please correct me if I'm not.

@LeaVerou
Copy link
Owner

Actually, given that everything resolves to degrees currently, you’re right. So yeah, mainly we need the radius to be fixed, then I will review for coding style etc.

Kumar Harsh added 2 commits August 27, 2015 02:27
* If the vertical component came alone, such as `at top`, then the position
  was wrongly calculated as (left, center). This is fixed.
* the radius of the canvas used was calculated as √2*(side/2). This was
  causing the painted gradient to not stretch till the end of the element
  when the gradient was positioned sufficiently off-center. The best way
  to ensure that the gradient covers the full element is having the
  radius to be √2*(longer-side) - this would ensure even gradients at the
  top-left corner to cover the bottom-right corner.
@LeaVerou
Copy link
Owner

Thanks for the updates, I took a closer look today.

So, you’re doing a ton of work to resolve the gradient center, and despite that, you only support very few values. This is typical when people re-implement things the browser already knows how to do. The gradient centerpoint is resolved identically to background position and the browser knows how to resolve it.

Also, the dimensions of the canvas element are NOT the dimensions of the background. That's the whole reason why its wrapped in the two svg elements. In addition, the problem here is that PrefixFree just replaces CSS text without any knowledge about the element it’s applied on, so percentages are practically impossible to implement, unless we do something clever in the SVG (which knows its viewport, and 100vw/vh correspond to the dimensions of your background). Lemme think about this for a bit.

@kumarharsh
Copy link
Contributor Author

yea, I am doing a "ton" of work to resolve the center 😄 ... Seemed pointless to me, as the browsers already did that... I did search for what algorithms the browsers used, but digging into webkit/gecko's sources was just too much hassle. Also, I didn't think of the idea of using the browser's implementation itself! I'm not clear how that'd work, but that'd be so so much better. Is there a place I can learn how to do this?

@LeaVerou
Copy link
Owner

Well, not exactly, as the browser doesn’t really expose it. However, getComputedStyle() on background-position does do most of the processing, giving you only percentages and pixels. So you could try that on the element (as a test), get the result, then restore the original background-position.

The above is generally speaking. There is a pretty difficult problem in this particular case though: You don’t actually have access to the element. To that, I have no solution yet (and your existing one doesn’t work). I think doing something smart with the SVG is probably the way to go, since that one is aware of its viewport and can use percentages.

@kumarharsh
Copy link
Contributor Author

Hmm, I agree... only if we had the access to the element. That'd solve some issues :)

@kumarharsh
Copy link
Contributor Author

Hi. I was able to get the center position properly working. Checkout this page:
http://kumarharsh.github.io/conic-gradient/center.html

I had to make some changes to the way the css was being read, the major part being building an AST for css in the <style> tags using the reworkcss/css library and then taking the element from there.

A minor side-effect of this is that I'm not able use the "Incrementable" feature properly, as it's not playing well with the prefixFree library apparently. When I'm trying to use the incrementable library, and the css is in the style attribute like so: <div style="...">, then the StyleFix.register callback has raw and element (2nd and 3rd parameters) as undefined.

So, I've not made an 'interactive' demo, but I have compiled a bunch of gradients on a different page: http://kumarharsh.github.io/conic-gradient/center.html

@kumarharsh
Copy link
Contributor Author

Hi @LeaVerou... did you get a chance to look at the updated code. I've still not been able to get the incrementable bit working with the new demos... I'd need your help in that

@LeaVerou
Copy link
Owner

LeaVerou commented Sep 4, 2015

Yes, it seems to work, but basically you rewrote the entire script? Also, these examples aren't editable…
Building an AST is not an option, this needs to work with -prefix-free as it is and I'm not going to process their entire CSS code into an AST just so they can use center points in their conical gradients.

@kumarharsh
Copy link
Contributor Author

Hi. No, I didn't rewrite the entire script. Most of the modifications lie only in the functions I'd introduced, like the _.GradientCenter and _.GradientCenter.stringToPosition.

The only change in the existing code was shoehorning the ast construction to find the target element. The good thing was that StyleFix already had the element for inlined styles, so that was a simple task. The challenge was that external css/those in the <style> tag were just a string of text... So, taking the element out was nearly impossible, if not impossible.

Now, there is support for all the css length units, which was achieved by using the window.getComputedStyle as you'd suggested on a dummy element.

I understand constructing the AST is costly, but (currently) I see it being more of a necessary evil 😉

@kumarharsh
Copy link
Contributor Author

@LeaVerou any pointers to a different approach?

@eonj
Copy link
Contributor

eonj commented Dec 22, 2016

@LeaVerou, I agree with @kumarharsh. This patch is not an entire rewrite of the script, and good to be merged.

Building AST is an option, I think. It can be another solutions, such as using polyfills for <position> (defined in W3C TR css3-values). For reference, as you all know, <conic-gradient> is defined as (per W3C TR css4-images):

<conic-gradient> = conic-gradient(
  [ at <position> , ]?
  <angular-color-stop> [ , <angular-color-stop> ]+
)

So, is <position> something that goes with -prefix-free? I am confused. How are the other polyfills resolving position value? Is there a dependable library?

Another changes are with formatting spaces and tabs. This is why i wrote PR #18. This can reduce breaking change of this PR.

In fact, I was writing a change for this conic-gradient polyfill last year, and now I'm writing a patch for #14, #17, and wrong color/angle issues I found. My patches are going to be likely breaking, but most of them are going to be reconstructing the whole code structure abstraction. Then the feature fix can be minimal. Also, the code cleaning may open possibility of applying ES6 features such as strict mode, so I think AST building cost can be dealt if building own AST comes to an unavoidable choice.

But I'm afraid I should write no more fix if I would rejected. Can "rewrote the entire" be a good reason rejecting this PR?

In addition, I think this -- specifying gradient center (in all units!) -- is the most urgent feature issue for this project.

@kumarharsh
Copy link
Contributor Author

@LeaVerou I can put in some more work if I can get some pointers to do things differently. Some other approact the get the gradient centers depending on the element. I do agree that building the AST is not a viable solution if you want to use this in a production-ready code, but with smaller projects it shouldn't be too expesive. Since the conic-gradient spec is anyways not stabilized yet, I don't think anybody should be using this in any serious project anyways. Therefore I'm of the opinion that it's better to bring as much functionality as possible to conic-gradient so that people can experiment more.

This brings me to a larger point - I see that the spec itself has seen very slow growth since you'd proposed it. I'd like to see conic-gradients in our browsers, and if there's anything I can do to see that day sooner, I'm up for it. If you do find some free time, I'd like to discuss it with you. Possibly over gitter or discord?

@LeaVerou LeaVerou closed this Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants