From 61f75448a216037251264e496e147299a1e264ae Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Wed, 29 Apr 2015 07:44:39 +0100 Subject: Fix text matching/exclusion for text link hints. We need to sort elements by their text length. This allows us to exclude the text of descendants from the hint text generated for their ancestors. Note: Tests not yet fixed. --- content_scripts/link_hints.coffee | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee index 440e6598..fc95bcd8 100644 --- a/content_scripts/link_hints.coffee +++ b/content_scripts/link_hints.coffee @@ -60,6 +60,12 @@ LinkHints = # For these modes, we filter out those elements which don't have an HREF (since there's nothing we can do # with them). elements = (el for el in elements when el.element.href?) if mode in [ COPY_LINK_URL, OPEN_INCOGNITO ] + if settings.get "filterLinkHints" + # When using text filtering, we sort the elements such that we visit descendants before their ancestors. + # This allows us to exclude the text used for matching descendants from that used for matching their + # ancestors. + textLength = (el) -> el.element.textContent?.length ? 0 + elements = elements.sort (a,b) -> textLength(a) - textLength b hintMarkers = (@createMarkerFor(el) for el in elements) @getMarkerMatcher().fillInMarkers(hintMarkers) @@ -238,8 +244,6 @@ LinkHints = # Remove rects from elements where another clickable element lies above it. nonOverlappingElements = [] # Traverse the DOM from first to last, since later elements show above earlier elements. - # NOTE(smblott). filterHints.generateLinkText also assumes this order when generating the content text for - # each hint. Specifically, we consider descendents before we consider their ancestors. visibleElements = visibleElements.reverse() while visibleElement = visibleElements.pop() rects = [visibleElement.rect] -- cgit v1.2.3 From bc5e9545398e0e74529738e690f16063ae41f8c0 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Wed, 29 Apr 2015 08:37:28 +0100 Subject: Fix text matching/exclusion; repair tests. --- content_scripts/link_hints.coffee | 2 +- tests/dom_tests/dom_tests.coffee | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee index fc95bcd8..a5be6c57 100644 --- a/content_scripts/link_hints.coffee +++ b/content_scripts/link_hints.coffee @@ -65,7 +65,7 @@ LinkHints = # This allows us to exclude the text used for matching descendants from that used for matching their # ancestors. textLength = (el) -> el.element.textContent?.length ? 0 - elements = elements.sort (a,b) -> textLength(a) - textLength b + elements.sort (a,b) -> textLength(a) - textLength b hintMarkers = (@createMarkerFor(el) for el in elements) @getMarkerMatcher().fillInMarkers(hintMarkers) diff --git a/tests/dom_tests/dom_tests.coffee b/tests/dom_tests/dom_tests.coffee index bb09a0a8..7bc50c72 100644 --- a/tests/dom_tests/dom_tests.coffee +++ b/tests/dom_tests/dom_tests.coffee @@ -228,9 +228,9 @@ context "Filtered link hints", hintMarkers = getHintMarkers() assert.equal "1", hintMarkers[0].textContent.toLowerCase() assert.equal "2", hintMarkers[1].textContent.toLowerCase() - assert.equal "3", hintMarkers[2].textContent.toLowerCase() + assert.equal "3: a label", hintMarkers[2].textContent.toLowerCase() assert.equal "4: a label", hintMarkers[3].textContent.toLowerCase() - assert.equal "5: a label", hintMarkers[4].textContent.toLowerCase() + assert.equal "5", hintMarkers[4].textContent.toLowerCase() context "Input focus", -- cgit v1.2.3 From 7917647ccad6712118b08cd86e3b4a6dda763ae3 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Wed, 29 Apr 2015 10:41:48 +0100 Subject: Use innerHTML instead of contentText. If we use contentText, then it is possible for a child and its to have exactly the same content text, in which case it is arbitrary as to which is sorted first, hence which we choose (where we should be choosing the child). Using innerHTML instead, we guess that even if the contentText lengths are the same, the innerHTML of the parent will be longer (since it contains that of the child). --- content_scripts/link_hints.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee index a5be6c57..3cebac4c 100644 --- a/content_scripts/link_hints.coffee +++ b/content_scripts/link_hints.coffee @@ -64,8 +64,8 @@ LinkHints = # When using text filtering, we sort the elements such that we visit descendants before their ancestors. # This allows us to exclude the text used for matching descendants from that used for matching their # ancestors. - textLength = (el) -> el.element.textContent?.length ? 0 - elements.sort (a,b) -> textLength(a) - textLength b + length = (el) -> el.element.innerHTML?.length ? 0 + elements.sort (a,b) -> length(a) - length b hintMarkers = (@createMarkerFor(el) for el in elements) @getMarkerMatcher().fillInMarkers(hintMarkers) -- cgit v1.2.3 From 561a63f31e387c6383a2e62558a44df7ab2918f4 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Wed, 29 Apr 2015 12:15:59 +0100 Subject: Fix text matching/exclusion; repair tests (again). We should also take a look at these tests and the assumptions they're making. They're difficult to follow as is. --- tests/dom_tests/dom_tests.coffee | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/dom_tests/dom_tests.coffee b/tests/dom_tests/dom_tests.coffee index 7bc50c72..8c2b73c3 100644 --- a/tests/dom_tests/dom_tests.coffee +++ b/tests/dom_tests/dom_tests.coffee @@ -156,6 +156,9 @@ context "Alphabetical link hints", assert.equal "", hintMarkers[0].style.display context "Filtered link hints", + # Note. In all of these tests, the order of the elements returned by getHintMarkers() may be different from + # the order they are listed in the test HTML content. This is because LinkHints.activateMode() sorts the + # elements. setup -> stub settings.values, "filterLinkHints", true @@ -205,8 +208,8 @@ context "Filtered link hints", should "label the images", -> hintMarkers = getHintMarkers() assert.equal "1: alt text", hintMarkers[0].textContent.toLowerCase() - assert.equal "2: alt text", hintMarkers[1].textContent.toLowerCase() - assert.equal "3: some title", hintMarkers[2].textContent.toLowerCase() + assert.equal "2: some title", hintMarkers[1].textContent.toLowerCase() + assert.equal "3: alt text", hintMarkers[2].textContent.toLowerCase() assert.equal "4", hintMarkers[3].textContent.toLowerCase() context "Input hints", -- cgit v1.2.3