Bug 157567

Summary:

outermost SVG currentScale should only scale SVG Element

Product:

WebKit

Reporter:

Mathias Menzel <self>

Component:

SVG

Assignee:

Said Abou-Hallawa <sabouhallawa>

Status:

REOPENED

Severity:

Normal

CC:

bfulgham, dino, sabouhallawa, simon.fraser, webkit-bug-importer, zimmermann

Priority:

Keywords:

InRadar

Version:

Safari 9

Hardware:

Mac

OS:

OS X 10.11

See Also:

https://bugs.webkit.org/show_bug.cgi?id=181366

Attachments:

Description	Flags
opera scale	none
Embedded-SVG-CurrentScale	none
Standalone-SVG-CurrentScale	none
Embedded-SVG-ViewBox	none
Patch	simon.fraser: review+

Mathias Menzel

Reported 2016-05-11 07:45:42 PDT

Created attachment 278625 [details] opera scale When setting the currentScale attribute on the outermost SVG inline Element in a web page, the whole web page is scaled, just as if cmd+ is pressed. According to the SVG specs http://www.w3.org/TR/SVG2/struct.html#InterfaceSVGSVGElement, the following (last) step should be performed when setting currentScale (on the outermost SVG Element): "Set the document's magnification and panning transform to [scale 0 0 scale e f]." This clearly references the SVG document and not the web page document. Actually Opera presumably does it right (Version 12.16 , Build 1860), allowing for Web pages with MDI style SVG Canvases. btw: the currentTranslate (SVGPoint) is implemented correctly, this just translates the SVG document and keeps the html elements on the webpage untouched. Example: With this markup: ---- <html><head> <meta charset="UTF-8"> <title>SVG</title> </head> <body> <h1>SVGCanvas</h1> <div style="width: 600px; height: 400px; overflow: auto; border: 1px solid red;"> <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" baseProfile="full" width="600" height="400" viewBox="0 0 600 400" id="svg"> <rect id="r1" x="100" y="100" width="100" height="100" fill="green"></rect> </svg> </div> </body> </html> ---- go to console and do: > document.getElementById('svg').currentScale = 2 BTW: The link in the bugzilla user's guide (https://bugs.webkit.org -> Bugzilla's Users Guide -> 5.6.1 Reporting a new Bug (https://landfill.bugzilla.org/bugzilla-3.2-branch/page.cgi?id=bug-writing.html)) is broken.

Attachments
opera scale (85.74 KB, image/png) 2016-05-11 07:45 PDT, Mathias Menzel	no flags	Details
Embedded-SVG-CurrentScale (470 bytes, text/html) 2016-05-11 11:50 PDT, Said Abou-Hallawa	no flags	Details
Standalone-SVG-CurrentScale (213 bytes, image/svg+xml) 2016-05-11 11:53 PDT, Said Abou-Hallawa	no flags	Details
Embedded-SVG-ViewBox (386 bytes, text/html) 2016-05-11 11:54 PDT, Said Abou-Hallawa	no flags	Details
Patch (8.19 KB, patch) 2016-05-13 12:23 PDT, Said Abou-Hallawa	simon.fraser: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Said Abou-Hallawa

Comment 1 2016-05-11 11:50:35 PDT

Created attachment 278644 [details] Embedded-SVG-CurrentScale

Said Abou-Hallawa

Comment 2 2016-05-11 11:52:54 PDT

I do not think we need to fix this because the behavior of currentScale is undefined when we're dealing with non-standalone SVG documents. Since there is no specs for currentScale with embedded SVG, browsers handle it differently: 1. Safari and Chrome: the scaling is handled by the host renderer. 2. FireFox: the scaling is compelety ignored. 3. Opera: the scale is handled as applying viewBox scaling. I attached two test cases for changing currentScale with standalone and embedded SVG. The embedded case behaves differently but the standalone behaves the same in all browsers. There is nicer and easier way to replace currentScale with embedded SVG. Using viewBox on the outer SVG element will do exactly the same as setting the currentScale. For example, the following SVG will be scaled 200%. <svg width="600" height="400" viewBox="0 0 300 200"> I attached also another test case which uses viewBox scaling and which behaves in WebKit exactly as currentScale behaves in Opera.

Said Abou-Hallawa

Comment 3 2016-05-11 11:53:43 PDT

Created attachment 278645 [details] Standalone-SVG-CurrentScale

Said Abou-Hallawa

Comment 4 2016-05-11 11:54:41 PDT

Created attachment 278646 [details] Embedded-SVG-ViewBox

Said Abou-Hallawa

Comment 5 2016-05-11 12:08:56 PDT

