163068 - (zoompan) Implement pan and zoom controls

Status: RESOLVED WONTFIX

(Core :: SVG, defect)

Description

[Jason Davis]

Per Alex Fritze's request, opening bug and assigning myself ownership for an XBL
implementation of the pan and zoom controls.

Comment 1

[Jason Davis]

marked dependent on bug 71191 (can't have binding on outermost element in an xml
document).

It blocks standalone SVG controls, but not for SVG embedded in another document.

Comment 2

[Jonathan Watt [:jwatt]]

Do you have part of the patch written Jason? I would be interested to see it. 

Comment 3

[Jason Davis]

Yes, I have a basic implementation done right now, but am working out the kinks.
I'll probably attach a finished copy later tonight or tomorrow.

Comment 4

[Jason Davis]

Jonathan, working out the bugs is taking longer than I thought.
http://rdhtml.resource-locator.com/svg/mozilla/
is where what I currently have resides. See
http://groups.google.com/groups?dq=&hl=en&lr=&ie=UTF-8&oe=UTF-8&threadm=3D60A974.2080402%40pinkjuice.com&prev=/groups%3Fhl%3Den%26lr%3D%26ie%3DUTF-8%26oe%3DUTF-8%26group%3Dnetscape.public.mozilla.svg
(or just check out n.p.m.svg - not sure how quickly Google updates newsgroups
with new postings) for more detailed info.

Comment 5

[Jonathan Watt [:jwatt]]

Great, thanks Jason. I'll have a look through that. 

Comment 6

[Jonathan Watt [:jwatt]]

I have made some changes to your zoom code to remove the code that creates extra
objects. Instead I have just applied the changes directly to a single
SVGTransform object belonging to the 'g' element. This would therefore require
that only one transform be applied to the 'g' element in the 'content' tag
otherwise the second will apply an extra transform. 


	var box = this.ownerDocument.getBoxObjectFor(this);
	var anonG = this.ownerDocument.getAnonymousNodes(this).item(0);

	var transform = anonG.transform.baseVal.getItem(0);
	var matrix = transform.matrix.scale(2);
	    matrix = matrix.translate(
		(box.width /2 - 2*event.clientX + matrix.e + 2*box.x) / matrix.a, 
     		(box.height/2 - 2*event.clientY + matrix.f + 2*box.y) / matrix.d
		);
	transform.setMatrix(matrix);

	this.currentScale = matrix.a;
	if (!this.currentTranslate)
	    this.currentTranslate = this.createSVGPoint();
	this.currentTranslate.x = matrix.e;
	this.currentTranslate.y = matrix.f;

Let me know what you think. The same idea would apply to the code for the other
bindings. 

Note: I don't have a svg.jar file in the .zip file I downloaded (the 1.1b svg
build) so for testing I had to create a webpage and mess with that. However it
seemed to work and I think I ported the code correctly. BTW, can anyone tell me
why I don't have a svg.jar file? It would be better if I could find my
svgBindings.xbl file so I can test directly. 

Comment 7

[Jonathan Watt [:jwatt]]

The algorithm I used previously was wrong, sorry about that. I have fixed it in
the following. Also, how about removing the zoom out code and changing the zoom
in code to handle both zooming in and out? It would just have to check for
event.shiftKey and the whole lot could be reduced to the following: 

	var box = this.ownerDocument.getBoxObjectFor(this);
	var anonG = this.ownerDocument.getAnonymousNodes(this).item(0);

	var factor = event.shiftKey? 0.5: 2;
	var transform = anonG.transform.baseVal.getItem(0);
	var matrix = transform.matrix.scale(2);
	    matrix = matrix.translate(
		(box.width/2  + factor*(box.x - event.clientX) + (factor-1)*matrix.e) / matrix.a, 
		(box.height/2 + factor*(box.y - event.clientY) + (factor-1)*matrix.f) / matrix.d
		);
	transform.setMatrix(matrix);

	this.currentScale = matrix.a;
	if (!this.currentTranslate)
	    this.currentTranslate = this.createSVGPoint();
	this.currentTranslate.x = matrix.e;
	this.currentTranslate.y = matrix.f;

Comment 8

[Jonathan Watt [:jwatt]]

And this should fix your pan problems: 

	var anonG = this.ownerDocument.getAnonymousNodes(this).item(0);
	var transform = anonG.transform.baseVal.getItem(0);
	var x = (event.clientX - window.x1) / transform.matrix.a;
	var y = (event.clientY - window.y1) / transform.matrix.d;
	transform.setMatrix(transform.matrix.translate(x, y));
	anonG.clickOffset = {x: event.clientX, y: event.clientY};

Just remember to check your code to make sure that there is only one transform
being set. There are currently a scale and a translate being set in several
places, and the translate will knock off the accuracy. 

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: zoom and pan 1

Okay, this works for me, but there is still plenty of scope for improvement. I
also removed the modifiers attribute from the zoom handler so that it could
accept events where event.altKey was both true and false. My thinking is that
the variable 'factor' could be worked out from a drag that creates a rectangle
like ASV. This would require the generic code that I have used that allows for
any value of zoom.

Comment 10

[Jason Davis]

Wow, excellent work. To make your code as transparent as possible, I did change
this.currentTranslate to anonG.currentTranslate (at least on my local copy) to
keep any pan and zoom variables to the anonymous <g>, but that's about it. Once
again, excellent.

I'll see what I can do about implementing the "zoom box" (I'm thinking a
dynamically generated <rect/> element with appropriate border styles might
suffice for visual representation).

Comment 11

[Jason Davis]

On second thought, I didn't see any need for currentTranslate and currentScale
properties, and got rid of them.

See here http://rdhtml.resource-locator.com/svg/mozilla/svgBindings.xml for a
very rough implementation of a zoom box. This ignores aspect ratios and zooms to
the box, while it seems ASV still respects aspect ratio. I also slightly changed
around the structure of the anonymous content to easily implement this. And the
box only forms when moving the mouse in "positive" directions - fixed easily
enough though in the mousemove handler, but I just wanted to quickly get
something to show. Zooming out from the box doesn't quite work as expected yet,
but its probably another trivial oversight on my part.

Comment 12

[Jonathan Watt [:jwatt]]

Thanks, I'm glad you're pleased with the changes. Concerning the currentScale
and currentTranslate properties, I was setting them on the SVGSVGElement so that
scripts in an SVG document would be able to discover the current zoom and offset
of the graphic. It was intended that they not be transparent. :) However, I did
notice that I was getting errors in the console, so they are perhaps best left
off for the moment (something about the props not being implemented). 

I have noticed that what ASV does to preserve the aspect ratio is take the
minimum ratio of either the hight of the graphic to the height of the zoom box,
or the width of the graphic to the width of the zoom box. This minimum scale is
then used for both the X and Y scaling. IMHO it is better to maintain the aspect
ratio like this. Note that if drag occurs when both the control and shift keys
are down then it is ignored. It doesn't make sense to zoom _out_ to the extents
of a box that is a sub-area of the current viewport after all. Again I like this
way of doing it. I.e. you can only zoom out with click. 

I implemented the draw the rect thing where you can drag to the right and the
left for something else I was doing. I will see if I can find it again and save
you the effort of thinking it through if you haven't already done it. 

Right, off to download your modified file now. BTW, can you attach your files as
attachments. Usually these files stick around longer here. I have just had a few
occasions where I have been looking over the development of an old bug and found
that the person has removed the file from their site. If it had been attached to
the bug I would still have been able to get it. Thanks Jason. :)

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: zoom to box

You can still zoom in and out by a factor of 2 and 0.5 respectively with click
and the ctrl (+alt) keys, but now you can also zoom to a box if you drag. I am
still not sure I like the idea of zooming out from a drag, but I kept it in
seeing as you coded for it Jason. Anyone care to comment. 

I see two main problems now: 

1) I don't know of a way to capture key events and we really need to be able to
do that to make this work properly. 

2) JavaScript is not really fast enough to smoothly update the rectangle when
dragged to the left or above of the mousedown point. This is because the width
and x values (and height and y values) of the rect are updated in separate
lines of code. This could possibly be smoothed by combining the values into a
style string and setting it all at once. However, I only just thought of that,
and it would require a rewrite so I will leave it until after feedback for now.


