From f49d4b2f5980d48e76fd2e32491c9793f5f6fdf8 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Wed, 16 Mar 2016 12:09:25 +0000 Subject: Make scrollable elements selectable with hints. Fixes #425. Conflicts: content_scripts/scroller.coffee --- content_scripts/link_hints.coffee | 25 +++++++++++++++++-------- content_scripts/scroller.coffee | 13 +++++++++---- 2 files changed, 26 insertions(+), 12 deletions(-) (limited to 'content_scripts') diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee index 3088812b..994d99b5 100644 --- a/content_scripts/link_hints.coffee +++ b/content_scripts/link_hints.coffee @@ -190,6 +190,7 @@ LocalHints = isClickable = false onlyHasTabIndex = false visibleElements = [] + reason = null # Insert area elements that provide click functionality to an img. if tagName == "img" @@ -254,8 +255,15 @@ LocalHints = when "label" isClickable ||= element.control? and (@getVisibleClickable element.control).length == 0 when "body" - isClickable ||= element == document.body and not document.hasFocus() and - window.innerWidth > 3 and window.innerHeight > 3 + isClickable ||= + if element == document.body and not document.hasFocus() and + window.innerWidth > 3 and window.innerHeight > 3 and + document.body?.tagName.toLowerCase() != "frameset" + reason = "Frame." + when "div", "ol", "ul" + isClickable ||= + if Scroller.isScrollableElement element + reason = "Scroll." # Elements with tabindex are sometimes useful, but usually not. We can treat them as second class # citizens when it improves UX, so take special note of them. @@ -267,7 +275,7 @@ LocalHints = if isClickable clientRect = DomUtils.getVisibleClientRect element, true if clientRect != null - visibleElements.push {element: element, rect: clientRect, secondClassCitizen: onlyHasTabIndex} + visibleElements.push {element: element, rect: clientRect, secondClassCitizen: onlyHasTabIndex, reason} visibleElements @@ -313,7 +321,7 @@ LocalHints = # Subtract negativeRect from every rect in rects, and concatenate the arrays of rects that result. rects = [].concat (rects.map (rect) -> Rect.subtract rect, negativeRect)... if rects.length > 0 - nonOverlappingElements.push {element: visibleElement.element, rect: rects[0]} + nonOverlappingElements.push extend visibleElement, rect: rects[0] else # Every part of the element is covered by some other element, so just insert the whole element's # rect. Except for elements with tabIndex set (second class citizens); these are often more trouble @@ -325,7 +333,7 @@ LocalHints = hint.hasHref = hint.element.href? for hint in localHints if Settings.get "filterLinkHints" @withLabelMap (labelMap) => - extend hint, @generateLinkText labelMap, hint.element for hint in localHints + extend hint, @generateLinkText labelMap, hint for hint in localHints localHints # Generate a map of input element => label text, call a callback with it. @@ -342,7 +350,8 @@ LocalHints = labelMap[forElement] = labelText callback labelMap - generateLinkText: (labelMap, element) -> + generateLinkText: (labelMap, hint) -> + element = hint.element linkText = "" showLinkText = false # toLowerCase is necessary as html documents return "IMG" and xhtml documents return "img" @@ -362,8 +371,8 @@ LocalHints = element.firstElementChild.nodeName.toLowerCase() == "img" linkText = element.firstElementChild.alt || element.firstElementChild.title showLinkText = true if linkText - else if element == document.body - linkText = "Frame." + else if hint.reason? + linkText = hint.reason showLinkText = true else linkText = (element.textContent.trim() || element.innerHTML.trim())[...512] diff --git a/content_scripts/scroller.coffee b/content_scripts/scroller.coffee index f83d0e87..7b46bab0 100644 --- a/content_scripts/scroller.coffee +++ b/content_scripts/scroller.coffee @@ -77,12 +77,14 @@ doesScroll = (element, direction, amount, factor) -> delta = getSign delta # 1 or -1 performScroll(element, direction, delta) and performScroll(element, direction, -delta) +isScrollableElement = (element, direction = "y", amount = 1, factor = 1) -> + doesScroll(element, direction, amount, factor) and shouldScroll element, direction + # From element and its parents, find the first which we should scroll and which does scroll. findScrollableElement = (element, direction, amount, factor) -> - while element != document.body and - not (doesScroll(element, direction, amount, factor) and shouldScroll(element, direction)) - element = (DomUtils.getContainingElement element) ? document.body - if element == document.body then firstScrollableElement element else element + while element != document.body and not isScrollableElement element, direction, amount, factor + element = DomUtils.getContainingElement(element) ? document.body + element # On some pages, document.body is not scrollable. Here, we search the document for the largest visible # element which does scroll vertically. This is used to initialize activatedElement. See #1358. @@ -257,6 +259,9 @@ Scroller = amount = getDimension(element,direction,pos) - element[scrollProperties[direction].axisName] CoreScroller.scroll element, direction, amount + isScrollableElement: (element) -> + isScrollableElement element + # Scroll the top, bottom, left and right of element into view. The is used by visual mode to ensure the # focus remains visible. scrollIntoView: (element) -> -- cgit v1.2.3 From d2691a43cc97a89c4d127ef6ef0be4c0417c675d Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Thu, 17 Mar 2016 10:37:11 +0000 Subject: Filter out link-hint false positives. We recognise elements with a class names containing the text "button" as clickable. However, often they're not, they're just wrapping a clickable thing, like a real button. Here, we filter out such false positives. This has two effects: - It eliminates quite a number of real false pasitives in practice. - With fewer hints close together, fewer hint markers are obscured by the hints from (non-clickable) wrappers. This reduces the need for rotating the hint stacking order, e.g #1252. --- content_scripts/link_hints.coffee | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) (limited to 'content_scripts') diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee index 994d99b5..f464f159 100644 --- a/content_scripts/link_hints.coffee +++ b/content_scripts/link_hints.coffee @@ -189,6 +189,7 @@ LocalHints = tagName = element.tagName.toLowerCase() isClickable = false onlyHasTabIndex = false + possibleFalsePositive = false visibleElements = [] reason = null @@ -229,7 +230,6 @@ LocalHints = # Check for attributes that make an element clickable regardless of its tagName. if (element.hasAttribute("onclick") or element.getAttribute("role")?.toLowerCase() in ["button", "link"] or - element.getAttribute("class")?.toLowerCase().indexOf("button") >= 0 or element.getAttribute("contentEditable")?.toLowerCase() in ["", "contentEditable", "true"]) isClickable = true @@ -265,6 +265,12 @@ LocalHints = if Scroller.isScrollableElement element reason = "Scroll." + # An element with a class name containing the text "button" might be clickable. However, real clickables + # are often wrapped in elements with such class names. So, when we find clickables based only on their + # class name, we mark them as unreliable. + if not isClickable and 0 <= element.getAttribute("class")?.toLowerCase().indexOf "button" + possibleFalsePositive = isClickable = true + # Elements with tabindex are sometimes useful, but usually not. We can treat them as second class # citizens when it improves UX, so take special note of them. tabIndexValue = element.getAttribute("tabindex") @@ -275,7 +281,8 @@ LocalHints = if isClickable clientRect = DomUtils.getVisibleClientRect element, true if clientRect != null - visibleElements.push {element: element, rect: clientRect, secondClassCitizen: onlyHasTabIndex, reason} + visibleElements.push {element: element, rect: clientRect, secondClassCitizen: onlyHasTabIndex, + possibleFalsePositive, reason} visibleElements @@ -299,6 +306,27 @@ LocalHints = visibleElement = @getVisibleClickable element visibleElements.push visibleElement... + # Traverse the DOM from descendants to ancestors, so later elements show above earlier elements. + visibleElements = visibleElements.reverse() + + # Filter out suspected false positives. A false positive is taken to be an element marked as a possible + # false positive for which a close descendant is already clickable. False positives tend to be close + # together in the DOM, so - to keep the cost down - we only search nearby elements. NOTE(smblott): The + # visible elements have already been reversed, so we're visiting descendants before their ancestors. + descendantsToCheck = [1..3] # This determines how many descendants we're willing to consider. + visibleElements = + for element, position in visibleElements + continue if element.possibleFalsePositive and do -> + index = Math.max 0, position - 6 # This determines how far back we're willing to look. + while index < position + candidateDescendant = visibleElements[index].element + for _ in descendantsToCheck + return true if candidateDescendant == element.element + candidateDescendant = candidateDescendant?.parentElement + index += 1 + false # This is not a false positive. + element + # TODO(mrmr1993): Consider z-index. z-index affects behviour as follows: # * The document has a local stacking context. # * An element with z-index specified @@ -313,8 +341,6 @@ LocalHints = # # Remove rects from elements where another clickable element lies above it. localHints = nonOverlappingElements = [] - # Traverse the DOM from first to last, since later elements show above earlier elements. - visibleElements = visibleElements.reverse() while visibleElement = visibleElements.pop() rects = [visibleElement.rect] for {rect: negativeRect} in visibleElements -- cgit v1.2.3 From 32ebe749a0242deda845558340aee02bafa83e76 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Thu, 17 Mar 2016 11:01:21 +0000 Subject: Fix for f6925630a7e22b4483bc872d9242776bce5337c1. We should start by checking the *parent* of the candidate descendant. --- content_scripts/link_hints.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'content_scripts') diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee index f464f159..ced90cc5 100644 --- a/content_scripts/link_hints.coffee +++ b/content_scripts/link_hints.coffee @@ -321,8 +321,8 @@ LocalHints = while index < position candidateDescendant = visibleElements[index].element for _ in descendantsToCheck - return true if candidateDescendant == element.element candidateDescendant = candidateDescendant?.parentElement + return true if candidateDescendant == element.element index += 1 false # This is not a false positive. element -- cgit v1.2.3 From d2b14cbe04acf1e03f988aa3ef8deb78c0480782 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sun, 27 Mar 2016 06:22:14 +0100 Subject: Better test for scrollability. Testing the `scrollHeight` is cheaper than testing scrollability. There are a lot of non-scrollabile divs, so it makes sense to use this cheaper test as a filter. --- content_scripts/link_hints.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'content_scripts') diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee index ced90cc5..1795f0f7 100644 --- a/content_scripts/link_hints.coffee +++ b/content_scripts/link_hints.coffee @@ -262,7 +262,7 @@ LocalHints = reason = "Frame." when "div", "ol", "ul" isClickable ||= - if Scroller.isScrollableElement element + if element.clientHeight < element.scrollHeight and Scroller.isScrollableElement element reason = "Scroll." # An element with a class name containing the text "button" might be clickable. However, real clickables -- cgit v1.2.3