Won't fix this one but I found another issue which I logged here https://bugs.webkit.org/show_bug.cgi?id=157578.

Mathias Menzel

Comment 6 2016-05-12 02:50:33 PDT

Though I agree, that viewBox does sort of scaling alright, how come currentScale is undefined, when dealing with non-standalone SVG (where does it say so)? And even if currentScale is undefined, why then is there an implementation at all, that is somewhat not helpful. Consider two SVG elements on a web page. Why would I want the whole web page to scale if I set the currentScale of one of the SVGs? And why is currentTranslate, that has the same aspect, is properly implemented? Especially in context with overflow/panning/scrollbars (overflow: scroll http://www.w3.org/TR/SVG11/masking.html#OverflowAndClipProperties - that do not display as of now) currentScale being bound to the very SVG Element would more then help. Thats what is more clearly specified in SVG2 anyway: http://www.w3.org/TR/SVG2/render.html#OverflowAndClipProperties Then we would have SVG documents embedded in XHTML documents, that would be scalable and panable/scrollable, so there really would be a viewBox onto a document (just like UML Tools and others do, that show part of a big layout in MDI applications). If I remember right, that was already implemented in Browsers like 15 years back (pan and zoom of embedded SVG).

Mathias Menzel

Comment 7 2016-05-13 07:24:58 PDT

Is reopen necessary to keep discussion going?

Simon Fraser (smfr)

Comment 8 2016-05-13 08:04:03 PDT

I think it's worth investigating why currentScale has the unexpected behavior, but currentTranslate does not.

Said Abou-Hallawa

Comment 9 2016-05-13 10:39:19 PDT

From looking at the code the difference between currentScale and currentTranslate is obvious. Setting these properties are done by the following code: void SVGSVGElement::setCurrentScale(float scale) { if (Frame* frame = frameForCurrentScale()) frame->setPageZoomFactor(scale); } void SVGSVGElement::setCurrentTranslate(const FloatPoint& translation) { if (m_currentTranslate == translation) return; m_currentTranslate = translation; updateCurrentTranslate(); } SVGSVGElement::setCurrentScale() changes the Frame zoomFactor while SVGSVGElement::setCurrentTranslate(() sets a local variable which is returned by currentTranslate() and then is used by RenderSVGRoot::buildLocalToBorderBoxTransform(). This later function builds the transform from SVG coordinates to CSS coordinate. void RenderSVGRoot::buildLocalToBorderBoxTransform() { float scale = style().effectiveZoom(); SVGPoint translate = svgSVGElement().currentTranslate(); LayoutSize borderAndPadding(borderLeft() + paddingLeft(), borderTop() + paddingTop()); m_localToBorderBoxTransform = svgSVGElement().viewBoxToViewTransform(contentWidth() / scale, contentHeight() / scale); if (borderAndPadding.isEmpty() && scale == 1 && translate == SVGPoint::zero()) return; m_localToBorderBoxTransform = AffineTransform(scale, 0, 0, scale, borderAndPadding.width() + translate.x(), borderAndPadding.height() + translate.y()) * m_localToBorderBoxTransform; } In this function, currentScale() is not called since the Frame itself has its zoomFactor changed with it.

Said Abou-Hallawa

Comment 10 2016-05-13 10:55:54 PDT

Blaming the code of SVGSVGElement::currentScale() and SVGSVGElement::currentTranslate() gives us the following bug https://bugs.webkit.org/show_bug.cgi?id=11272. Looking at the patches attached to this bug shows that, Rob Buis started by having currentScale() and currentTranslate() behave the same. But Maciej Stachowiak suggested that we treat the currentScale() as we treat the page zoom factor. Then Rob Buis changed the fix accordingly. He also added the following comment: "This version uses zoomFactor from the Frame. Note that it behaves a bit differently, ie. the root svg increases/decreases along the zoom, also when embedded. I noticed this behaviour when testing with Opera, and it seems intuitive." I am not sure I understand what he meant be the intuitive behavior in Opera since I do not have Opera 2007 version and currently the behavior in Safari is not intuitive at all. Open Embedded-SVG-CurrentScale or Standalone-SVG-CurrentScale and change the zooming factor of the page. Zoom-in scales the page down first before scaling it up.

Said Abou-Hallawa

Comment 11 2016-05-13 11:45:37 PDT

From looking at the w3c discussion about currentScale and currentTranslate, I think they were added to overcome the restriction that the SVG root does not have a transform attribute. From the specs: DOM attributes currentScale and currentTranslate are equivalent to the 2x3 matrix [a b c d e f] = [currentScale 0 0 currentScale currentTranslate.x currentTranslate.y] So currentScale and currentTranslate were added to give some sort of transformation to the root SVG but to prevent operation like: rotate, skew and shear on the root SVG.

Mathias Menzel

Comment 12 2016-05-13 12:21:02 PDT

Please correct if I am wrong or if that does not make sense. I didn't want to start of with the discussion, if currentScale and currentTranslate do the same thing, but my intent was to say, that, even so they are specified to operate on the same object, they don't. I read the specs so, that it starts with the zoomAndPan attribute on an outermost SVG Element (http://www.w3.org/TR/SVG/single-page.html#interact-ZoomAndPanAttribute). If that is set to "disable", currentScale and currentTranslate are unchangeable (i.e. set to 1 and (0,0)). For user interactable environments (browsers), this attribute defaults to "magnify", so the user agent should enable panning and zooming on the SVG document fragment (acc. to specs - Mozilla explicitly states that they did not implement that). So zooming is changing the currentScale of the SVG object fragment (not the whole embedding page) and panning changes the currentTranslate implicitly (that's why currentTranslate is marked readonly in the specs, currentTranslate shouldn't be settable by the user directly, but is now). Lastly the overflow comes in and does what overflow does in CSS2+ (that's why there should be scrollbars I assume, just like on fixed width DIVs with "overflow:scroll", that's what has to be wrapped round SVG to be scrollable, but shouldn't be necessary). This all only applies to outermost SVG Elements. In regards to a zoomed-in page (cmd+) the zoom-in on SVG Objects should still be possible. So the page zoom enlargens the whole page alright, but if I mouseover an SVG document fragment I would expect only the SVG to zoom (e.g. by strg+mousscroll) and pan (e.g. by dragging the SVG), similar to DIVs with overflow content, when scrolled. On SVG document fragments, the scroll would set the currentScale, the pan would set the currentTranslate. And the whole page scale would not touch the descriptive SVG Object, but most likely would do the scale by other means (I checked other browsers, e.g. IE keeps currentScale to 1 when whole page is scaled), which probably should not be part of this thread. The said behaviour would allow multiple SVG document fragments with complex content in parallel on one page, each being pan- and zommable and still keeping image like SVGs not being zoomable by defining zoomAndPan="disable" explicitly on the embedded SVG. Makes sense?

Said Abou-Hallawa

Comment 13 2016-05-13 12:23:12 PDT

Created attachment 278861 [details] Patch

Said Abou-Hallawa

Comment 14 2016-05-13 12:28:44 PDT

Comment on attachment 278861 [details] Patch This patch fixes the issue and removes the dependency between the SVG currentScale and the page zooming factor. Although I still think that the specs is a little bit confusing regarding the transformation of the SVG root element. The SVG root element can only be scaled and translated. The problem is this can be done with three different ways: 1. SVG viewBox attribute 2. currentScale and currentTranslate which can only be applied to the root SVG and which can only applied through the DOM. 3. CSS styles.

Said Abou-Hallawa

Comment 15 2016-05-13 12:30:12 PDT

*** Bug 157578 has been marked as a duplicate of this bug. ***

Said Abou-Hallawa

Comment 16 2016-05-16 11:02:52 PDT

(In reply to comment #12) > Please correct if I am wrong or if that does not make sense. > > I didn't want to start of with the discussion, if currentScale and > currentTranslate do the same thing, but my intent was to say, that, even so > they are specified to operate on the same object, they don't. > > I read the specs so, that it starts with the zoomAndPan attribute on an > outermost SVG Element > (http://www.w3.org/TR/SVG/single-page.html#interact-ZoomAndPanAttribute). If > that is set to "disable", currentScale and currentTranslate are unchangeable > (i.e. set to 1 and (0,0)). > For user interactable environments (browsers), this attribute defaults to > "magnify", so the user agent should enable panning and zooming on the SVG > document fragment (acc. to specs - Mozilla explicitly states that they did > not implement that). > I can see why chose to do that. I think the specs is unclear regarding the case of embedded SVG. > So zooming is changing the currentScale of the SVG object fragment (not the > whole embedding page) and panning changes the currentTranslate implicitly > (that's why currentTranslate is marked readonly in the specs, > currentTranslate shouldn't be settable by the user directly, but is now). I think this is not true. currentTranslate is read-only because you can't change the SVGPoint attribute itself. But you can still change the x and the y members of this attribute. For example this code translates a standalone SVG by (100, 100): var root = document.documentElement; root.currentTranslate.x = 100; root.currentTranslate.y = 100; Also in Safari, if you open a standalone SVG and you hold Shift+Click and drag, the SVG will be translated by the amount of dragging. > Lastly the overflow comes in and does what overflow does in CSS2+ (that's > why there should be scrollbars I assume, just like on fixed width DIVs with > "overflow:scroll", that's what has to be wrapped round SVG to be scrollable, > but shouldn't be necessary). This all only applies to outermost SVG Elements. > > In regards to a zoomed-in page (cmd+) the zoom-in on SVG Objects should > still be possible. So the page zoom enlargens the whole page alright, but if > I mouseover an SVG document fragment I would expect only the SVG to zoom > (e.g. by strg+mousscroll) and pan (e.g. by dragging the SVG), similar to > DIVs with overflow content, when scrolled. > On SVG document fragments, the scroll would set the currentScale, the pan > would set the currentTranslate. And the whole page scale would not touch the > descriptive SVG Object, but most likely would do the scale by other means (I > checked other browsers, e.g. IE keeps currentScale to 1 when whole page is > scaled), which probably should not be part of this thread. > I could not follow this part regarding the zooming and panning of an embedded SVG. Is this part of any specs? Or is this your suggestion/design? > The said behaviour would allow multiple SVG document fragments with complex > content in parallel on one page, each being pan- and zommable and still > keeping image like SVGs not being zoomable by defining zoomAndPan="disable" > explicitly on the embedded SVG. > I could not follow this part also. zoomAndPan, currentScale and currentTranslate are defined only on the root SVG element. When you say "defining zoomAndPan="disable" explicitly on the embedded SVG." are you referring to implementation detail or user setting? And what do you mean by "the embedded SVG" in this context? When you mention "SVG document fragments", do you mean embedded SVG in an HTML page? For example the following html page contains two SVGs and these what I call embedded SVGs. <html> <body>  <img src="x.svg"> <svg> ... </svg> </body> </html> > Makes sense? My understanding about currentScale and currentTranslate was they are kind of transform applied on the root SVG. But when I read the zoomAndPan attribute, I realized that these attributes are intended to be integrated and synced with some UI state. They are a programmable way to mimimc the user interaction. For example, if a user opens a standalone SVG and hold Shift+Click and drag the SVG, the currentTranslate of the root SVG element will hold the dragged amount. If the following script runs later: var root = document.documentElement; root.currentTranslate.x = 100; root.currentTranslate.y = 100; The currentTranslate will be overridden as if the user drags the SVG from (100, 100). So I am fixing the patch to reflect that but for the standalone SVG since this is what WebKit has been supporting. For the case of embedded SVGs I think you might need to reach the w3c SVG working group with your suggestion/design.

Mathias Menzel

Comment 17 2016-05-16 12:34:53 PDT

(In reply to comment #16) > (In reply to comment #12) > > Please correct if I am wrong or if that does not make sense. > > > > I didn't want to start of with the discussion, if currentScale and > > currentTranslate do the same thing, but my intent was to say, that, even so > > they are specified to operate on the same object, they don't. > > > > I read the specs so, that it starts with the zoomAndPan attribute on an > > outermost SVG Element > > (http://www.w3.org/TR/SVG/single-page.html#interact-ZoomAndPanAttribute). If > > that is set to "disable", currentScale and currentTranslate are unchangeable > > (i.e. set to 1 and (0,0)). > > For user interactable environments (browsers), this attribute defaults to > > "magnify", so the user agent should enable panning and zooming on the SVG > > document fragment (acc. to specs - Mozilla explicitly states that they did > > not implement that). > > > > I can see why chose to do that. I think the specs is unclear regarding the > case of embedded SVG. > > > So zooming is changing the currentScale of the SVG object fragment (not the > > whole embedding page) and panning changes the currentTranslate implicitly > > (that's why currentTranslate is marked readonly in the specs, > > currentTranslate shouldn't be settable by the user directly, but is now). > > I think this is not true. currentTranslate is read-only because you can't > change the SVGPoint attribute itself. But you can still change the x and the > y members of this attribute. For example this code translates a standalone > SVG by (100, 100): > > var root = document.documentElement; > root.currentTranslate.x = 100; > root.currentTranslate.y = 100; > > Also in Safari, if you open a standalone SVG and you hold Shift+Click and > drag, the SVG will be translated by the amount of dragging. I agree, that the latter is the desired behaviour, this sets the currentTranslate by user interaction. Though I first had a different understanding reading the specs, I think setting currentTranslate.x/y by script is fine. > > > Lastly the overflow comes in and does what overflow does in CSS2+ (that's > > why there should be scrollbars I assume, just like on fixed width DIVs with > > "overflow:scroll", that's what has to be wrapped round SVG to be scrollable, > > but shouldn't be necessary). This all only applies to outermost SVG Elements. > > > > In regards to a zoomed-in page (cmd+) the zoom-in on SVG Objects should > > still be possible. So the page zoom enlargens the whole page alright, but if > > I mouseover an SVG document fragment I would expect only the SVG to zoom > > (e.g. by strg+mousscroll) and pan (e.g. by dragging the SVG), similar to > > DIVs with overflow content, when scrolled. > > On SVG document fragments, the scroll would set the currentScale, the pan > > would set the currentTranslate. And the whole page scale would not touch the > > descriptive SVG Object, but most likely would do the scale by other means (I > > checked other browsers, e.g. IE keeps currentScale to 1 when whole page is > > scaled), which probably should not be part of this thread. > > > > I could not follow this part regarding the zooming and panning of an > embedded SVG. Is this part of any specs? Or is this your suggestion/design? > > > The said behaviour would allow multiple SVG document fragments with complex > > content in parallel on one page, each being pan- and zommable and still > > keeping image like SVGs not being zoomable by defining zoomAndPan="disable" > > explicitly on the embedded SVG. > > > > I could not follow this part also. zoomAndPan, currentScale and > currentTranslate are defined only on the root SVG element. When you say > "defining zoomAndPan="disable" explicitly on the embedded SVG." are you > referring to implementation detail or user setting? And what do you mean by > "the embedded SVG" in this context? > > When you mention "SVG document fragments", do you mean embedded SVG in an > HTML page? For example the following html page contains two SVGs and these > what I call embedded SVGs. Yes, I call that embedded too. As how I read the Specs, they call it fragment: http://www.w3.org/TR/SVG/intro.html#TermSVGDocumentFragment > > <html> > <body> >  > <img src="x.svg"> > <svg> > ... > </svg> > </body> > </html> > > > Makes sense? > > My understanding about currentScale and currentTranslate was they are kind > of transform applied on the root SVG. But when I read the zoomAndPan > attribute, I realized that these attributes are intended to be integrated > and synced with some UI state. They are a programmable way to mimimc the > user interaction. For example, if a user opens a standalone SVG and hold > Shift+Click and drag the SVG, the currentTranslate of the root SVG element > will hold the dragged amount. If the following script runs later: > > var root = document.documentElement; > root.currentTranslate.x = 100; > root.currentTranslate.y = 100; > > The currentTranslate will be overridden as if the user drags the SVG from > (100, 100). Yes, I agree. > > So I am fixing the patch to reflect that but for the standalone SVG since > this is what WebKit has been supporting. For the case of embedded SVGs I > think you might need to reach the w3c SVG working group with your > suggestion/design. For me (and I thought this in line with the specs, but maybe I am mistaken) a "root" Element of SVG is any <svg> TAG, that is the outermost <svg> TAG of an SVG document. The specs call that "rootmost 'svg' element": http://www.w3.org/TR/SVG/intro.html#TermRootmostSVGElement Let's assume we have two standalone SVG documents (just to make it short without doctype etc.): --- svg 1 --- <?xml version="1.0" encoding="UTF-8" standalone="yes"?> <svg id="svg1"> ... any svg tags, also including <svg> </svg> --- svg 2 --- <?xml version="1.0" encoding="UTF-8" standalone="yes"?> <svg id="svg2"> ... any svg tags, also including <svg> </svg> Each has a root element: svg1,svg2 respectively. Now when I embed that into X/HTML ... ------- <html> <body> <svg id="svg1"> ... any svg tags, also including <svg> </svg> <svg id="svg2"> ... any svg tags, also including <svg> </svg> </body> </html> ------- there are still two "root" <svg> Elements, i.e. svg1 and svg2. These are not the root of the SGML (here X/HTML) document, but they are still the root of there respective embedded SVG document (which they markup, so they start an SVG context). But with what you stated above (non-interactivity of embedded <svg>) I see, why those are treated differently from standalone SVG documents and indeed does the specs state in 16.5.1 that the behaviour of clicks on the rootmost svg element ist not defined in the SVG1.1 spec: http://www.w3.org/TR/SVG/interact.html#hit-testing (cite: future specifications may define this behavior, but for the purpose of this specification, the behavior is implementation-specific) That's probably where at least my confusion comes from, why standalone and direct embedded SVG documents (...<body>...<svg></svg>...<body>...) are treated somewhat differently. So thanks for making void the propagation of currentScale to the page root level in multi embedded SVG documents and let's see where V2.0 will take us ...

Radar WebKit Bug Importer

Comment 18 2022-07-15 17:20:30 PDT

<rdar://problem/97102247>

Note You need to log in before you can comment on or make changes to this bug.