Title says it all.
Or could this be the intended behavior?
Yomi | |
Apr 1 2014, 7:07 PM |
F141348: Screen_Shot_2014-04-08_at_8.17.21_PM.png | |
Apr 9 2014, 3:31 AM |
F141351: Screen_Shot_2014-04-08_at_8.18.23_PM.png | |
Apr 9 2014, 3:31 AM |
Title says it all.
Or could this be the intended behavior?
This is not intended, but kind of a pain to fix, and argubably not having tooltips is less distracting?
The easiest fix for this is probably for tips to check if they'll display off screen in their intended direction, and to flip directions if they will (i.e., "N" -> "S", or "E" -> "W"). I think that will never give us worse results, and won't require goofy special-casing around these tips.
First, find a comment box. An easy way to find one is to go to Maniphest, then Create New Task, then create a test task, then scroll to the bottom of the page.
In the comment box, if you hover over the "B", "I", "T", etc., icons above a comment box, you get tooltip popups saying "Bold", "Italic", etc. These appear above (vs beside/below) the icons, since they're internally specified to have "N" (north) alignment.
However, if you click the double-arrow icon on the right-hand side to expand the comment area to go to "Fullscreen Mode", the tooltips are no longer visible because they're off the top of the screen. This is the button in question:
To fix this in the general case, I think we should do this:
The goal is for the tips to appear in full screen mode, but show up below the buttons instead of above them.
(We could do other things, like special case the alignment for the tips in this case, but I think this general rule will improve all current/future cases of this problem and can't come up with any cases it makes worse.)
The code for rendering tooltips is in phabricator/webroot/rsrc/js/core/ToolTip.js, and the part that positions the tooltips is near line 50:
... // Append the tip to the document, but offscreen, so we can measure it. node.style.left = '-10000px'; document.body.appendChild(node); var p = JX.$V(root); var d = JX.Vector.getDim(root); var n = JX.Vector.getDim(node); // Move the tip so it's nicely aligned. switch (align) { case 'N': node.style.left = parseInt(p.x - ((n.x - d.x) / 2), 10) + 'px'; ...
I think you should be able to put some code in the middle (near the // Move the tip.. comment). For context:
You'll also probably need to make a call to JX.Vector.getViewport(). That will give you a vector with x and y that correspond to the scroll position in the document. This will help you figure out if the tip will be offscreen or not.
To get started, if you just add some code like p.x += 50 and reload the page, that should affect the tooltips.
Once you have a patch, run this script before sending a diff:
phabricator/ $ ./bin/celerity map
You should run that script before sending code for review if your patch includes static resource changes (CSS, JS, or images). You can find more information here:
This might be vaguely useful:
Small correction -- JX.Vector.getViewport() may be useful, but JX.Vector.getScroll() is the method I was actually talking about. The "Viewport" is the width and height of the browser window, while the "Scroll" is the scroll position within the document.
On the z-index thing: I can't find any record of explicitly having tried to hide the tips in fullscreen mode or other nonsense like that -- let's just move the z-index for tips to 51.