third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp - Issue 1827793003: Support currentScale for embedded SVG

Side by Side Diff: third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp

Issue 1827793003: Support currentScale for embedded SVG (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 4 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html ('K') | « third_party/WebKit/LayoutTests/svg/custom/resources/green-rect.svg ('k') | third_party/WebKit/Source/core/svg/SVGSVGElement.cpp » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 /*	1 /*

2 * Copyright (C) 2004, 2005, 2007 Nikolas Zimmermann <zimmermann@kde.org>	2 * Copyright (C) 2004, 2005, 2007 Nikolas Zimmermann <zimmermann@kde.org>

3 * Copyright (C) 2004, 2005, 2007, 2008, 2009 Rob Buis <buis@kde.org>	3 * Copyright (C) 2004, 2005, 2007, 2008, 2009 Rob Buis <buis@kde.org>

4 * Copyright (C) 2007 Eric Seidel <eric@webkit.org>	4 * Copyright (C) 2007 Eric Seidel <eric@webkit.org>

5 * Copyright (C) 2009 Google, Inc.	5 * Copyright (C) 2009 Google, Inc.

6 * Copyright (C) Research In Motion Limited 2011. All rights reserved.	6 * Copyright (C) Research In Motion Limited 2011. All rights reserved.

7 *	7 *

8 * This library is free software; you can redistribute it and/or	8 * This library is free software; you can redistribute it and/or

9 * modify it under the terms of the GNU Library General Public	9 * modify it under the terms of the GNU Library General Public

10 * License as published by the Free Software Foundation; either	10 * License as published by the Free Software Foundation; either

(...skipping 283 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
294	294

295 // LayoutBox methods will expect coordinates w/o any transforms in coordinates	295 // LayoutBox methods will expect coordinates w/o any transforms in coordinates

296 // relative to our borderBox origin. This method gives us exactly that.	296 // relative to our borderBox origin. This method gives us exactly that.

297 void LayoutSVGRoot::buildLocalToBorderBoxTransform()	297 void LayoutSVGRoot::buildLocalToBorderBoxTransform()

298 {	298 {

299 SVGSVGElement* svg = toSVGSVGElement(node());	299 SVGSVGElement* svg = toSVGSVGElement(node());

300 ASSERT(svg);	300 ASSERT(svg);

301 float scale = style()->effectiveZoom();	301 float scale = style()->effectiveZoom();

302 FloatPoint translate = svg->currentTranslate();	302 FloatPoint translate = svg->currentTranslate();

303 LayoutSize borderAndPadding(borderLeft() + paddingLeft(), borderTop() + padd ingTop());	303 LayoutSize borderAndPadding(borderLeft() + paddingLeft(), borderTop() + padd ingTop());

304 m_localToBorderBoxTransform = svg->viewBoxToViewTransform(contentWidth() / s cale, contentHeight() / scale);	304 float viewWidth = contentWidth();

	305 float viewHeight = contentHeight();

	306

	307 if (isEmbeddedThroughFrameContainingSVGDocument()) {
	fs 2016/03/29 11:58:51 I think a better way to structure this code would I think a better way to structure this code would be to have a function/method that computes the scale to use, and then compute the view width/height and call viewBoxToViewTransform() with those.
	308 LayoutPart* ownerLayoutObject = svg->document().frame()->ownerLayoutObje ct();

	309 float currentPageZoom = svg->document().frame()->pageZoomFactor();
	fs 2016/03/29 11:58:51 Won't LocalFrame::setPageZoomFactor cause the effe Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, meaning that the effective zoom alone will be enough here? (Also, the "if parent frame zoom and current frame zoom differs - then use owner zoom" feels kind of dodgy...) Shanmuga Pandi 2016/03/30 08:40:06 Yes. LocalFrame::setPageZoomFactor will cause the Show quoted text On 2016/03/29 11:58:51, fs wrote: > Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, meaning > that the effective zoom alone will be enough here? (Also, the "if parent frame > zoom and current frame zoom differs - then use owner zoom" feels kind of > dodgy...) Yes. LocalFrame::setPageZoomFactor will cause the the effective zoom to update. But consider the below case. <style> body { zoom: 50%;} </style> <body> <object type="image/svg+xml" data="rect.svg"> </object> </body> With that content, I could see value of currentPageZoom and ownerPageZoom is 1. And style()->effectiveZoom() is 1 and ownerLayoutObject->style()->effectiveZoom() is 0.5. So I felt to choose comparing the parent frame zoom and current frame zoom rather than effectiveZoom. consider the case, mainFrame pageZoom as 0.5 childFrame pageZoom as 0.25 svg rect on childFrame with size 100X100, should be displayed as 25x25. And by applying the currentScale to only to current frame, will not change the FrameView::setFrameRect and layout size of layoutEmbeddedObject will be based on parentFrame. To compensate that I have divided with parent scale. fs 2016/03/30 09:04:12 I don't think you should be comparing anything at Show quoted text On 2016/03/30 at 08:40:06, Shanmuga Pandi wrote: > On 2016/03/29 11:58:51, fs wrote: > > Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, meaning > > that the effective zoom alone will be enough here? (Also, the "if parent frame > > zoom and current frame zoom differs - then use owner zoom" feels kind of > > dodgy...) > > Yes. LocalFrame::setPageZoomFactor will cause the the effective zoom to update. > But consider the below case. > > <style> > body { zoom: 50%;} > </style> > > <body> > <object type="image/svg+xml" data="rect.svg"> > </object> > </body> > > > With that content, > I could see value of currentPageZoom and ownerPageZoom is 1. > And style()->effectiveZoom() is 1 and ownerLayoutObject->style()->effectiveZoom() is 0.5. So I felt to choose comparing the parent frame zoom and current frame zoom rather than effectiveZoom. I don't think you should be comparing anything at all. Show quoted text > consider the case, > mainFrame pageZoom as 0.5 > childFrame pageZoom as 0.25 > svg rect on childFrame with size 100X100, should be displayed as 25x25. More like 0.5 * 0.25 * 100 = 12.5 I think. Show quoted text > And by applying the currentScale to only to current frame, will not change the FrameView::setFrameRect and layout size of layoutEmbeddedObject will be based on parentFrame. To compensate that I have divided with parent scale. It would be expected that the layout size of the embedding "frame" would not change. It's also quite possible that this is where using the "page zoom" to implement currentScale starts to crumble (and may explain why this wasn't done in the first place.) The more obvious implementation of currentScale would be to keep a separate currentScale and apply that before the viewBox transform but after the "border box transform". Shanmuga Pandi 2016/03/30 09:17:38 Yes . I agree with that. Layout size of embedding Show quoted text On 2016/03/30 09:04:12, fs wrote: > On 2016/03/30 at 08:40:06, Shanmuga Pandi wrote: > > On 2016/03/29 11:58:51, fs wrote: > > > Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, > meaning > > > that the effective zoom alone will be enough here? (Also, the "if parent > frame > > > zoom and current frame zoom differs - then use owner zoom" feels kind of > > > dodgy...) > > > > Yes. LocalFrame::setPageZoomFactor will cause the the effective zoom to > update. > > But consider the below case. > > > > <style> > > body { zoom: 50%;} > > </style> > > > > <body> > > <object type="image/svg+xml" data="rect.svg"> > > </object> > > </body> > > > > > > With that content, > > I could see value of currentPageZoom and ownerPageZoom is 1. > > And style()->effectiveZoom() is 1 and > ownerLayoutObject->style()->effectiveZoom() is 0.5. So I felt to choose > comparing the parent frame zoom and current frame zoom rather than > effectiveZoom. > > I don't think you should be comparing anything at all. > > > consider the case, > > mainFrame pageZoom as 0.5 > > childFrame pageZoom as 0.25 > > svg rect on childFrame with size 100X100, should be displayed as 25x25. > > More like 0.5 * 0.25 * 100 = 12.5 I think. > > > And by applying the currentScale to only to current frame, will not change the > FrameView::setFrameRect and layout size of layoutEmbeddedObject will be based on > parentFrame. To compensate that I have divided with parent scale. > > It would be expected that the layout size of the embedding "frame" would not > change. It's also quite possible that this is where using the "page zoom" to > implement currentScale starts to crumble (and may explain why this wasn't done > in the first place.) Yes . I agree with that. Layout size of embedding frame should not change. Show quoted text > The more obvious implementation of currentScale would be to > keep a separate currentScale and apply that before the viewBox transform but > after the "border box transform". Could you little more elaborate ? It will helpful to understand better!! fs 2016/03/30 10:16:30 Well, something like: AffineTransform viewToBorde Show quoted text On 2016/03/30 at 09:17:38, Shanmuga Pandi wrote: > On 2016/03/30 09:04:12, fs wrote: > > On 2016/03/30 at 08:40:06, Shanmuga Pandi wrote: > > > On 2016/03/29 11:58:51, fs wrote: > > > > Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, > > meaning > > > > that the effective zoom alone will be enough here? (Also, the "if parent > > frame > > > > zoom and current frame zoom differs - then use owner zoom" feels kind of > > > > dodgy...) > > > > > > Yes. LocalFrame::setPageZoomFactor will cause the the effective zoom to > > update. > > > But consider the below case. > > > > > > <style> > > > body { zoom: 50%;} > > > </style> > > > > > > <body> > > > <object type="image/svg+xml" data="rect.svg"> > > > </object> > > > </body> > > > > > > > > > With that content, > > > I could see value of currentPageZoom and ownerPageZoom is 1. > > > And style()->effectiveZoom() is 1 and > > ownerLayoutObject->style()->effectiveZoom() is 0.5. So I felt to choose > > comparing the parent frame zoom and current frame zoom rather than > > effectiveZoom. > > > > I don't think you should be comparing anything at all. > > > > > consider the case, > > > mainFrame pageZoom as 0.5 > > > childFrame pageZoom as 0.25 > > > svg rect on childFrame with size 100X100, should be displayed as 25x25. > > > > More like 0.5 * 0.25 * 100 = 12.5 I think. > > > > > And by applying the currentScale to only to current frame, will not change the > > FrameView::setFrameRect and layout size of layoutEmbeddedObject will be based on > > parentFrame. To compensate that I have divided with parent scale. > > > > > > It would be expected that the layout size of the embedding "frame" would not > > change. It's also quite possible that this is where using the "page zoom" to > > implement currentScale starts to crumble (and may explain why this wasn't done > > in the first place.) > > Yes . I agree with that. Layout size of embedding frame should not change. > > > The more obvious implementation of currentScale would be to > > keep a separate currentScale and apply that before the viewBox transform but > > after the "border box transform". > > Could you little more elaborate ? It will helpful to understand better!! Well, something like: AffineTransform viewToBorderBoxTransform(scale, 0, 0, scale, borderAndPadding.width() + translate.x(), borderAndPadding.height() + translate.y()); viewToBorderBoxTransform.scale(svg->currentScale()); ... I.e have SVGSVGElement::m_currentScale and then have the accessors read/update that (rather than modifying "page zoom"). Shanmuga Pandi 2016/03/30 10:23:30 Thank you for your suggestion. But What will be t Show quoted text On 2016/03/30 10:16:30, fs wrote: > On 2016/03/30 at 09:17:38, Shanmuga Pandi wrote: > > On 2016/03/30 09:04:12, fs wrote: > > > On 2016/03/30 at 08:40:06, Shanmuga Pandi wrote: > > > > On 2016/03/29 11:58:51, fs wrote: > > > > > Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, > > > meaning > > > > > that the effective zoom alone will be enough here? (Also, the "if parent > > > frame > > > > > zoom and current frame zoom differs - then use owner zoom" feels kind of > > > > > dodgy...) > > > > > > > > Yes. LocalFrame::setPageZoomFactor will cause the the effective zoom to > > > update. > > > > But consider the below case. > > > > > > > > <style> > > > > body { zoom: 50%;} > > > > </style> > > > > > > > > <body> > > > > <object type="image/svg+xml" data="rect.svg"> > > > > </object> > > > > </body> > > > > > > > > > > > > With that content, > > > > I could see value of currentPageZoom and ownerPageZoom is 1. > > > > And style()->effectiveZoom() is 1 and > > > ownerLayoutObject->style()->effectiveZoom() is 0.5. So I felt to choose > > > comparing the parent frame zoom and current frame zoom rather than > > > effectiveZoom. > > > > > > I don't think you should be comparing anything at all. > > > > > > > consider the case, > > > > mainFrame pageZoom as 0.5 > > > > childFrame pageZoom as 0.25 > > > > svg rect on childFrame with size 100X100, should be displayed as 25x25. > > > > > > More like 0.5 * 0.25 * 100 = 12.5 I think. > > > > > > > And by applying the currentScale to only to current frame, will not change > the > > > FrameView::setFrameRect and layout size of layoutEmbeddedObject will be > based on > > > parentFrame. To compensate that I have divided with parent scale. > > > > > > > > > > It would be expected that the layout size of the embedding "frame" would not > > > change. It's also quite possible that this is where using the "page zoom" to > > > implement currentScale starts to crumble (and may explain why this wasn't > done > > > in the first place.) > > > > Yes . I agree with that. Layout size of embedding frame should not change. > > > > > The more obvious implementation of currentScale would be to > > > keep a separate currentScale and apply that before the viewBox transform but > > > after the "border box transform". > > > > Could you little more elaborate ? It will helpful to understand better!! > > Well, something like: > > AffineTransform viewToBorderBoxTransform(scale, 0, 0, scale, > borderAndPadding.width() + translate.x(), borderAndPadding.height() + > translate.y()); > viewToBorderBoxTransform.scale(svg->currentScale()); > ... > > I.e have SVGSVGElement::m_currentScale and then have the accessors read/update > that (rather than modifying "page zoom"). Thank you for your suggestion. But What will be the behavior with currentScale with svg which is in mainFrame ? We still need to modify "page zoom" ? fs 2016/03/30 10:46:39 That would be an option I guess, there's nothing t Show quoted text On 2016/03/30 at 10:23:30, Shanmuga Pandi wrote: > On 2016/03/30 10:16:30, fs wrote: > > On 2016/03/30 at 09:17:38, Shanmuga Pandi wrote: > > > On 2016/03/30 09:04:12, fs wrote: > > > > On 2016/03/30 at 08:40:06, Shanmuga Pandi wrote: > > > > > On 2016/03/29 11:58:51, fs wrote: > > > > > > Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, > > > > meaning > > > > > > that the effective zoom alone will be enough here? (Also, the "if parent > > > > frame > > > > > > zoom and current frame zoom differs - then use owner zoom" feels kind of > > > > > > dodgy...) > > > > > > > > > > Yes. LocalFrame::setPageZoomFactor will cause the the effective zoom to > > > > update. > > > > > But consider the below case. > > > > > > > > > > <style> > > > > > body { zoom: 50%;} > > > > > </style> > > > > > > > > > > <body> > > > > > <object type="image/svg+xml" data="rect.svg"> > > > > > </object> > > > > > </body> > > > > > > > > > > > > > > > With that content, > > > > > I could see value of currentPageZoom and ownerPageZoom is 1. > > > > > And style()->effectiveZoom() is 1 and > > > > ownerLayoutObject->style()->effectiveZoom() is 0.5. So I felt to choose > > > > comparing the parent frame zoom and current frame zoom rather than > > > > effectiveZoom. > > > > > > > > I don't think you should be comparing anything at all. > > > > > > > > > consider the case, > > > > > mainFrame pageZoom as 0.5 > > > > > childFrame pageZoom as 0.25 > > > > > svg rect on childFrame with size 100X100, should be displayed as 25x25. > > > > > > > > More like 0.5 * 0.25 * 100 = 12.5 I think. > > > > > > > > > And by applying the currentScale to only to current frame, will not change > > the > > > > FrameView::setFrameRect and layout size of layoutEmbeddedObject will be > > based on > > > > parentFrame. To compensate that I have divided with parent scale. > > > > > > > > > > > > > > It would be expected that the layout size of the embedding "frame" would not > > > > change. It's also quite possible that this is where using the "page zoom" to > > > > implement currentScale starts to crumble (and may explain why this wasn't > > done > > > > in the first place.) > > > > > > Yes . I agree with that. Layout size of embedding frame should not change. > > > > > > > The more obvious implementation of currentScale would be to > > > > keep a separate currentScale and apply that before the viewBox transform but > > > > after the "border box transform". > > > > > > Could you little more elaborate ? It will helpful to understand better!! > > > > Well, something like: > > > > AffineTransform viewToBorderBoxTransform(scale, 0, 0, scale, > > borderAndPadding.width() + translate.x(), borderAndPadding.height() + > > translate.y()); > > viewToBorderBoxTransform.scale(svg->currentScale()); > > ... > > > > I.e have SVGSVGElement::m_currentScale and then have the accessors read/update > > that (rather than modifying "page zoom"). > > Thank you for your suggestion. > But What will be the behavior with currentScale with svg which is in mainFrame ? We still need to modify "page zoom" ? That would be an option I guess, there's nothing that prevents doing the same everywhere though I think. (This would also work for case where "outermost svg" != document root.)
	310 float ownerPageZoom = ownerLayoutObject->document().frame()->pageZoomFac tor();

	311 if (currentPageZoom != ownerPageZoom)

	312 m_localToBorderBoxTransform = svg->viewBoxToViewTransform(viewWidth / ownerPageZoom, viewHeight / ownerPageZoom);

	313 else

	314 m_localToBorderBoxTransform = svg->viewBoxToViewTransform(viewWidth / scale, viewHeight / scale);

	315 } else {

	316 m_localToBorderBoxTransform = svg->viewBoxToViewTransform(viewWidth / sc ale, viewHeight / scale);

	317 }

305	318

306 AffineTransform viewToBorderBoxTransform(scale, 0, 0, scale, borderAndPaddin g.width() + translate.x(), borderAndPadding.height() + translate.y());	319 AffineTransform viewToBorderBoxTransform(scale, 0, 0, scale, borderAndPaddin g.width() + translate.x(), borderAndPadding.height() + translate.y());

307 m_localToBorderBoxTransform.preMultiply(viewToBorderBoxTransform);	320 m_localToBorderBoxTransform.preMultiply(viewToBorderBoxTransform);

308 }	321 }

309	322

310 const AffineTransform& LayoutSVGRoot::localToParentTransform() const	323 const AffineTransform& LayoutSVGRoot::localToParentTransform() const

311 {	324 {

312 // Slightly optimized version of m_localToParentTransform = AffineTransform: :translation(x(), y()) * m_localToBorderBoxTransform;	325 // Slightly optimized version of m_localToParentTransform = AffineTransform: :translation(x(), y()) * m_localToBorderBoxTransform;

313 m_localToParentTransform = m_localToBorderBoxTransform;	326 m_localToParentTransform = m_localToBorderBoxTransform;

314 if (location().x())	327 if (location().x())

(...skipping 105 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
420 updateHitTestResult(result, pointInBorderBox);	433 updateHitTestResult(result, pointInBorderBox);

421 if (result.addNodeToListBasedTestResult(node(), locationInContainer, boundsRect) == StopHitTesting)	434 if (result.addNodeToListBasedTestResult(node(), locationInContainer, boundsRect) == StopHitTesting)

422 return true;	435 return true;

423 }	436 }

424 }	437 }

425	438

426 return false;	439 return false;

427 }	440 }

428	441

429 } // namespace blink	442 } // namespace blink

OLD	NEW