Oh, and we also need to find suitable cursors and a way of setting them
reliably.

Comment 14

[Jason Davis]

The issue with cursors has been discussed before in the newsgroup.

Essentially, Mozilla provides a suitable panning cursor through -moz-grab. The
ideal zoom in/out cursors imho would be similar if not the same as (if they're
free for use of course) Adobe's SVG Viewer. I checked, and it doesn't seem
Mozilla provides any zoom cursors, which means the only way to indicate it would
be through a custom cursor, i.e. cursor: url(chrome://svg/content/zoomin.cur);
for example.
See bug 38447 for the status of the url cursor value.

http://www.w3.org/TR/SVG/interact.html#Cursors provides a way to create a cursor
in SVG and use it. But to reference it, cursor: url() is still needed, and would
have to point to a fragment identifier (if I understand the specs correctly). So
we may not even need a premade cursor - writing one in SVG wouldn't be too hard
at all.

But it seems the only solutions are support for url(), or two additions to the
Mozilla cursor values, "-moz-zoom-in" and "-moz-zoom-out." I don't feel not
having visual representation warrants marking bug 38447 as a blocker though, but
I'd understand if someone else decides to change dependancies on this bug.

Comment 15

[Jonathan Watt [:jwatt]]

At present you can zoom the text in the browser window. However, David Baron
seems to have plans to eventually replace that with zooming that scales
everything together. With that in mind -moz-zoom-in and -moz-zoom-out cursors
may well be useful to parts of mozilla other than the svg component. My
preference would be to add the cursors to mozilla at it's core. 

Where did you find out about the built-in cursors? Can you post links? Also
seeing as you seem to know about them do you want to file a bug asking for them
to be added and depend this bug on it? Let me know your thoughts. 

Comment 16

[Jonathan Watt [:jwatt]]

Here are my thoughts on how zoom should ideally work. The same sort of thing
would go for pan but if we can get zoom implemented as described below that
should fall into place. 

The cursor should change to the zoom cursor when:
    1) The ctrl key is depressed while the cursor is over the SVG document area*
    2) The cursor enters the SVG document area while the ctrl key is depressed
    *  cannot be implemented with document events
    n: additionally, if the ctrl key is depressed the start point should be
registered

The zoom initialization coordinates should be registered when:
    1) The mouse is depressed when event.button == 0 && event.ctrlKey*
    *  can be done with document events

The zoom box should be drawn/updated when:
    1) onmousemove when zoom initialization coordinates exist*
    *  even when pointer moves outside window area! can't do with document events.

The zoom should execute when:
    1) onmouseup when zoom initialization coordinates exist*
    *  even when the pointer is outside window area! can't do with document events.
    
The the cursor should change to auto when:
    1) The ctrl key is released*
    *  even when the pointer is outside the window area!
    n: zoom should be terminated if it is in progress

Feedback on this would be great. 

Comment 17

[Jonathan Watt [:jwatt]]

Jason, thanks for the tip on -moz-user-focus. I was hopeing that being able to
capture the key events when the cursor is over the SVG would solve some of our
problems with your patch. Unfortunately unless you actually click on the SVG to
give it focus even with -moz-user-focus it won't capture the key events. We
could set focus on the svg element onmouseover, but that is a nasty hack and
would be noticable if the SVG was embeded in a page that had a textarea that a
user was typing in at the time they moved the mouse over the SVG for example. Do
you have any thoughts on getting around this?

Comment 18

[Jason Davis]

The XBL 1.0 specification calls for an "attachto" attribute on the <handler>
element, which can have a value of "document." Instead of binding the event to
the element, it will bind it to the document - which is exactly what we want.
http://www.mozilla.org/projects/xbl/xbl.html#handler

I've only tried using it once, exactly for this purpose, but it didn't work. I
didn't extensively test it afterwards, so it may or may not work.

The XBLRef project at MozDev confirms my suspicions by not listing it as an
attribute of <handler>:
http://www.mozdev.org/source/browse/~checkout~/xblref/docs/handler.html?rev=1.2&content-type=text/html

And after a quick search, putting my doubts at ease, bug 55265. It appears
attachto functionality was disabled for the 1.0 branch (and it appears to have
stayed disabled in 1.1) due to causing instability.

But attachto would solve some of the implementation's shortcomings I believe.

Also, looking at your description for what's required for pan and zoom
mouse/keyboard controls, they seem to be good, and from memory seem to be very
similar to Adobe's SVG Viewer's, which I believe should be a goal.

I'm not sure if bug 55265 is a blocker, or if a new bug filed to re-implement
attachto would be the blocker - I'm still a newb (to an extent) with BugZilla,
and am not sure which would be the correct action.

Comment 19

[Jonathan Watt [:jwatt]]

The reason I added bug 55288 to the list that this bug depends on is because we
won't be able to allow SVG authors to prevent the zoom and pan with
preventDefault until that bug is fixed. Anyway, I will leave it up to you
whether or not you want to re-add it. 

I know about attachto, but I didn't know it was disabled. Thanks for letting me
know. Re: bug 55265, they have asked that a new bug be opened to emplement
attachto, so we can't depend this bug on it. I have opened bug 164239, but I
won't depend this one on it myself coz it will probably just get removed by you.
;) Anyway, you know where it is. 

BTW, can you get the button attribute to work? I have had it working some times
and not at other times. I haven't figured out what is going on yet, but it would
be useful to be able to add button="0" to the appropriate handler elements. 

Comment 20

[Alex Fritze]

Adding a dependency on bug#172574:

We want our <svg>-element to have display style 'block', so that we can specify 
percentage-based units on the width/height attributes. However, because of 
bug#172574 we then can't have any binding attached to it (even if it's not 
root).

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: Inline SVG page

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: svgBindings.xml with zoom and pan

This patch updates the old bit-rotted patch. By replacing your existing
svgBindings.xml file with this one and adding the following CSS declaration to
svg.css you should be able to zoom and pan inline SVG. 

svg:not(:root) {
	-moz-binding:url('chrome://svg/content/svgBindings.xml#svg');
	-moz-user-focus: normal;
}

The behaviour is as follows:

Ctrl-click to zoom in by a factor of 2 centred on the point clicked
Shift-click to zoom out by a factor of 2 centred on the point clicked
Ctrl-drag to create a zoom box and zoom into the enclosed area
Ctrl-context-click to restore the original view

The patch isn't perfect but I have given it as much time as I can just now. At
least it should be enought to save anyone implementing a proper patch
considerable time on working out the algorithms. 

Any comments anyone has would be welcome.

Comment 23

[Alex Fritze]

I had a play with this and it's looking pretty good.

A few comments:
For several reasons it's probably a good idea to just have _one_ mousedown
handler and create mousemove/mouseup listeners dynamically like so:

<handler event="mousedown">

  var _this = this; // need "_this" to access binding obj through closure
                    // in event listeners below

  function moveListener(evt) { _this.doSomething(); }
  function mouseupListener(evt) { 
    _this.doSomething();
    _this.removeEventListener("mousemove", 
                              moveListener, true);
    _this.removeEventListener("mouseup", 
                              mouseupListener, true);
  }

  this.addEventListener("mousemove", mousemoveListener, true);
  this.addEventListener("mouseup", mouseupListener, true);
</handler>

This is more efficient, since we don't constantly have to process mousemoves.
Also, on my linux box (openwindows) I don't seem to get any mousemoves when
filtered by modifiers="accel" for some reason. 

Comment 24

[Jonathan Watt [:jwatt]]

I had briefly wondered about doing that but didn't because (a) I didn't know I
could do the _this thing (b) more to the point making those modifications will
prevent a zoom box from being started if the mouse button is already down and
the cursor is being dragged when the ctrl/accel button is pressed. I wasn't sure
how much overhead is caused by listening for all mousemove events with the ctrl
key pressed. If you think it is serious enough to remove functionality (b) let
me know and I will make the changes. 

Comment 25

[Alex Fritze]

(In reply to comment #24)
> I had briefly wondered about doing that but didn't because (a) I didn't know I
> could do the _this thing 

It's the magic of closures!

> (b) more to the point making those modifications will
> prevent a zoom box from being started if the mouse button is already down and
> the cursor is being dragged when the ctrl/accel button is pressed. 

Are you sure that we actually want this logic? I know that under Windows a
Drag&Drop action can be modified to 'copy' or 'link' by pressing Ctrl/Alt during
a drag, but that seems kind-of a special case to me. Shouldn't the modifier
usually be pressed before initiating operations like zoom-box dragging?

> I wasn't sure
> how much overhead is caused by listening for all mousemove events with the 
> ctrl key pressed. If you think it is serious enough to remove functionality 
> (b) let me know and I will make the changes. 

I don't know how serious the overhead is - the more serious problem that I am
encountering is that my windowmanager (openwindows) doesn't seem to give me any
modifiers for mousemoves. I don't know whether this is a peculiarity of
OpenWindows or a general Linux thing. 

Another comment:
>  <binding id="svg">
>
>    <!-- Add anonymous 'rect' element to act as a visible zoom box -->
>
>    <content>
>      <svg:g> 
>        <svg:g transform="scale(1)" xbl:inherits="style fill fill-opacity 
> fill-rule stroke stroke-dashoffset stroke-linecap stroke-linejoin 
> stroke-miterlimit stroke-opacity stroke-width clip-path clip-rule cursor 
> display filter image-rendering mask opacity pointer-events shape-rendering 
> text-rendering visibility alignment-baseline baseline-shift direction 
> dominant-baseline glyph-orientation-horizontal glyph-orientation-vertical 
> kerning letter-spacing text-anchor text-decoration unicode-bidi word-spacing 
> font-family font-size font-size-adjust font-stretch font-style font-variant 
> font-weight">
>          <children /> 
>        </svg:g>

You can completely remove the xbl:inherits attribute here, since the g-element
will automatically inherits all relevant attributes through normal CSS
cascading. (For the <a>-element the xbl:inherits is needed, because it binds
a generic xml element).

Comment 26

[Jonathan Watt [:jwatt]]

Attachment: svgBindings.xml v2

Changed to add event listeners dynamically as per c23 while still allowing the
modifier keys to be depressed after the mousedown. Also the modifiers can now
be released and the operation continued until mouseup. I have also removed the
xbl:inherits attribute as per c25 (infact that whole 'g' element), and the lack
of modifiers for mousemoves shouldn't be a problem anymore. Also the problems
when the document was scrolled are now fixed. The comments in the file should
provided further information. The attachment still needs work, but it is fully
functional and shows where I'm at so far.

Comment 27

[tor]

Attachment: c++ backend support for zoomAndPan/currentScale/currentTranslate

Comment 28

[Jonathan Watt [:jwatt]]

Comment on attachment 166983 [details] [diff] [review]
c++ backend support for zoomAndPan/currentScale/currentTranslate

r=jwatt with the folling issues addressed:

nsSVGSVGElement.cpp
-------------------

In Init() I'd prefer you scope currentScale and currentTranslate to be
consistant with the style. Also there is no need for the
NS_ADD_SVGVALUE_OBSERVER(mZoomAndPan) since AddMappedSVGValue does the
AddObserver() thing. In fact, we don't need it for mViewBox or
mPreserveAspectRatio either, so can you remove the "// add observers ---"
section and put the NS_ADD_SVGVALUE_OBSERVER(mCurrentScale) and
NS_ADD_SVGVALUE_OBSERVER(mCurrentTranslate) up after the
NS_ENSURE_SUCCESS(rv,rv) for their respective sections.

I'm wondering if nsSVGSVGElement::GetCurrentTranslate() shouldn't copy
mCurrentTranslate rather than returning it. Do we want to allow scripts to
change the translation by modifying the SVGPoint that is returned? Unless you
have a strong opinion either way can you add in a comment to the function to
the effect of "// returning mCurrentTranslate allows scripts access" or
something.

In SetZoomAndPan() we want to make sure that the argument is either
SVG_ZOOMANDPAN_DISABLE or SVG_ZOOMANDPAN_MAGNIFY. I suggest you change the body
to something like:

  return aZoomAndPan == nsIDOMSVGZoomAndPan::SVG_ZOOMANDPAN_DISABLE ||
	 aZoomAndPan == nsIDOMSVGZoomAndPan::SVG_ZOOMANDPAN_MAGNIFY ?
	 mZoomAndPan->SetIntegerValue(aZoomAndPan) :
	 NS_ERROR_DOM_SVG_INVALID_VALUE_ERR;

Actually the spec doesn't say what to return if the value isn't valid, but I'd
go with that.

nsSVGOuterSVGFrame.cpp
----------------------

Any reason for the number of spaces? Can you change:

  nsCOMPtr<nsISVGEnum>		    mZoomAndPan;
  nsCOMPtr<nsIDOMSVGPoint>	    mCurrentTranslate;
  nsCOMPtr<nsIDOMSVGNumber>	    mCurrentScale;

to:

  nsCOMPtr<nsISVGEnum>	    mZoomAndPan;
  nsCOMPtr<nsIDOMSVGPoint>  mCurrentTranslate;
  nsCOMPtr<nsIDOMSVGNumber> mCurrentScale;

Also, you can get rid of 'fini' by changing:

      nsCOMPtr<nsIDOMSVGMatrix> tmp, fini;
      mCanvasTM->Translate(x, y, getter_AddRefs(tmp));
      tmp->Scale(scale, getter_AddRefs(fini));
      mCanvasTM = fini;

to:

      nsCOMPtr<nsIDOMSVGMatrix> tmp;
      mCanvasTM->Translate(x, y, getter_AddRefs(tmp));
      tmp->Scale(scale, getter_AddRefs(mCanvasTM));

Comment 29

[tor]

Backend code committed with suggested changes.

Comment 30

[Jonathan Watt [:jwatt]]

Not to self: that was
http://bonsai.mozilla.org/cvsquery.cgi?who=tor%25cs.brown.edu&date=explicit&mindate=2004-12-02+15%3A13&maxdate=2004-12-02+15%3A13

Comment 31

[Jonathan Watt [:jwatt]]

Okay, now that bug 38447 is fixed I plan to move on this again. Somehow that bug
was no longer in the dependency list, so I'm adding it and bug 286388. Since I
intend to implement this in C++ I'm also removing the dependencies on bug 71191
and bug 172574.

Comment 32

[Holger Will]

from a personal email with jonathan, i know this bug will take another while to
be fixed, for the mean time,i ve written a little extension which can serve as a
frontend(only works with DeerPark).
http://www.treebuilder.de/zoomandpan/index.htm

Comment 33

[Jonathan Watt [:jwatt]]

There is opposition to including (ASV style?) zoom and pan controls in Firefox,
so even if I were to finish this in time it seems unlikely that the patch would
be accepted. It looks like an extension is going to be the way to go for Firefox
1.5, but we'll still need to fix bug 302103 for such an extension to function
correctly.

Comment 34

[Adam Guthrie]

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

Comment 35

[Jesse Ruderman]

The UI proposed in this bug is unnecessarily inconsistent with the rest of Firefox.  I think it would be better to support Ctrl++ / Ctrl+- for zooming SVG and scrollbars for panning.

Comment 36

[Jonathan Watt [:jwatt]]

I still maintain that infinite canvas means infinite panning, which would be precluded by using scrollbars as the panning mechanism.

Comment 37

[Jesse Ruderman]

SVG that extends infinitely is rare, so let's not screw up the UI for the common case just to accommodate those documents.

Btw, MSVC's solution to the "infinite scrolling" problem is to show a scrollbar with a small thumb, centered within the scrollbar.  The thumb never moves, but you can scroll by a page at a time using the normal methods.

Comment 38

[Jonathan Watt [:jwatt]]

I'll leave debating whether SVG that extends infinitely is rare for another day, but that MSVC uses that solution is interesting because I seem to remember I got pushback when I proposed the same idea.

Comment 40

[SamyDagan]

I've just stumbled on this bug report. For anyone interested I have a solution of Zoom and Pan controls for SVG with width and height of 100%, based on ecma-scripting, which is based on the original idea of Jonathan WAtt.
It is open source and anyone interested can copy, modify and use it. 
It is found at "http://alzt.tau.ac.il/~dagan/tools/"
Look for the "Zoom and Pan" title on the bottom. Cheers, Samy