WIP - Add hover effect. Closes #9. #12

Merged
merged 1 commit into from Dec 7, 2017

Conversation

Projects
None yet
2 participants
Collaborator

biancadanforth commented Dec 7, 2017

mrrobotpageeffect

I discovered why we were seeing multiple "span" elements wrapped around the same matching word -- we were looking for "div" and "p" elements to check, and frequently, a "div" would have nested "div"s and "p"s, so if a word was in one of the nested elements, it'd get wrapped multiple times. I have changed the querySelectorAll to look for "p", "h1", "h2" and "h3" now, which are not commonly nested within each other. A quick check on the Mozilla privacy page showed they are never nested within each other for that page.

Loose ends:

  • There is currently a race condition with addon start-up between the background script and content scripts being loaded into existing tabs. The content script immediately sends a message to the background script to get the word list, sometimes before it is listening, throwing an error. Currently, this means some of the time on "web-ext run", the content script executes correctly, and sometimes it throws and does nothing. This seems like a chicken and egg problem to me, so I would appreciate ideas.
  • The page linked on the hover effect tooltip element currently opens in a new window, rather than a new tab. This is unusual behavior, since I am using an "a" element with target="_blank". Do we care?
  • While the fix above for multiple wrapped "span" elements should help some with the performance issue ("this addon is slowing down Firefox"), we should check to see if we are still receiving the warning.
  • The CSS for the hover effect adds quite a bit of CSS. Currently the position of the tooltip is to the right of the word, but there are styles in case we'd like to try "left", "bottom" and "top". Perhaps we should consider moving it to an external stylesheet now?

@biancadanforth biancadanforth changed the title from Add hover effect. Closes #9. to WIP - Add hover effect. Closes #9. Dec 7, 2017

Add hover effect. Closes #9.
I discovered why we were seeing multiple "span" elements wrapped around the same matching word -- we were looking for "div" and "p" elements to check, and frequently, a "div" would have nested "div"s and "p"s, so if a word was in one of the nested elements, it'd get wrapped multiple times. I have changed the `querySelectorAll` to look for "p", "h1", "h2" and "h3" now, which are not commonly nested within each other. A quick check on the Mozilla privacy page showed they are never nested within each other for that page.

Loose ends:
* There is currently a race condition with addon start-up between the background script and content scripts being loaded into existing tabs. The content script immediately sends a message to the background script to get the word list, sometimes before it is listening, throwing an error. Currently, this means some of the time on "web-ext run", the content script executes correctly, and somethings it throws and does nothing. This seems like a chicken and egg problem to me, so I would appreciate ideas.
* The page linked on the hover effect element currently opens in a new window, rather than a new tab. This is unusual behavior, since I am using an "a" element with target="_blank". Do we care?
* While the fix above for multiple wrapped "span" elements should help some with the performance issue (this addon is slowing down Firefox), we should check to see if we are still receiving the warning.
* The CSS for the hover effect adds quite a bit of CSS. Currently the position of the tooltip is to the right of the word, but there are styles in case we'd like to try "left", "bottom" and "top". Perhaps we should consider moving it to an external stylesheet now?
Member

gregglind commented Dec 7, 2017

I think the multiple wrap problem will go away with the tree walker implementation.

Member

gregglind commented Dec 7, 2017

I will make the treeWalker a different commit.

@gregglind gregglind merged commit 526535e into mozilla:master Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment