search

yushiro / svg-edit

Automatically exported from code.google.com/p/svg-edit
MIT License
0 stars 0 forks source link

Crash on resize in Chrome Windows because of vectorEffect:non-scaling-stroke #904

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. Load svg-edit
2. draw rectangle
3. Drag a handle to resize the rectangle.
4. Chrome crashes the current browser tab.

What is the expected output? What do you see instead?

The rectangle should change size as the mouse moves.

In what browser did you experience this problem? (ALL, Firefox, Opera, etc)

Chrome 17.0.963.56 m
Windows 7 Professional SP1 64bit

NOTE: It's working in OS X 10.7.3 with Chrome 17.0.963.56

In what version of SVG-edit does the problem occur? (Latest trunk, 2.5.1, etc)

Latest trunk. I did get a fresh copy and reproduced the problem and workaround.

Please provide any additional information below.

The problem is with svgcanvas.js, line 2519
mouse_target.style.vectorEffect = 'non-scaling-stroke';

Patch file with workaround attached. I'm guessing we need to update 
svgedit.browser.supportsNonScalingStroke() to return false for Chrome (specific 
versions?) in browser.js line 135.
Opinions?

Original issue reported on code.google.com by FlintOBr...@gmail.com on 16 Feb 2012 at 5:50

Attachments:

GoogleCodeExporter commented 9 years ago 
Opened chromium bug:
http://code.google.com/p/chromium/issues/detail?id=114625

Original comment by FlintOBr...@gmail.com on 16 Feb 2012 at 6:27

GoogleCodeExporter commented 9 years ago 
Hi Flint - I much prefer your second suggestion (updating the return value of 
supportsNonScalingStroke()) than your workaround patch.  Do you think you could 
tackle that?

Original comment by codedr...@gmail.com on 16 Feb 2012 at 8:31

GoogleCodeExporter commented 9 years ago 
I'll work on detecting Chrome windows and returning false for 
svgedit.browser.supportsNonScalingStroke(). 

I didn't see any code detecting the OS, so I'm hesitant to start down that 
path. I know how to do it, but I'm assuming there is some collective wisdom in 
not doing it. I'm looking for recommendations on how restrictive I should be. 
For example, I could detect Webkit and return false or Webkit && Windows. 
Remember, Webkit non-scaling-stroke works on OS X (with Chrome).

Original comment by FlintOBr...@gmail.com on 17 Feb 2012 at 5:21

GoogleCodeExporter commented 9 years ago 
"collective wisdom"?  Probably not.  I just don't think it's been required 
before.

Original comment by codedr...@gmail.com on 17 Feb 2012 at 9:38

GoogleCodeExporter commented 9 years ago 
While the crash can be consistently made, it can also be consistently avoided.

It seems if you try to scale it *after* you mouse-over the element, it works 
flawlessly. 

I'm out of ideas why, I've tried everything including adding the style from CSS 
tab of Chrome's element inspector. It'll still crash if I did not hover the 
element first.

So to make supportsNonScalingStroke() return false for this platform is a bit 
misleading, unless there is no workaround for this peculiar bug.

Original comment by asyazwan on 22 Feb 2012 at 11:19

GoogleCodeExporter commented 9 years ago 
Hi again,

on Windows 7 x64 and latest Chrome, I seem to be able to do this as workaround:

- If Chrome+Windows remove stroke
- Apply CSS
- If Chrome+Windows re-add stroke after some interval

For the last step, I use setTimeout(). It won't work without so I had to 
introduce delay. I started with 500ms but the flicker (stroke remove-add) was 
too apparent so I kept reducing it... then I found out it works even at 1ms. 
Weird.

Is this solution good enough?

Here's the diff to illustrate above steps.

Original comment by asyazwan on 22 Feb 2012 at 5:02

Attachments:

GoogleCodeExporter commented 9 years ago 
asyazwan - your solution looks ok to me... Flint?

Two things for this patch:

  1) Please move the isChromeWindows() function into browser.js.  Possibly split them up into two functions (isChrome and isWindows)

  2) Please use the same indentation.  If you view the diff on googlecode, it's pretty obvious.  We made the (probably) bad choice of using tabs way back and so now we just need to be consistent.

Original comment by codedr...@gmail.com on 22 Feb 2012 at 5:35

GoogleCodeExporter commented 9 years ago 
Sorry for the format -- that was just to show my idea.

Here's a patch following your requests.

Original comment by asyazwan on 23 Feb 2012 at 2:19

Attachments:

GoogleCodeExporter commented 9 years ago 
Fixed with r2053, thanks asyazwan!  Flint, please let me know if this fixes the 
problem (if not, I'll reopen the bug).

Original comment by codedr...@gmail.com on 23 Feb 2012 at 3:12

GoogleCodeExporter commented 9 years ago 
Scrap that, I totally missed the loop below which does the same bug to all 
children, e.g. grouping.

Will need to apply the same fix and test all resize combinations I can think of.

Original comment by asyazwan on 23 Feb 2012 at 3:31

GoogleCodeExporter commented 9 years ago

Original comment by codedr...@gmail.com on 23 Feb 2012 at 3:55

GoogleCodeExporter commented 9 years ago 
Here's my latest attempt. Fixed grouping and nested groups resize.

Something interesting to note is that I did the remove-add after vectorEffect = 
'non-scaling-stroke' line... it works. This bug continues to baffle me.

Original comment by asyazwan on 23 Feb 2012 at 3:59

Attachments:

GoogleCodeExporter commented 9 years ago 
Can you add some TODOs around the code so that when this bug is gone we can 
easily undo the workarounds?

Original comment by codedr...@gmail.com on 23 Feb 2012 at 4:04

GoogleCodeExporter commented 9 years ago 
Added // TODO: Remove this workaround (all isChromeWindows blocks) once vendor 
fixes the issue

Original comment by asyazwan on 23 Feb 2012 at 4:10

Attachments:

GoogleCodeExporter commented 9 years ago 
r2054 committed, thanks again!  Please tell me when I can close this bug

Original comment by codedr...@gmail.com on 23 Feb 2012 at 4:15

GoogleCodeExporter commented 9 years ago 
Just for reference, here is the bug reported by Flint in Chrome project: 
https://code.google.com/p/chromium/issues/detail?id=114625

Original comment by asyazwan on 23 Feb 2012 at 4:16

GoogleCodeExporter commented 9 years ago 
asyazwan - has this bug been fixed now?

Original comment by codedr...@gmail.com on 27 Feb 2012 at 11:20

GoogleCodeExporter commented 9 years ago 
As far as  I can tell -- yes. Was waiting for some feedback from Flint.

Marking it fixed...

Original comment by asyazwan on 27 Feb 2012 at 11:56

GoogleCodeExporter commented 9 years ago 
The latest trunk works. No crash and the stroke stays the correct size during a 
resize drag.
Chrome 17.0.963.56 m on Windows 7.
Thanks!

Original comment by FlintOBr...@gmail.com on 29 Feb 2012 at 3:16

GoogleCodeExporter commented 9 years ago 
This bug appears in Chrome Linux 18.0.1025.39 beta 64bit. This may be a Windows 
and Linux issue.

Original comment by mcdonald...@gmail.com on 1 Mar 2012 at 1:06

GoogleCodeExporter commented 9 years ago 
Checked Chromium on my Arch Linux and yeah it crashed alright.

Patch to remove windows detection attached.

Thanks.

Original comment by asyazwan on 1 Mar 2012 at 2:07

Attachments:

GoogleCodeExporter commented 9 years ago 
Can I get some status update on this?

Original comment by asyazwan on 4 Mar 2012 at 7:40

GoogleCodeExporter commented 9 years ago 
Marking this fixed as no further feedback was provided, and it is fixed for me, 
tested with Chromium 17, Arch Linux x64.

Original comment by asyazwan on 20 Mar 2012 at 5:41

GoogleCodeExporter commented 9 years ago 
r2068 incorporate above patch for Linux fix.

Original comment by asyazwan on 26 Mar 2012 at 7:55

GoogleCodeExporter commented 9 years ago 
Can anyone confirm that vectorEffect works for upcoming Chrome 19 (if you have 
dev installed) without the workaround?

https://code.google.com/p/chromium/issues/detail?id=114625

Original comment by asyazwan on 30 Mar 2012 at 2:07

GoogleCodeExporter commented 9 years ago 
This no longer crashes recent versions of Chrome, but it does make the stroke 
disappear while you are scaling it. Unfortunately the bug seems to have 
propagated to Safari 6 and Mobile Safari on iOS6. Here is a patch to address 
all versions of Webkit.

Original comment by duopi...@gmail.com on 13 Nov 2012 at 10:33

Attachments:

GoogleCodeExporter commented 9 years ago 
Disregard the patch, I've committed the code since it's pretty straightforward.

Original comment by duopi...@gmail.com on 13 Nov 2012 at 10:38

GoogleCodeExporter commented 9 years ago

Original comment by bret...@gmail.com on 7 Apr 2014 at 3:38

GoogleCodeExporter commented 9 years ago

Original comment by bret...@gmail.com on 7 Apr 2014 at 3:45