From 0e6da9fe7b873f8a3d576cd597498c451de36f62 Mon Sep 17 00:00:00 2001 From: Simon Lydell Date: Wed, 10 Sep 2014 21:01:04 +0200 Subject: [PATCH] Improve marker placement and fix related bug Before commit 89e1bc4 we used to recursively search the rect of an element until it found a non-covered point; after that commit we changed into only checking 6 simple locations of the element, for performance reasons. However, this meant two problems: - It was very confusing when markers ended up to the far right of elements. Sometimes it was difficult to understand which element the marker actually belonged to. Sometimes a tiny bit of the left side of an element was covered (possible by a _transparent_ element!) causing the marker to end up on the right instead, seemingly for no reason. - Some elements are overlapped by one pixel both on the left and right side (such as the github pagination links), causing no marker at all to appear. The solution is to use the best of both before the referenced commit and after it. Now we check 3 (instead of 6) simple places: Only the 3 on the left side. If the place is covered by another element, we try once to the right of that element. So in total there may be 6 attempts, just as before. Author: Simon Lydell Date: Sat Aug 2 16:01:36 2014 +0200 Improve marker generation performance `getFirstNonCoveredPoint` used to recursively search the rect of an element until it found a non-covered point. This works well most of the time, but on some sites (such as prisjakt.se) it took way too much time. Moreover, this technique required some CSS to be reset in a few special cases, which is very costly performance-wise. It is also brittle and makes the code unnecessarily complex. Lastly, there was a bug in the algorithm that caused uncaught exceptions sometimes (such as on youtube.com). Now we use a much simpler approach instead. `getFirstNonCoveredPoint` tries 6 different points of the element: +-------------------------------+ |1 left-top right-top 4| | | |2 left-middle right-middle 5| | | |3 left-bottom right-bottom 6| +-------------------------------+ If all of those are covered (or are reported as covered because of one of the CSS special cases we used to reset) then the whole element is simply considered to be covered. This seems to work really well. The above means that the markers can now be placed at any of the points in the above figure. The result is much faster, simpler and more robust. --- extension/packages/mode-hints/hints.coffee | 85 ++++++++++++--------- extension/packages/mode-hints/marker.coffee | 15 ++-- 2 files changed, 56 insertions(+), 44 deletions(-) diff --git a/extension/packages/mode-hints/hints.coffee b/extension/packages/mode-hints/hints.coffee index b477d97..35685a8 100644 --- a/extension/packages/mode-hints/hints.coffee +++ b/extension/packages/mode-hints/hints.coffee @@ -159,8 +159,8 @@ getElementShape = (window, element, viewport, parents) -> # Even if `element` has a visible rect, it might be covered by other elements. for visibleRect in visibleRects - nonCoveredPoint = getFirstNonCoveredPoint(window, element, visibleRect, - parents) + nonCoveredPoint = getFirstNonCoveredPoint(window, viewport, element, + visibleRect, parents) if nonCoveredPoint nonCoveredPoint.rect = visibleRect break @@ -200,7 +200,7 @@ adjustRectToViewport = (rect, viewport) -> } -getFirstNonCoveredPoint = (window, element, elementRect, parents) -> +getFirstNonCoveredPoint = (window, viewport, element, elementRect, parents) -> # Before we start we need to hack around a little problem. If `element` has # `border-radius`, the corners won’t really belong to `element`, so # `document.elementFromPoint()` will return whatever is behind. This will @@ -208,48 +208,63 @@ getFirstNonCoveredPoint = (window, element, elementRect, parents) -> # add a CSS class that removes `border-radius`. element.classList.add('VimFxNoBorderRadius') - tryPoint = (x, y) -> + tryPoint = (x, y, tryRight = 0) -> + elementAtPoint = window.document.elementFromPoint(x, y) + offset = {left: 0, top: 0} + found = false + # Ensure that `element`, or a child of `element` (anything inside an `` # is clickable too), really is present at (x,y). Note that this is not 100% # bullet proof: Combinations of CSS can cause this check to fail, even # though `element` isn’t covered. We don’t try to temporarily reset such CSS # (as with `border-radius`) because of performance. Instead we rely on that - # some of the 6 attempts below will work. - elementAtPoint = window.document.elementFromPoint(x, y) - return false unless element.contains(elementAtPoint) - # Note that `a.contains(a) == true`! - - # If we’re currently in a frame, there might be something on top of the - # frame that covers `element`. Therefore we ensure that the frame really is - # present at the point for each parent in `parents`. - currentWindow = window - offset = left: 0, top: 0 - for parent in parents by -1 - offset.left += parent.offset.left - offset.top += parent.offset.top - elementAtPoint = parent.window.document.elementFromPoint(offset.left + x, offset.top + y) - if elementAtPoint != currentWindow.frameElement - return false - currentWindow = parent.window - - return {x, y, offset} - - # Try the following 6 positions in order. If all of those are covered the - # whole element is considered to be covered. + # some of the attempts below will work. + if element.contains(elementAtPoint) # Note that `a.contains(a) == true`! + found = true + # If we’re currently in a frame, there might be something on top of the + # frame that covers `element`. Therefore we ensure that the frame really + # is present at the point for each parent in `parents`. + currentWindow = window + for parent in parents by -1 + offset.left += parent.offset.left + offset.top += parent.offset.top + elementAtPoint = parent.window.document.elementFromPoint( + offset.left + x, offset.top + y + ) + unless elementAtPoint == currentWindow.frameElement + found = false + break + currentWindow = parent.window + + if found + return {x, y, offset} + else + return false if elementAtPoint == null or tryRight == 0 + rect = elementAtPoint.getBoundingClientRect() + x = rect.right - offset.left + 1 + return false if x > viewport.right + return tryPoint(x, y, tryRight - 1) + + + # Try the following 3 positions, or immediately to the right of a covering + # element at one of those positions, in order. If all of those are covered the + # whole element is considered to be covered. The reasoning is: + # + # - A marker should show up as near the left edge of its visible area as + # possible. Having it appear to the far right (for example) is confusing. + # - We can’t try too many times because of performance. + # # +-------------------------------+ - # |1 left-top right-top 4| + # |1 left-top | # | | - # |2 left-middle right-middle 5| + # |2 left-middle | # | | - # |3 left-bottom right-bottom 6| + # |3 left-bottom | # +-------------------------------+ nonCoveredPoint = - tryPoint(elementRect.left, elementRect.top ) or - tryPoint(elementRect.left, elementRect.top + elementRect.height / 2) or - tryPoint(elementRect.left, elementRect.bottom ) or - tryPoint(elementRect.right, elementRect.top ) or - tryPoint(elementRect.right, elementRect.top + elementRect.height / 2) or - tryPoint(elementRect.right, elementRect.bottom ) + tryPoint(elementRect.left, elementRect.top , 1) or + tryPoint(elementRect.left, elementRect.top + elementRect.height / 2, 1) or + tryPoint(elementRect.left, elementRect.bottom , 1) element.classList.remove('VimFxNoBorderRadius') diff --git a/extension/packages/mode-hints/marker.coffee b/extension/packages/mode-hints/marker.coffee index a41288a..4399ece 100644 --- a/extension/packages/mode-hints/marker.coffee +++ b/extension/packages/mode-hints/marker.coffee @@ -32,18 +32,15 @@ class Marker setPosition: (viewport) -> { markerElement: { offsetHeight: height, offsetWidth: width } - elementShape: { nonCoveredPoint: { x, y, offset, rect } } + elementShape: { nonCoveredPoint: { x: left, y: top, offset, rect } } } = this - # Center the marker on the non-covered point. - left = x - width / 2 - top = y - height / 2 + # Center the marker vertically on the non-covered point. + top -= height / 2 - # Make sure that the marker stays within its element. - left = Math.min(left, rect.right - width) - top = Math.min(top, rect.bottom - height) - left = Math.max(left, rect.left) - top = Math.max(top, rect.top) + # Make sure that the marker stays within its element (vertically). + top = Math.min(top, rect.bottom - height) + top = Math.max(top, rect.top) # Make the position relative to the top frame. left += offset.left -- 2.39.3