Implement the getting/setting of the property currentScale and getting of property currentTranslate so the FIXMEs can be removed.
Created attachment 14563 [details] First attempt This patch allows simple zooming and panning. I think some of the values can be stored in the svg container in the render tree as well, but that would need first some cleanup of the svg render classes, as already listed in another bug report. Nevertheless I think this patch is valuable, going into the experimental branch first is also an option. Panning should work on all platforms, zooming only on Mac (since AFAIK others have no context menu's where I can easily add Zoom entries to). Cheers, Rob.
Created attachment 14579 [details] Now should compile Olliej noticed the previous patch didnt compile, this one should. Cheers, Rob.
Hi Rob, I think the incZoom() etc methods should at least go into SVGDocument, if not somewhere else where it fits better. Suggestions? Greetings, Niko
Hi Niko, (In reply to comment #3) > Hi Rob, > > I think the incZoom() etc methods should at least go into SVGDocument, > if not somewhere else where it fits better. Suggestions? They are in SVGDocument now :) I also added them to Document.h since SVGDocument.h was not visible for WebKit. I now know it can be made visible. We could stick with the methods being only in SVGDocument, but some more #if ENABLE(SVG) constructions would be needed then, which are not always considered nice. Anyway this and other issues will probably come up during reviewing. Also notice that probably the svg render object handling the outer <svg> should know enough about it to ask the zoom and pan factor from the SVGSVGElement, and other svg container render objects don't need to know about these values. But I propose to do that later (there already is a bug report for it). Cheers, Rob.
Instead of having SVG-specific zoom in and out menu items, I would rather see SVG work off of the scale factor that is used for text zoom in HTML. Then Cmd-plus and Cmd-minus would work in apps that bind those to text zoom, as would the menu items. I think on mac at least we already do this for PDF.
Created attachment 14629 [details] Using zoomFactor 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. Cheers, Rob.
I would suggest not adding the zoom items to the context menu - I think the menu items, keyboard shortcuts, and optional toolbar buttons are more than enough UI for zooming. The rest of this seems good to go for the feature branch.
Comment on attachment 14629 [details] Using zoomFactor r- for the context menu items, I think it's best not to add those and instead we should make the text zoom menu item labels dynamic.
Created attachment 14782 [details] Without the webkit changes As agreed, this time without the webkit changes to the context menu. Cheers, Rob.
Comment on attachment 14782 [details] Without the webkit changes I don't think we should necessarily mix the zoom factor together with the text size multiplier. The text size multiplier is still called "zoomFactor" in Frame.cpp, but I think we may want a separate way to manipulate the zoom factor and text size multiplier independently. And I think a zoom factor should zoom everything, not just SVG content. + if (document() && document()->frame()) + return float(document()->frame()->zoomFactor()) / 100.; + return 1.0; The right way to write the above so it will be floating point is: if (document() && document()->frame()) return document()->frame()->zoomFactor() / 100.0f; return 1.0f; There's no need for the cast to "float". + document()->frame()->setZoomFactor(scale / 100.); This will do the math as double. You want "scale / 100.0f". The word "translate" is a verb not a noun. It should be "translation" instead in the various function names. - //canvasView()->enableZoomAndPan(zoomAndPan == SVG_ZOOMANDPAN_MAGNIFY); We normally don't check in commented-out code. if we must check this in, it needs a comment to indicate why it's commented out. return m_lastScrollbarUnderMouse->handleMouseMoveEvent(mouseEvent); - // Treat mouse move events while the mouse is pressed as "read-only" in prepareMouseEvent Why remove this blank line? + static_cast<SVGDocument*>(m_frame->document())->zoomEnabled()) { Why does zoomEnabled() control panning? Should it be named zoomAndPanEnabled() instead? + AffineTransform ctm; This local variable is unused? + m_width = float(m_width) * svg->currentScale(); + m_height = float(m_height) * svg->currentScale(); No need for the cast to float here. review- at least for the "translate" name.
Hi Darin, (In reply to comment #10) > (From update of attachment 14782 [details] [edit]) > I don't think we should necessarily mix the zoom factor together with the text > size multiplier. The text size multiplier is still called "zoomFactor" in > Frame.cpp, but I think we may want a separate way to manipulate the zoom factor > and text size multiplier independently. And I think a zoom factor should zoom > everything, not just SVG content. Right, Text zoom seems very specific and doesn't translate well to the svg world. My original patch had them separated. > + if (document() && document()->frame()) > + return float(document()->frame()->zoomFactor()) / 100.; > + return 1.0; > > The right way to write the above so it will be floating point is: > > if (document() && document()->frame()) > return document()->frame()->zoomFactor() / 100.0f; > return 1.0f; > > There's no need for the cast to "float". Fixed. > + document()->frame()->setZoomFactor(scale / 100.); > > This will do the math as double. You want "scale / 100.0f". Fixed. > The word "translate" is a verb not a noun. It should be "translation" instead > in the various function names. Fixed. > - //canvasView()->enableZoomAndPan(zoomAndPan == SVG_ZOOMANDPAN_MAGNIFY); > > We normally don't check in commented-out code. if we must check this in, it > needs a comment to indicate why it's commented out. Its being removed here :) > return m_lastScrollbarUnderMouse->handleMouseMoveEvent(mouseEvent); > - > // Treat mouse move events while the mouse is pressed as "read-only" in > prepareMouseEvent > > Why remove this blank line? No particular reason. Fixed. > + static_cast<SVGDocument*>(m_frame->document())->zoomEnabled()) { > > Why does zoomEnabled() control panning? Should it be named zoomAndPanEnabled() > instead? Agreed. Fixed. > + AffineTransform ctm; > > This local variable is unused? Maybe copy and paste error. Fixed. > + m_width = float(m_width) * svg->currentScale(); > + m_height = float(m_height) * svg->currentScale(); > > No need for the cast to float here. Fixed. Cheers, Rob.
Created attachment 14784 [details] Cleaning up This should address Darin's issues. Cheers, Rob.
Comment on attachment 14784 [details] Cleaning up + rootElement()->setCurrentTranslate(FloatPoint(pos.x() - m_translate.x(), m_translate.y() - pos.y())); + if (renderer()) + renderer()->repaint(); Shouldn't setCurrentTranslate be the code doing the repaint? + FloatPoint m_translate; I think this should be removed. + if (m_svgPan) { + m_svgPan = false; + return true; + } I think we want to do a final "updatePan" at the end location. + m_width = float(m_width) * svg->currentScale(); + m_height = float(m_height) * svg->currentScale(); This still has the cast, even though you mentioned removing it in your comments. review+, although I think this needs a little refinement
Hi Darin, (In reply to comment #13) > (From update of attachment 14784 [details] [edit]) > + rootElement()->setCurrentTranslate(FloatPoint(pos.x() - > m_translate.x(), m_translate.y() - pos.y())); > + if (renderer()) > + renderer()->repaint(); > > Shouldn't setCurrentTranslate be the code doing the repaint? Right, that is better, will do. > + FloatPoint m_translate; > > I think this should be removed. I am not sure that is possible. We have to keep track of intermendiate results like that I think. > + if (m_svgPan) { > + m_svgPan = false; > + return true; > + } > > I think we want to do a final "updatePan" at the end location. Right, will fix. > + m_width = float(m_width) * svg->currentScale(); > + m_height = float(m_height) * svg->currentScale(); > > This still has the cast, even though you mentioned removing it in your > comments. Right, I searched for (float), should have searched for float(. Will also fix. Patch will go to the feature branch. Cheers, Rob.
Landed in r21925.