Bug 11272 – Implement currentScale(), setCurrentScale() and currentTranslate() in SVGSVGElement

• Component: SVG
• Status: RESOLVED FIXED

[Rob Buis]

Implement the getting/setting of the property currentScale and getting of property currentTranslate so the FIXMEs can be removed.

[Rob Buis]

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.

[Rob Buis]

Created attachment 14579 [details]
Now should compile

Olliej noticed the previous patch didnt compile, this one should.
Cheers,

Rob.

[Nikolas Zimmermann]

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

[Rob Buis]

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.

[Maciej Stachowiak]

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.

[Rob Buis]

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.

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

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.

[Rob Buis]

Created attachment 14782 [details]
Without the webkit changes

As agreed, this time without the webkit changes to the context menu.
Cheers,

Rob.

[Darin Adler]

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.

[Rob Buis]

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.

[Rob Buis]

Created attachment 14784 [details]
Cleaning up

This should address Darin's issues.
Cheers,

Rob.

[Darin Adler]

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

[Rob Buis]

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.

[Sam Weinig]

Landed in r21925.