From 656bf9ce2527979d8eb7b9a70098b747898e3419 Mon Sep 17 00:00:00 2001 From: Simon Lydell Date: Sun, 3 Apr 2016 16:01:14 +0200 Subject: [PATCH] Improve Find mode search start position By default, Firefox starts searching after the end of the first selection range, or from the top of the page if there are no selection ranges. This means that if you search for "vim", a match for "vim" further up or down the page might be selected, even though the word "vim" might already be onscreen, because of the current selection (or lack thereof). Vim starts searching in a similar way: After the cursor. The difference is that in Vim, the cursor is _always_ onscreen. When you scroll, the cursor is "pushed along" by the edges of the screen. This way, it is always very easy to tell where you are going to start your search from. In Firefox, though, using the `/` command can be quite disorienting. Another Vim example: Let's say I searched for "vim" again (pressing `/vim` in Vim). I then go through a few matches using `n`. Then I come to a block of text that mentions "vim" _lots_ of times, and I realize that I'm not interested in that block at all. Then it is easier to scroll (or "jump") past that block rather than spamming `n` over and over (or trying to guess an appropriate count). When doing the same in Firefox, though, after having scrolled past the mentioned block of text, the next `n` keypress will scroll up right back to the last match and continue the search there. This is pretty annoying to me. While there's the Caret mode (which few people use) and the possibility to select text in Firefox, there's usually no concept of a caret, and the caret does not follow the scroll; it stays where it is, possibly offscreen. So where should we start searching from, really? This commit implements the following logic: If there's a visible selection on screen, start searching after that point. If not, start searching from the top of the current viewport. This works much more intuitively. Note: Elements with `position: fixed;` are excluded when determining "the top of the viewport". Inspired by vimium/issues#1471. --- documentation/options.md | 25 ++++++++ extension/lib/commands-frame.coffee | 46 +++++++++++++++ extension/lib/commands.coffee | 41 ++++++++----- extension/lib/defaults.coffee | 1 + extension/lib/utils.coffee | 12 ++++ extension/lib/viewport.coffee | 92 ++++++++++++++++++++++++++--- 6 files changed, 194 insertions(+), 23 deletions(-) diff --git a/documentation/options.md b/documentation/options.md index 8090ac7..0cacfa7 100644 --- a/documentation/options.md +++ b/documentation/options.md @@ -215,6 +215,31 @@ Controls whether [counts] are enabled or not. [counts]: commands.md#counts +### `find_from_top_of_viewport` + +Toggles whether the various find commands are Vim-style or Firefox +default-style. + +Disable this pref if you want `/` to work more like `` and `n`/`N` to work +more like ``/``. + +If there is selected text on the page, Firefox starts searching after that. +VimFx does so too, but only if the selection is currently _visible_ (inside the +current viewport). + +If there _isn’t_ selected text on the page, Firefox starts searching from the +top of the page. VimFx instead starts searching from the top of the current +viewport. + +The VimFx behavior is designed to be less disorienting. It is also similar to +how searching in Vim works. Again, you can return to the Firefox default +behavior (if you prefer that) by disabling this pref. + +One of the main benefits of the VimFx behavior is that you can scroll past a +block of the text with lots of search matches and then continue going through +matches with `n` after that block, without having to spam `n` lots and lots of +times. + ### `ignore_ctrl_alt` This option is enabled by default on Windows, and disabled otherwise. diff --git a/extension/lib/commands-frame.coffee b/extension/lib/commands-frame.coffee index 7d3f678..fb6113e 100644 --- a/extension/lib/commands-frame.coffee +++ b/extension/lib/commands-frame.coffee @@ -29,6 +29,7 @@ translate = require('./l10n') utils = require('./utils') viewportUtils = require('./viewport') +{FORWARD, BACKWARD} = SelectionManager {isProperLink, isTextInputElement, isTypingElement, isContentEditable} = utils XULDocument = Ci.nsIDOMXULDocument @@ -479,6 +480,51 @@ commands.move_focus = ({vim, direction}) -> utils.focusElement(nextInput, {select: true}) return true +# This is an attempt to enhance Firefox’s native “Find in page” functionality. +# Firefox starts searching after the end of the first selection range, or from +# the top of the page if there are no selection ranges. If there are frames, the +# top-most document in DOM order with selections seems to be used. +# +# Replace the current selection with one single range. (Searching clears the +# previous selection anyway.) That single range is either the first visible +# range, or a newly created (and collapsed) one at the top of the viewport. This +# way we can control where Firefox searches from. +commands.find_from_top_of_viewport = ({vim, direction}) -> + viewport = viewportUtils.getWindowViewport(vim.content) + + range = viewportUtils.getFirstVisibleRange(vim.content, viewport) + if range + window = range.startContainer.ownerGlobal + selection = window.getSelection() + utils.clearSelectionDeep(vim.content) + window.focus() + # When the next match is in another frame than the current selection (A), + # Firefox won’t clear that selection before making a match selection (B) in + # the other frame. When searching again, selection B is cleared because + # selection A appears further up the viewport. This causes us to search + # _again_ from selection A, rather than selection B. In effect, we get stuck + # re-selecting selection B over and over. Therefore, collapse the range + # first, in case Firefox doesn’t. + range.collapse() + selection.addRange(range) + # Collapsing the range causes backwards search to keep re-selecting the same + # match. Therefore, move it one character back. + selection.modify('move', 'backward', 'character') if direction == BACKWARD + return + + result = viewportUtils.getFirstVisibleText(vim.content, viewport) + return unless result + [textNode, offset] = result + + utils.clearSelectionDeep(vim.content) + window = textNode.ownerGlobal + window.focus() + range = window.document.createRange() + range.setStart(textNode, offset) + range.setEnd(textNode, offset) + selection = window.getSelection() + selection.addRange(range) + commands.esc = (args) -> {vim} = args commands.blur_active_element(args) diff --git a/extension/lib/commands.coffee b/extension/lib/commands.coffee index aafb279..0f7cfb8 100644 --- a/extension/lib/commands.coffee +++ b/extension/lib/commands.coffee @@ -32,9 +32,11 @@ hints = require('./hints') prefs = require('./prefs') translate = require('./l10n') utils = require('./utils') +SelectionManager = require('./selection') viewportUtils = require('./viewport') {ContentClick} = Cu.import('resource:///modules/ContentClick.jsm', {}) +{FORWARD, BACKWARD} = SelectionManager commands = {} @@ -632,6 +634,12 @@ commands.element_text_copy = ({vim}) -> findStorage = {lastSearchString: ''} +helper_find_from_top_of_viewport = (vim, direction, callback) -> + if vim.options.find_from_top_of_viewport + vim._run('find_from_top_of_viewport', {direction}, callback) + else + callback() + helper_find = ({highlight, linksOnly = false}, {vim}) -> helpSearchInput = help.getSearchInput(vim.window) if helpSearchInput @@ -639,16 +647,18 @@ helper_find = ({highlight, linksOnly = false}, {vim}) -> return helper_mark_last_scroll_position(vim) - findBar = vim.window.gBrowser.getFindBar() + helper_find_from_top_of_viewport(vim, FORWARD, -> + findBar = vim.window.gBrowser.getFindBar() - mode = if linksOnly then findBar.FIND_LINKS else findBar.FIND_NORMAL - findBar.startFind(mode) - utils.focusElement(findBar._findField, {select: true}) + mode = if linksOnly then findBar.FIND_LINKS else findBar.FIND_NORMAL + findBar.startFind(mode) + utils.focusElement(findBar._findField, {select: true}) - return if linksOnly - return unless highlightButton = findBar.getElement('highlight') - if highlightButton.checked != highlight - highlightButton.click() + return if linksOnly + return unless highlightButton = findBar.getElement('highlight') + if highlightButton.checked != highlight + highlightButton.click() + ) commands.find = helper_find.bind(null, {highlight: false}) @@ -661,15 +671,18 @@ helper_find_again = (direction, {vim}) -> if findStorage.lastSearchString.length == 0 vim.notify(translate('notification.find_again.none')) return + helper_mark_last_scroll_position(vim) - findBar._findField.value = findStorage.lastSearchString - findBar.onFindAgainCommand(direction) - message = findBar._findStatusDesc.textContent - vim.notify(message) if message + helper_find_from_top_of_viewport(vim, direction, -> + findBar._findField.value = findStorage.lastSearchString + findBar.onFindAgainCommand(not direction) + message = findBar._findStatusDesc.textContent + vim.notify(message) if message + ) -commands.find_next = helper_find_again.bind(null, false) +commands.find_next = helper_find_again.bind(null, FORWARD) -commands.find_previous = helper_find_again.bind(null, true) +commands.find_previous = helper_find_again.bind(null, BACKWARD) diff --git a/extension/lib/defaults.coffee b/extension/lib/defaults.coffee index 633028b..290facd 100644 --- a/extension/lib/defaults.coffee +++ b/extension/lib/defaults.coffee @@ -161,6 +161,7 @@ advanced_options = 'notify_entered_keys': true 'prevent_target_blank': true 'counts_enabled': true + 'find_from_top_of_viewport': true 'ignore_ctrl_alt': (Services.appinfo.OS == 'WINNT') 'prevent_autofocus_modes': 'normal' 'config_file_directory': '' diff --git a/extension/lib/utils.coffee b/extension/lib/utils.coffee index 6ef71cd..6195beb 100644 --- a/extension/lib/utils.coffee +++ b/extension/lib/utils.coffee @@ -316,6 +316,13 @@ simulateMouseEvents = (element, sequence) -> area = (element) -> return element.clientWidth * element.clientHeight +checkElementOrAncestor = (element, fn) -> + window = element.ownerGlobal + while element.parentElement + return true if fn(element) + element = element.parentElement + return false + clearSelectionDeep = (window) -> # The selection might be `null` in hidden frames. selection = window.getSelection() @@ -369,6 +376,9 @@ insertText = (input, value) -> isDetached = (element) -> return not element.ownerDocument?.documentElement?.contains?(element) +isNonEmptyTextNode = (node) -> + return node.nodeType == 3 and node.data.trim() != '' + isPositionFixed = (element) -> computedStyle = element.ownerGlobal.getComputedStyle(element) return computedStyle?.getPropertyValue('position') == 'fixed' @@ -566,6 +576,7 @@ module.exports = { simulateMouseEvents area + checkElementOrAncestor clearSelectionDeep containsDeep createBox @@ -573,6 +584,7 @@ module.exports = { injectTemporaryPopup insertText isDetached + isNonEmptyTextNode isPositionFixed querySelectorAllDeep setAttributes diff --git a/extension/lib/viewport.coffee b/extension/lib/viewport.coffee index 699f1a6..bbcb861 100644 --- a/extension/lib/viewport.coffee +++ b/extension/lib/viewport.coffee @@ -46,6 +46,53 @@ adjustRectToViewport = (rect, viewport) -> height, width, area } +getAllRangesInsideViewport = (window, viewport, offset = {left: 0, top: 0}) -> + selection = window.getSelection() + {rangeCount} = selection + ranges = [] + + if rangeCount > 0 + {header, headerBottom, footer, footerTop} = + getFixedHeaderAndFooter(window, viewport) + + # Many sites use `text-indent: -9999px;` or similar to hide text intended + # for screen readers only. That text can still be selected and found by + # searching, though. Therefore, we have to allow selection ranges that are + # within the viewport vertically but not horizontally, even though they + # actually are outside the viewport. Otherwise you won’t be able to press + # `n` to get past those elements (instead, `n` would start over from the top + # of the viewport). + for index in [0...rangeCount] + range = selection.getRangeAt(index) + continue if range.collapsed + rect = range.getBoundingClientRect() + if (rect.top >= headerBottom - MINIMUM_EDGE_DISTANCE and + rect.bottom <= footerTop + MINIMUM_EDGE_DISTANCE) or + header.contains(range.commonAncestorContainer) or + footer.contains(range.commonAncestorContainer) + adjustedRect = { + left: offset.left + rect.left + top: offset.top + rect.top + right: offset.left + rect.right + bottom: offset.top + rect.bottom + width: rect.width + height: rect.height + } + ranges.push({range, rect: adjustedRect}) + + for frame in window.frames + {viewport: frameViewport, offset: frameOffset} = + getFrameViewport(frame.frameElement, viewport) ? {} + continue unless frameViewport + newOffset = { + left: offset.left + frameOffset.left + top: offset.top + frameOffset.top + } + frameRanges = getAllRangesInsideViewport(frame, frameViewport, newOffset) + ranges.push(frameRanges...) + + return ranges + getFirstNonWhitespace = (element) -> window = element.ownerGlobal viewport = getWindowViewport(window) @@ -63,9 +110,7 @@ getFirstNonWhitespace = (element) -> getFirstVisibleOffset = (textNode, viewport) -> {length} = textNode.data return null if length == 0 - [header] = getFixedHeaderAndFooter(textNode.ownerGlobal, viewport) - headerBottom = - if header then header.getBoundingClientRect().bottom else viewport.top + {headerBottom} = getFixedHeaderAndFooter(textNode.ownerGlobal, viewport) [nonMatch, match] = utils.bisect(0, length - 1, (offset) -> range = textNode.ownerDocument.createRange() # Using a zero-width range sometimes gives a bad rect, so make it span one @@ -80,6 +125,36 @@ getFirstVisibleOffset = (textNode, viewport) -> ) return match +getFirstVisibleRange = (window, viewport) -> + ranges = getAllRangesInsideViewport(window, viewport) + first = null + for item in ranges + if not first or item.rect.top < first.rect.top + first = item + return if first then first.range else null + +getFirstVisibleText = (window, viewport) -> + for element in window.document.getElementsByTagName('*') + rect = element.getBoundingClientRect() + continue unless isInsideViewport(rect, viewport) + + if element.contentWindow and + not utils.checkElementOrAncestor(element, utils.isPositionFixed) + {viewport: frameViewport} = getFrameViewport(element, viewport) ? {} + continue unless frameViewport + result = getFirstVisibleText(element.contentWindow, frameViewport) + return result if result + continue + + continue unless Array.some(element.childNodes, utils.isNonEmptyTextNode) + + continue if utils.checkElementOrAncestor(element, utils.isPositionFixed) + + result = getFirstNonWhitespace(element) + return result if result + + return null + # Adapted from Firefox’s source code for `` scrolling (which is where the # arbitrary constants below come from). # @@ -116,7 +191,7 @@ getFixedHeaderAndFooter = (window) -> footer = candidate footerTop = rect.top - return [header, footer] + return {header, headerBottom, footer, footerTop} getFrameViewport = (frame, parentViewport) -> rect = frame.getBoundingClientRect() @@ -161,11 +236,7 @@ getFrameViewport = (frame, parentViewport) -> getViewportCappedClientHeight = (element) -> window = element.ownerGlobal viewport = getWindowViewport(window) - [header, footer] = getFixedHeaderAndFooter(window) - headerBottom = - if header then header.getBoundingClientRect().bottom else viewport.top - footerTop = - if footer then footer.getBoundingClientRect().bottom else viewport.bottom + {headerBottom, footerTop} = getFixedHeaderAndFooter(window) return Math.min(element.clientHeight, footerTop - headerBottom) getWindowViewport = (window) -> @@ -217,8 +288,11 @@ scroll = (element, args) -> module.exports = { MINIMUM_EDGE_DISTANCE adjustRectToViewport + getAllRangesInsideViewport getFirstNonWhitespace getFirstVisibleOffset + getFirstVisibleRange + getFirstVisibleText getFixedHeaderAndFooter getFrameViewport getViewportCappedClientHeight -- 2.39.3