From dbcd38aa881b090c17ca36a6b816a2c036fa03e6 Mon Sep 17 00:00:00 2001 From: Anh Nhan Nguyen Date: Sun, 7 Apr 2013 18:10:43 -0700 Subject: [PATCH] Fix the attempt of graceful alignment for Hovercards Summary: Refs T1048; Fixes T2902 - (Probably) fixes @vrana's issue. I managed to repro it on Ubuntu FF (though on Windows with 1.0GHz/512MB it's really worse...). This revises the approach to the graceful degradation for `too-far-to-the-screen-edge-edge-case`. I noticed that `x` could become very negative, up to about ~`-170px`. This is due to the //"already-on-the-left-side"// nature of object tags. `50 - 200 - 20` = `negative`. Adding `100px` (node.dimension.x / 4) to that won't really help, as the hovercard would still be offscreen. Instead, display them left-aligned with the object tags on the left edge per default, and offer centerization in center cases. This is also better for Pholio, Phriction, which have a way lower min-x than Maniphest, Differential. I also disabled placing the hovercard below the tag in case there's not enough space on the north side. The hovercard would not display 99.99% of the times after being hidden (and it retains the flickering behaviour). Another reason is also our current hide-behaviour, which only assumes north-side alignment. Adding south-side didn't really work (I'm bad with JS), so I didn't bother with it. Disabling this is //acceptable//, since it only really affects Pholio, Phriction. And nobody places object tags in the first line, anyway. Except for my test pages, of course :/ Btw, this also removes the weird jaggy horizontal shifts for object of various lengths (e.g. `{D4356}`, `{rP32ofhw0842obw}` etc.). I think that's only good. Test Plan: Hovered in Pholio, Phriction in Chrome, FF. Did not touch left screen border. Hovered in Maniphest, Differential. Tags farther to the left were aligned left, tags more in the center were pretty centered. Reviewers: epriestley, vrana Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T1048, T2902 Differential Revision: https://secure.phabricator.com/D5612 --- src/__celerity_resource_map__.php | 84 +++++++++---------- webroot/rsrc/js/application/core/Hovercard.js | 27 +++--- .../js/application/core/behavior-hovercard.js | 1 + 3 files changed, 58 insertions(+), 54 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 01e1f51f2f..3d415a3624 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -1905,7 +1905,7 @@ celerity_register_resource_map(array( ), 'javelin-behavior-phabricator-hovercards' => array( - 'uri' => '/res/65824840/rsrc/js/application/core/behavior-hovercard.js', + 'uri' => '/res/7c16603a/rsrc/js/application/core/behavior-hovercard.js', 'type' => 'js', 'requires' => array( @@ -2749,7 +2749,7 @@ celerity_register_resource_map(array( ), 'maniphest-task-edit-css' => array( - 'uri' => '/res/e9eddd31/rsrc/css/application/maniphest/task-edit.css', + 'uri' => '/res/0760f768/rsrc/css/application/maniphest/task-edit.css', 'type' => 'css', 'requires' => array( @@ -2911,7 +2911,7 @@ celerity_register_resource_map(array( ), 'phabricator-core-css' => array( - 'uri' => '/res/09a94677/rsrc/css/core/core.css', + 'uri' => '/res/169b95b3/rsrc/css/core/core.css', 'type' => 'css', 'requires' => array( @@ -3043,7 +3043,7 @@ celerity_register_resource_map(array( ), 'phabricator-hovercard' => array( - 'uri' => '/res/80f2fdb1/rsrc/js/application/core/Hovercard.js', + 'uri' => '/res/d70f1091/rsrc/js/application/core/Hovercard.js', 'type' => 'js', 'requires' => array( @@ -3900,7 +3900,7 @@ celerity_register_resource_map(array( ), array( 'packages' => array( - 'b5f1b8bd' => + '30e5f188' => array( 'name' => 'core.pkg.css', 'symbols' => @@ -3942,7 +3942,7 @@ celerity_register_resource_map(array( 34 => 'phabricator-object-item-list-view-css', 35 => 'global-drag-and-drop-css', ), - 'uri' => '/res/pkg/b5f1b8bd/core.pkg.css', + 'uri' => '/res/pkg/30e5f188/core.pkg.css', 'type' => 'css', ), 'd95b69e5' => @@ -4132,16 +4132,16 @@ celerity_register_resource_map(array( 'reverse' => array( 'aphront-attached-file-view-css' => '6b1fccc6', - 'aphront-dialog-view-css' => 'b5f1b8bd', - 'aphront-error-view-css' => 'b5f1b8bd', - 'aphront-form-view-css' => 'b5f1b8bd', - 'aphront-list-filter-view-css' => 'b5f1b8bd', - 'aphront-pager-view-css' => 'b5f1b8bd', - 'aphront-panel-view-css' => 'b5f1b8bd', - 'aphront-table-view-css' => 'b5f1b8bd', - 'aphront-tokenizer-control-css' => 'b5f1b8bd', - 'aphront-tooltip-css' => 'b5f1b8bd', - 'aphront-typeahead-control-css' => 'b5f1b8bd', + 'aphront-dialog-view-css' => '30e5f188', + 'aphront-error-view-css' => '30e5f188', + 'aphront-form-view-css' => '30e5f188', + 'aphront-list-filter-view-css' => '30e5f188', + 'aphront-pager-view-css' => '30e5f188', + 'aphront-panel-view-css' => '30e5f188', + 'aphront-table-view-css' => '30e5f188', + 'aphront-tokenizer-control-css' => '30e5f188', + 'aphront-tooltip-css' => '30e5f188', + 'aphront-typeahead-control-css' => '30e5f188', 'differential-changeset-view-css' => '8aaacd1b', 'differential-core-view-css' => '8aaacd1b', 'differential-inline-comment-editor' => 'e96b08f8', @@ -4155,7 +4155,7 @@ celerity_register_resource_map(array( 'differential-table-of-contents-css' => '8aaacd1b', 'diffusion-commit-view-css' => 'c8ce2d88', 'diffusion-icons-css' => 'c8ce2d88', - 'global-drag-and-drop-css' => 'b5f1b8bd', + 'global-drag-and-drop-css' => '30e5f188', 'inline-comment-summary-css' => '8aaacd1b', 'javelin-aphlict' => 'd95b69e5', 'javelin-behavior' => 'a2f94024', @@ -4227,47 +4227,47 @@ celerity_register_resource_map(array( 'javelin-util' => 'a2f94024', 'javelin-vector' => 'a2f94024', 'javelin-workflow' => 'a2f94024', - 'lightbox-attachment-css' => 'b5f1b8bd', + 'lightbox-attachment-css' => '30e5f188', 'maniphest-task-summary-css' => '6b1fccc6', 'maniphest-transaction-detail-css' => '6b1fccc6', 'phabricator-busy' => 'd95b69e5', 'phabricator-content-source-view-css' => '8aaacd1b', - 'phabricator-core-buttons-css' => 'b5f1b8bd', - 'phabricator-core-css' => 'b5f1b8bd', - 'phabricator-crumbs-view-css' => 'b5f1b8bd', - 'phabricator-directory-css' => 'b5f1b8bd', + 'phabricator-core-buttons-css' => '30e5f188', + 'phabricator-core-css' => '30e5f188', + 'phabricator-crumbs-view-css' => '30e5f188', + 'phabricator-directory-css' => '30e5f188', 'phabricator-drag-and-drop-file-upload' => 'e96b08f8', 'phabricator-dropdown-menu' => 'd95b69e5', 'phabricator-file-upload' => 'd95b69e5', - 'phabricator-filetree-view-css' => 'b5f1b8bd', - 'phabricator-flag-css' => 'b5f1b8bd', - 'phabricator-form-view-css' => 'b5f1b8bd', - 'phabricator-header-view-css' => 'b5f1b8bd', - 'phabricator-jump-nav' => 'b5f1b8bd', + 'phabricator-filetree-view-css' => '30e5f188', + 'phabricator-flag-css' => '30e5f188', + 'phabricator-form-view-css' => '30e5f188', + 'phabricator-header-view-css' => '30e5f188', + 'phabricator-jump-nav' => '30e5f188', 'phabricator-keyboard-shortcut' => 'd95b69e5', 'phabricator-keyboard-shortcut-manager' => 'd95b69e5', - 'phabricator-main-menu-view' => 'b5f1b8bd', + 'phabricator-main-menu-view' => '30e5f188', 'phabricator-menu-item' => 'd95b69e5', - 'phabricator-nav-view-css' => 'b5f1b8bd', + 'phabricator-nav-view-css' => '30e5f188', 'phabricator-notification' => 'd95b69e5', - 'phabricator-notification-css' => 'b5f1b8bd', - 'phabricator-notification-menu-css' => 'b5f1b8bd', - 'phabricator-object-item-list-view-css' => 'b5f1b8bd', + 'phabricator-notification-css' => '30e5f188', + 'phabricator-notification-menu-css' => '30e5f188', + 'phabricator-object-item-list-view-css' => '30e5f188', 'phabricator-object-selector-css' => '8aaacd1b', 'phabricator-prefab' => 'd95b69e5', 'phabricator-project-tag-css' => '6b1fccc6', - 'phabricator-remarkup-css' => 'b5f1b8bd', + 'phabricator-remarkup-css' => '30e5f188', 'phabricator-shaped-request' => 'e96b08f8', - 'phabricator-side-menu-view-css' => 'b5f1b8bd', - 'phabricator-standard-page-view' => 'b5f1b8bd', + 'phabricator-side-menu-view-css' => '30e5f188', + 'phabricator-standard-page-view' => '30e5f188', 'phabricator-textareautils' => 'd95b69e5', 'phabricator-tooltip' => 'd95b69e5', - 'phabricator-transaction-view-css' => 'b5f1b8bd', - 'phabricator-zindex-css' => 'b5f1b8bd', - 'sprite-apps-large-css' => 'b5f1b8bd', - 'sprite-gradient-css' => 'b5f1b8bd', - 'sprite-icon-css' => 'b5f1b8bd', - 'sprite-menu-css' => 'b5f1b8bd', - 'syntax-highlighting-css' => 'b5f1b8bd', + 'phabricator-transaction-view-css' => '30e5f188', + 'phabricator-zindex-css' => '30e5f188', + 'sprite-apps-large-css' => '30e5f188', + 'sprite-gradient-css' => '30e5f188', + 'sprite-icon-css' => '30e5f188', + 'sprite-menu-css' => '30e5f188', + 'syntax-highlighting-css' => '30e5f188', ), )); diff --git a/webroot/rsrc/js/application/core/Hovercard.js b/webroot/rsrc/js/application/core/Hovercard.js index 2bb018da01..ec1a5a0289 100644 --- a/webroot/rsrc/js/application/core/Hovercard.js +++ b/webroot/rsrc/js/application/core/Hovercard.js @@ -80,21 +80,24 @@ JX.install('Hovercard', { var n = JX.Vector.getDim(child); // Move the tip so it's nicely aligned. - // I'm just doing north alignment for now - // TODO: Gracefully align to the side in edge cases - // I know, hardcoded paddings... - var x = parseInt(p.x - ((n.x - d.x) / 2)) + 20; - var y = parseInt(p.y - n.y) - 20; + // I'm just doing north/south alignment for now + // TODO: Fix southern graceful align + var margin = 20; + // We can't shift left by ~$margin or more here due to Pholio, Phriction + var x = parseInt(p.x) - margin / 2; + var y = parseInt(p.y - n.y) - margin; - // Why use 4? Shouldn't it be just 2? - if (x < (n.x / 4)) { - x += (n.x / 4); + // If more in the center, we can safely center + if (x > (n.x / 2) + margin) { + x = parseInt(p.x - (n.x / 2) + d.x); } - if (y < n.y) { - // Place it at the bottom - y += n.y + d.y + 50; - } + // Temporarily disabled, since it gives weird results (you can only see + // a hovercard once, as soon as it's hidden, it cannot be shown again) + // if (y < n.y) { + // // Place it southern, since northern is not enough space + // y = p.y + d.y + margin; + // } node.style.left = x + 'px'; node.style.top = y + 'px'; diff --git a/webroot/rsrc/js/application/core/behavior-hovercard.js b/webroot/rsrc/js/application/core/behavior-hovercard.js index 7a12e78232..a9c97fc037 100644 --- a/webroot/rsrc/js/application/core/behavior-hovercard.js +++ b/webroot/rsrc/js/application/core/behavior-hovercard.js @@ -36,6 +36,7 @@ JX.behavior('phabricator-hovercards', function(config) { var root = JX.Hovercard.getAnchor(); var node = JX.Hovercard.getCard(); + // TODO: Add southern cases here, too var mouse = JX.$V(e); var node_pos = JX.$V(node); var node_dim = JX.Vector.getDim(node);