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

Drawing path with stroke attribute set to "none" #47

Open
morgangiraud opened this issue Mar 25, 2015 · 9 comments
Open

Drawing path with stroke attribute set to "none" #47

morgangiraud opened this issue Mar 25, 2015 · 9 comments

Comments

@morgangiraud
Copy link

Hello Maxwellito !

First thanks for your lib, it works well and that's nice !
I was playing around with some SVG when i found a path element in some random SVG used only to fill areas: the stroke attribute was set to "none".
This is breaking the animation when the oneByOne option is used because you have to wait for the invisible path to finish before going on.

I made a very simple modification to remove paths that have stroke set to none. Do you think it would be interesting to add this ? ( I can make a P.R. if you want)

Vivus.prototype.mapping = function () {
  var i, paths, path, pAttrs, pathObj, totalLength, lengthMeter, timePoint;
  timePoint = totalLength = lengthMeter = 0;
  paths = this.el.querySelectorAll('path');

 // START: Added part
  var cleanPaths = [];
  for (i = 0; i < paths.length; i++) {
    path = paths[i];
    if(path.getAttribute("stroke") === "none"){
      continue;
    }
    cleanPaths.push(path);
  }
  paths = cleanPaths;
// END: Added part

  for (i = 0; i < paths.length; i++) {
    path = paths[i];
    pathObj = {
      el: path,
      length: Math.ceil(path.getTotalLength())
    };
  ...
};
@jolic
Copy link

jolic commented Mar 25, 2015

Maybe better:

Vivus.prototype.mapping = function () {
  var i, paths, path, pAttrs, pathObj, totalLength, lengthMeter, timePoint;
  timePoint = totalLength = lengthMeter = 0;
  paths = this.el.querySelectorAll('path');

  for (i = 0; i < paths.length; i++) {
// added
    if ( "none" === paths[i].getAttribute("stroke") ) continue;
// /added
    path = paths[i];
    pathObj = {
      el: path,
      length: Math.ceil(path.getTotalLength())
    };
    // Test if the path length is correct
....

@maxwellito
Copy link
Owner

Hi,

Thanks for these kinds words :)
I have been thinking to a way to ignore some paths but there would be so much rules that I forgot the idea. Because stroke='none' could be the marker to ignore a path, but in this case I should also think about path which aren't visible (display: none, opacity: 0, parent elment not visible too..).
So I was thinking to use a simple attribute to set in the path element to ignore. Something like data-vivus-ignore. I'm open to suggestions for this one :)

I put your ticket as improvement. I can't promess to implement it soon, I have proper bugs to fix first :-S

@morgangiraud
Copy link
Author

If we can find a good way to implement it, i don't mind making a PR with the implementation.

What about:

Vivus.prototype.mapping = function () {
  var i, paths, path, pAttrs, pathObj, totalLength, lengthMeter, timePoint;
  timePoint = totalLength = lengthMeter = 0;
  paths = this.el.querySelectorAll('path');

  for (i = 0; i < paths.length; i++) {
// added
    if ( this.isInvisible(paths[i])) continue;
// /added
    path = paths[i];
    pathObj = {
      el: path,
      length: Math.ceil(path.getTotalLength())
    };

With:

Vivus.prototype.isInvisible = function () {
// if attr data-vivus-ignore exists and is set to true 
//     return true
// If attr data-vivus-ignore exists and is set to false  <- gives more control to the user
//     return false
// if attr stroke is set to none 
//     return true
// if inline style containts display:none || visibility:hidden || || opacity:0 
//     return true
// return false
}

It's clearly not perfect but it's already an on the current state, don't you think ?
If we want to handle parent visibility i guess we will need to have a recursive mapping function on the svg childNodes tree.

@jolic
Copy link

jolic commented Mar 26, 2015

That's definitly better.

@jolic
Copy link

jolic commented Mar 26, 2015

about the children stuff we could check inside the isInvisible method if the current element is not visible and if so, mark all children with data-vivus-ignore="true", example:

Vivus.prototype.mapping = function () {
  var i, paths, path, pAttrs, pathObj, totalLength, lengthMeter, timePoint;
  timePoint = totalLength = lengthMeter = 0;
  paths = this.el.querySelectorAll('path');

  for (i = 0; i < paths.length; i++) {
// added
    if ( this.isInvisible(paths[i])) {
      var childNodes = paths[i].childNodes;
      if ( childNodes.length > 0 ) { // mark all children of current element as to be ignored
          for ( var j = 0; j < childNodes.length; j++ ){
               j.setAttribute("data-vivus-ignore", true)
          }
      }
      continue;
    }
// /added
    path = paths[i];
    pathObj = {
      el: path,
      length: Math.ceil(path.getTotalLength())
    };

@jolic
Copy link

jolic commented Mar 27, 2015

ups, typo mismatch.... should be:

...
          childNodes[j].setAttribute("data-vivus-ignore", true);
...

@maxwellito
Copy link
Owner

To be honest I'm really confused about what to do in this case. There's no reliable way to know if a shape is visible or not (parent visible or not, and class styling). But I think we should focus on the most common usecases. Usually there no class styling for an SVG, they usually come freshly out from Illustrator or Inkscape. So I would try your code @morgangiraud (which simple and clean, brilliant) but with a little change:

For example, a SVG exported from Illustrator might look like this:

<?xml version="1.0" encoding="utf-8"?>
<!-- Generator: Adobe Illustrator 15.0.2, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg version="1.1" id="Calque_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
   width="40px" height="40px" viewBox="0 0 40 40" enable-background="new 0 0 40 40" xml:space="preserve">
  <circle fill="none" stroke="#000000" stroke-miterlimit="10" cx="16.26392" cy="15.48047" r="11.72144"/>
  <g display="none">
    <circle display="inline" fill="none" stroke="#000000" stroke-miterlimit="10" cx="24.3811" cy="22.89746" r="11.09155"/>
    <circle display="inline" fill="none" stroke="#000000" stroke-miterlimit="10" cx="24.10132" cy="15.33984" r="8.15259"/>
  </g>
</svg>

So usually we choose to not display a complete group. With this recursive trick, we can check the parents. It's not optimal in term of performances but it's only done when the SVG is ready and not between each frame.

Vivus.prototype.isInvisible = function (el) {
  if attr data-vivus-ignore exists and is set to true 
    return true
  if attr stroke is set to none 
    return true
  if inline style containts display:none || visibility:hidden || opacity:0 
    return true
  if (this.el !== el)
    return this.isInvisible(el.parentNode);
  else
    return false;
}

@maxwellito
Copy link
Owner

I think I found something to help :

function isHidden(el) {
    return (el.offsetParent === null)
}

http://stackoverflow.com/questions/19669786/check-if-element-is-visible-in-dom

I need some more tests but it should help to check if an element is visible. It works if the parent element is not visible. But you perfectly fit in @morgangiraud comment.

I'll push some updates here tonight, based on @morgangiraud PR #48

@maxwellito
Copy link
Owner

For now the method I have look like this: (fork from #48)

Vivus.prototype.isInvisible = function (el) {
  var ignoreAttr = el.getAttribute('data-vivus-ignore');
  if (ignoreAttr !== null) {
    return ignoreAttr !== 'false';
  }

  return (this.ignoreInvisible && el.offsetParent === null);
};

this.ignoreInvisible would be a new option (boolean, default true) to let developer choose if they want to enable the option to ignore invisible paths.

But where are the checks about opacity and stroke?

Well, not that I don't want them, they are really relevant but won't work in all cases. If the style is applied via a CSS rule we won't be able to detect it. We can only check the attribute or inline style on the element. It's a shame. But if we have a good way to check if the element is visible, which is a big improvement.

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

No branches or pull requests

3 participants