fsprojects / FsReveal

FsReveal parses markdown and F# script file and generates reveal.js slides.
http://fsprojects.github.io/FsReveal
258 stars 100 forks source link

F# tooltips position is off #71

Open vlaci opened 9 years ago

vlaci commented 9 years ago

On my notebook at 1366x768 resolution tooltips of all example presentation are displayed at the bootom edge of the screen and sometimes even off the screen. The problem is present in both IE11 and FF38 at normal zoom level. I fixed the issue for mysef by replacing the following call of getPos:

    var pos = findPos(owner ? owner : (evt.srcElement ? evt.srcElement : evt.target));
    var posx = pos[0];
    var posy = pos[1];

To:

    var posx = evt.clientX;
    var posy = evt.clientY;

I don't know how the findPos code should be better than just getting the coordinates directly from the event args.

Andrea commented 8 years ago

Having the same issue for a while now too , having this issue with chrome

isaacabraham commented 8 years ago

This is an issue for me in Firefox but seems to work ok in Chrome.

forki commented 8 years ago

If someone can do a pull request that would be awesome

rmunn commented 8 years ago

I plan to look into this when I have time, sometime in the next couple of weeks. If someone else beats me to it, though, it looks like getBoundingClientRect() might be the right answer, possibly with window.scrollX and window.scrollY added. See https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect for more details.

rmunn commented 8 years ago

I've looked at this a little, and the current code is doing exactly what getBoundingClientRect() would do: the tooltips are being set to a position that should be immediately below the "parent" element. However, reveal.js is doing something funny with zoom levels and scaling that's causing the abolutely-positioned elements to not be placed where they normally "should" be placed based on their coordinates. So the cause of this bug is an unintended interaction between the reveal.js code and the tooltip placement.

I haven't yet found the details of the interaction, so I don't yet have a solution to propose.

rmunn commented 8 years ago

The reason reveal.js is throwing things off appears to be that it's using 3D transforms to style its slide transitions (at least, if you use the most common transitions) and to scale the content to fit the browser window. Try it: load an FsReveal slide and try using the Ctrl-plus and Ctrl-minus shortcuts to zoom the browser window. You'll find that the slide text stays pretty much the same size, because reveal.js is using 3D transforms to scale to whatever size your viewport is.

So why does that matter? Well, it appears that getBoundingClientRect() and other similar methods (like the calculations that findPos() is currently doing) return positions that are calculated after the 3D transform are applied -- in other words, its actual position on the screen. But when you assign an element's top and left positions in pixels, those values are taken as representing its position before the current 3D transform is applied.

Both of these are normally what you would want, but here they're working against what we're trying to do. I found a detailed discussion of this phenomenon here:

https://www.bountysource.com/issues/1510871-initializing-inside-a-transformed-css-block-invalidates-codemirror-s-positioning-assumptions

This is a discussion between several CodeMirror devs who are trying to figure out how to use CodeMirror inside a CSS-transformed element. (CodeMirror does a lot of positioning various elements relative to each other.) The fifth comment in that discussion looks particularly useful: Nathan Hammond discusses using a "canary" element (as in "canary in the coal mine") to calculate what the transform matrix must be, by setting its position attributes to various values and observing what results. His idea appears to be that they could then apply that transform matrix (or its inverse) to switch between what I'll call "original" coordinates and "transformed" coordinates.

In that discussion thread, they eventually give up on that idea because the performance hit of constantly recalculating the positions of elements from a transform matrix would be too great. However, in the FsReveal context, we can afford to take a greater performance hit since we don't need to feel instantly responsive to keystrokes -- and tooltip-position calculations only happen when the user mouses over the code anyway, which isn't very often. So we could potentially rewrite the findPos() function to calculate the transform matrix that reveal.js is using, and apply that transform matrix in reverse to the values we get from getBoundingClientRect() in order to figure out what values to assign to the tooltip's left and top attributes.

If you're reading this and that made no sense at all to you, don't worry about it -- I'm mostly writing this down as notes to myself so that I can remember what I was thinking when I come back to this later. (I'm running out of spare time to work on it today).

But if you're reading this and this comment made perfect sense to you, then you probably understand CSS and 3D transforms better than I do (I'm a complete novice at 3D math), so _please_ feel free to tackle the problem yourself -- because your solution will probably be better than what I come up with.

rmunn commented 8 years ago

Actually, I believe I've found an answer that should work for every zoom level and screen size -- though it needs testing. Replace your tips.js with the following:

var currentTip = null;
var currentTipElement = null;

function hideTip(evt, name, unique) {
    var el = document.getElementById(name);
    el.style.display = "none";
    currentTip = null;
}

function hideUsingEsc(e) {
    if (!e) { e = event; }
    hideTip(e, currentTipElement, currentTip);
}

function showTip(evt, name, unique, owner) {
    document.onkeydown = hideUsingEsc;
    if (currentTip == unique) return;
    currentTip = unique;
    currentTipElement = name;

    var target = owner ? owner : (evt.srcElement ? evt.srcElement : evt.target);
    var posx = target.offsetLeft;
    var posy = target.offsetTop + target.offsetHeight;

    var el = document.getElementById(name);
    var parent = target.offsetParent;
    el.style.position = "absolute";
    el.style.left = posx + "px";
    el.style.top = posy + "px";
    parent.appendChild(el);
    el.style.display = "block";
}

Notice how findPos() is gone. Instead, we can just calculate the X,Y offset of the span whose tooltip we're displaying. Then we find the .offsetParent of the span, which is the element relative to which the offset is calculated. Finally, we make the tooltip a child of that element so that we're calculating the tooltip's position in the exact same way as the span element's position is calculated.

Result: so far I've seen the tooltip show up precisely under its corresponding span, each time, every time.

HOWEVER: I still need to test one scenario. Looking at the CSS for reveal.js, it seems that if you select no transitions, 3D transforms are not used. When I tested the above Javascript against a "normal" F# page formatted with FSharp.Formatting, the parent.appendChild(el) line did not have the desired effect. When 3D transforms are not happening, position: absolute positions things relative to the document just like you would expect. So if someone has selected NO transforms for their presentation, I believe the code above would fail.

I will test this further, and then submit a proper PR.

In the meantime, anyone affected by this bug has a workaround: after you run build.sh, replace the tips.js file in your output/css/fsharp.formatting/styles folder with the code above and the tooltips should be properly positioned. Note that this will be overwritten the next time a build runs, so if you're still editing your presentation you'll have to apply this workaround multiple times.

If that's too much hassle for you, then just wait for the pull request. Once I fix this properly, it should Just Work™ without needing to edit files that build.sh is going to overwrite. PR should hopefully be coming in a couple of days.

rmunn commented 8 years ago

I've tested this, and it works properly with NO transitions as well. So here's my PR.

rmunn commented 8 years ago

This should be fixed now that FsReveal 1.3.1 is out. If @vlaci can confirm that it works, this issue can be closed.

You can tell if you have FsReveal 1.3.1 by looking at your paket.lock file in the root of your repo. If you see FsReveal 1.3 or earlier, run paket update to pull in the latest version of FsReveal, after which point your tooltip positions should now be correct.

pirrmann commented 7 years ago

I've also had this issue for a while and updating to 1.3.1 fixed it, I think this issue can be closed.