Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chrome 32/33, SVG output: zoom box not covering expression #749

Closed
pkra opened this issue Feb 24, 2014 · 17 comments

Comments

Projects
None yet
3 participants
@pkra
Copy link
Member

commented Feb 24, 2014

Example http://codepen.io/pkra/full/FpzwL -- just click an equation.

@dpvc dpvc added this to the Bugfix Version milestone Mar 4, 2014

@dpvc dpvc added Accepted labels Mar 4, 2014

@dpvc

This comment has been minimized.

Copy link
Member

commented Mar 4, 2014

Looks like a WebKit bug (it also affects Safari). Apparently the size of the SVG element is not contributing properly to the enclosing element. I'll see what I can do.

@Klortho

This comment has been minimized.

Copy link

commented Mar 18, 2014

Here is my attempt to address this issue: https://github.com/Klortho/MathJax/blob/0e5443d0a68726e2bf628be6d1dde0a1e30819ea/unpacked/extensions/MathZoom.js#L263-267. Please let me know if there are problems with this.

@dpvc

This comment has been minimized.

Copy link
Member

commented Mar 18, 2014

@Klortho, your solution is a little too aggressive, as it is possible for the equation to be to big to fit on screen, and the zoom window then will be (correctly) smaller than its contents. You don't want to expand it in that case. So just checking if the zoom size is smaller than the sag size is not quite sophisticated enough.

I'm in the process of updating the zoom code, and will be looking at this when I do.

Klortho added a commit to Klortho/MathJax that referenced this issue Mar 19, 2014

@Klortho

This comment has been minimized.

Copy link

commented Mar 19, 2014

You are right (of course). I updated it to be a little bit more sophisticated, but now it might be a bit brittle (not sure): https://github.com/Klortho/MathJax/blob/pmc-19668/unpacked/extensions/MathZoom.js#L206-217

Anyway, I know you will probably want to fix this in your own way. We are getting ready to do a new release of PubReader, which uses SVG output, so I'll be including my patch in that, since it's better than nothing.

I also wanted to give you a heads-up that I have a hacky fix for the issue described in an old forum thread Zoom window being cut off by margins within the body. I suppose I will open a pull request for it, although I don't expect you'll want to pull my changes -- nevertheless it's a good way to describe the feature request and show you what I've done.

@dpvc

This comment has been minimized.

Copy link
Member

commented Mar 19, 2014

An interesting idea. Note, however, that older versions of IE do not support getComputedStyle (IE8 and below), and so this will fail for them (and may cause the zooming to fail to set up completely). You might want to put

if (MathJax.Hub.Browser.isChrome) {
...
}

around your check.

@dpvc

This comment has been minimized.

Copy link
Member

commented Mar 19, 2014

I already have the "zoom window cut off" error in my sights, so it is not really necessary, but if you want to add your approach to the record, that is fine.

@Klortho

This comment has been minimized.

Copy link

commented Mar 30, 2014

Here are the changes that I made: https://github.com/Klortho/MathJax/pull/1/files. I took Math_ZoomFrame out of the content, and instead make it a child of #MathJax_ZoomWrapper, which can be pre-existing in the HTML page, or will be auto-created by default as the last child of . I then position everything absolutely, adjusting the position and size so that it always fits in the viewport.

It was really a bear, and if I had to do it over again, I wouldn't. I am not a CSS expert, and exercises like this really make me admire you guys who do such good work in it. I tested it in the latest versions of Chrome, FF, and IE, but I suspect it might have problems in earlier versions.

Anyway, maybe you can use it for ideas. Cheers!

Oh, yeah, and, it depends on jQuery.

@dpvc

This comment has been minimized.

Copy link
Member

commented Mar 31, 2014

Thanks for your update, and the work you have put into it. I know this sort of thing is very challenging to get to work reliably in all browsers.

There are a couple of issues with your approach. First, as you point out, it uses jQuery, and that is not a dependency that we are willing to add to MathJax at this point. Second, the reason that the Zoom box is placed at the location of the math itself is so that it will have the same context as the math as far as CSS is concerned. That way, the zoomed math will be affected by CSS in the same way (or a close to the same way as we can provide) as the original. For example, things like color and font size affect the original equation, and those affects will be lost in the zoomed version if you move the zoom box to another location. Similarly, when mtextInheritFont is in effect, <mtext> elements should use the surrounding font, and that may not be the same as the font at the new location of the zoom box. Finally, MathJax does allow CSS to style the various math elements directly (via .MathJax .mo, etc), and those CSS directives may be context dependent (e.g., .question .MathJax .mo or td .MathJax .mo). Such context-dependent directives will not be properly zoomed using your approach.

So while your technique may fix one problem, it introduces others, so I doubt we will add it into MathJax. But it is good to see that you could get it to work. Thanks for contributing.

@Klortho

This comment has been minimized.

Copy link

commented Mar 31, 2014

Yes, I know you wouldn't want the jQuery dependency, of course.

... things like color and font size affect the original equation, and those affects will be lost in the zoomed version ... not be properly zoomed using your approach.

This is a good thing, IMO. In our application, and I suspect in many others, we want the zoom box to have a consistent look and feel independent of where (article body vs. figure caption, for example) the equation appears.

The main problem I tried to address was the clipping. I still don't see any easy way to allow the zoom box to take up the whole viewport if it is a child of an element that is restricted to just a part of it. So, I hope you can keep that use case in mind. I'll look forward to seeing what you come up with!

@dpvc

This comment has been minimized.

Copy link
Member

commented Mar 31, 2014

Yes, the clipping is a problem, and my plan is to locate the closest parent object with clipping and restrict the zoom box to the width of that. This seems the best compromise between using as much space for zooming as you can and keeping the rendering in line with that of the original. If pages use restrictive elements, that is a design decision, and it makes sense for MathJax to respect them. Also, in some situations (e.g., the use of a iframe), there would be no choice but to be restricted.

@dpvc

This comment has been minimized.

Copy link
Member

commented Mar 31, 2014

This is a good thing, IMO. ... we want the zoom box to have a consistent look and feel

The box has a consistent look and feel, but its content is what I'm talking about. The content should reflect the original equation. If it has an element that has a color change due to CSS rules, so should the zoomed version. If that CSS is context-specific, and you zoom the equation in a different location, then that coloring would not be present (the coloring is done by the CSS, not MathJax directly). That is what I mean by making sure the zoomed math matches the original math as closely as possible.

@pkra

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2014

@Klortho you wrote

we want the zoom box to have a consistent look and feel independent of where (article body vs. figure caption, for example) the equation appears.

This sounds to me more like knowl's or what Lens does for figures/references. Perhaps the zoom functionality is not the right tool and a different (customizable) extensions would fit better?

@Klortho

This comment has been minimized.

Copy link

commented Mar 31, 2014

knowl's or what Lens does for figures/references

I'm not familiar with those. If you mean something like a lightbox, then yes. I guess that's what I've been thinking all along -- the zoom box, in my mind, shouldn't necessarily be a strict "zoom". For instance, I would think it would be better not to do line breaking, even if there is line breaking in the original, but rather to let the scroll bars handle overflow. The zoom box should let the user get the optimal view of the equation, whereas what's scrunched into the content might not be optimal at all.

@pkra

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2014

If you mean something like a lightbox, then yes. I guess that's what I've been thinking all along
-- the zoom box, in my mind, should necessarily be a strict "zoom".

Did you mean "should not"? Either way, a lightbox extension sounds like a nice idea. Would you be willing to try writing one?

Since I forgot the links earlier: Lens and knowl.js. There's also a JQueryUI Tooltip example.

@Klortho

This comment has been minimized.

Copy link

commented Apr 1, 2014

Did you mean "should not"?

Fixed.

Would you be willing to try writing one?

I am afraid I don't think I will have time. It would be fun to try, but I have too many other things going on right now.

@dpvc

This comment has been minimized.

Copy link
Member

commented Apr 11, 2014

=> Merged.

@dpvc dpvc closed this Apr 11, 2014

@dpvc

This comment has been minimized.

Copy link
Member

commented Apr 22, 2014

=> In Test Suite.

UI/Zoom/issue460.html (this picks up this error as well).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.