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

shadow doesn't render the same in chrome vs safari #4402

Closed
blobinabottle opened this issue Oct 23, 2017 · 6 comments · Fixed by #4413
Closed

shadow doesn't render the same in chrome vs safari #4402

blobinabottle opened this issue Oct 23, 2017 · 6 comments · Fixed by #4413

Comments

@blobinabottle
Copy link
Contributor

Version

2.0 beta

Test Case

http://jsfiddle.net/7gvJG/207/

Steps to reproduce

Check the shadow in safari vs chrome. Chrome is less blurry than Safari. From our test it seems Safari is more coherent with the same shadow props in html/CSS.

Safari Behavior

screen shot 2017-10-23 at 13 37 59

Chrome Behavior

screen shot 2017-10-23 at 13 38 06

@asturur
Copy link
Member

asturur commented Oct 24, 2017

Is a browser issue i found out some months ago, for which i can do little for now.
Each browser do blurring in its own way, specs do not dictate anything.

The only solution is a custom shadow engine, we can evaluate it as soon as we can extend filters to all objects.

https://jsfiddle.net/asturur/ot2s2Lja/3/

open this fiddle in chrome, firefox, safari and IE and check the different results of shadow blur across browsers.

CHROME
image

SAFARI
image

FIREFOX
image

@blobinabottle
Copy link
Contributor Author

Thanks. We'll check if we can find the magic numbers to make all browser match. Could be useful I guess.

@asturur
Copy link
Member

asturur commented Oct 24, 2017

Internet explorer and edge do different also.
Not sure is worth trying to fix some multiplier so that are all happy.
Fabric zooming often brings you in situations where you have high numbers

updated the fiddle to better show what happens
https://jsfiddle.net/asturur/ot2s2Lja/5/

chrome
image

safari
image

firefox
image

As you see firefox has a wider blur, but is immediately capped.

@asturur
Copy link
Member

asturur commented Oct 24, 2017

each pixel of that image represent a shadowblur value.

@melvynhills
Copy link
Contributor

melvynhills commented Oct 24, 2017

This is my current solution, which uses user-agent to determine the blur value to use.
Dirty but does the job.

fabric.Object.prototype._setShadow = function(ctx) {
  if (!this.shadow) {
    return;
  }

  var multX = (this.canvas && this.canvas.viewportTransform[0]) || 1,
      multY = (this.canvas && this.canvas.viewportTransform[3]) || 1,
      scaling = this.getObjectScaling();
  if (this.canvas && this.canvas._isRetinaScaling()) {
    multX *= fabric.devicePixelRatio;
    multY *= fabric.devicePixelRatio;
  }
 

  //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
  // CUSTOM CODE START
  //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

  var isEdge = /Edge/.test(navigator.userAgent);
  var isChrome = /Chrome/.test(navigator.userAgent) && /Google/.test(navigator.vendor);
  var isSafari = /Safari/.test(navigator.userAgent) && /Apple/.test(navigator.vendor);
  var isFirefox = /Firefox/.test(navigator.userAgent);
  var isIE = /MSIE|Trident/.test(navigator.userAgent);

  var shadowBlurMultiplier = 1;
  if (isEdge) {
    shadowBlurMultiplier = 1.75;
  } else if (isChrome) {
    shadowBlurMultiplier = 1.5;
  } else if (isSafari) {
    shadowBlurMultiplier = 0.95;
  } else if (isFirefox) {
    shadowBlurMultiplier = 0.9;
  }
  ctx.shadowBlur = this.shadow.blur * shadowBlurMultiplier * (multX + multY) * (scaling.scaleX + scaling.scaleY) / 4;

  //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
  // CUSTOM CODE END
  //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////


  ctx.shadowColor = this.shadow.color;
  ctx.shadowOffsetX = this.shadow.offsetX * multX * scaling.scaleX;
  ctx.shadowOffsetY = this.shadow.offsetY * multY * scaling.scaleY;
},

Not sure yet how to implement it cleanly in the library…Maybe something like a variable on fabric object such as browserShadowBlurConstant, by default set to 1?
If you do think it's a good idea, I can submit a PR that exposes that variable so everyone can use their own browser detection logic if they want it.

/HEADER.js

fabric.browserShadowBlurConstant = 1;

/src/shapes/object.class.js

_setShadow: function(ctx) {
  ...
  ctx.shadowBlur = this.shadow.blur * fabric.browserShadowBlurConstant * (multX + multY) * (scaling.scaleX + scaling.scaleY) / 4;
  ...
},

custom code

var isEdge = /Edge/.test(navigator.userAgent);
var isChrome = /Chrome/.test(navigator.userAgent) && /Google/.test(navigator.vendor);
var isSafari = /Safari/.test(navigator.userAgent) && /Apple/.test(navigator.vendor);
var isFirefox = /Firefox/.test(navigator.userAgent);
var isIE = /MSIE|Trident/.test(navigator.userAgent);

if (isEdge) {
  fabric.browserShadowBlurConstant = 1.75;
} else if (isChrome) {
  fabric.browserShadowBlurConstant = 1.5;
} else if (isSafari) {
  fabric.browserShadowBlurConstant = 0.95;
} else if (isFirefox) {
  fabric.browserShadowBlurConstant = 0.9;
}

@asturur
Copy link
Member

asturur commented Oct 24, 2017

this does not solve the cap problem that i think is unsolvable.
I would avoid browser sniffing if possible in the library, but this is a solution that works for the apps that uses fabric.

Just exposing browserShadowBlurConstant, default to 1 in the HEADER.js i think is an ok change. Helps everyone and everyone can support also future browsers if any ( or changes in canvases that may happen if they switch the drawing library cairo or other )

I would also add the costants you discovers in a comment right over the variable declaration.

asturur pushed a commit that referenced this issue Oct 25, 2017
* Expose browserShadowBlurConstant to adjust shadowBlur value (#4402)
* Add test for fabric.browserShadowBlurConstant
* added test description in case of failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants