Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: iframe click detection #222

Merged
merged 4 commits into from
Aug 29, 2020

Conversation

renatodeleao
Copy link
Collaborator

@renatodeleao renatodeleao commented Aug 25, 2020

Attempt to close #80 with iframe click detection workaround.

Clicks on iframe don't bubble, so they are undetected by our document.documentElement listeners. Workarounds in the SO threads and dev internet fall into 3 categories:

  1. add pseudo-elements cover ups on iframes, blocking click on iframe content but still bubbling document click;
  2. wrap iframes on an extra <div> and capture the click on it;
  3. window.onblur + document.activeElement combo;

The last one seemed the less intrusive:

  • it doesn't block clicks on iframe content itself;
  • it doesn't add extra DIVs which could cause layout issues;
  • it keeps performance impact to a minimum;

How it works

The gist is that a click on an iframe will move focus to it. Since iframe has its own window our main app window will blur: on that event we can then check if document.activeElement is an IFRAME and execute the handler.

Implementation details

adds a custom vco:faux-iframe-click eventName id to the provided eventsNames array, this will be used to perform conditional logic:
- bind blur event to window instead of document.documentElement;
- use onFauxIframeClick instead of default onEvent as handler wrapper;

  • Adds detectIframe config flag that defaults to true — see fix: iframe click detection #222 (comment)
  • if true, the following event object will be merged together with el[HANDLERS_PROPERTY] array.
     {
       event: 'blur',
       srcTarget: window,
       handler: (event) => onFauxIframeClick({ event, handler, middleware }),
     }
  • The regular loop through el[HANDLERS_PROPERTY] for event binding will occur, but now also binds our special blur event to the window
  • onFauxIframeClick doesn't require the event path of regular onEvent, i simply checks if document.activeElement is an iframe and wraps handler execution into a setTimeout to workaround an issue with Firefox, that only sets activeElement correctly after blur, on the next tick (event loop).
  • Documents the following #caveats section under README.md

Caveats

  • Click outside will be triggered once on iframe. Subsequent clicks on iframe will not execute the handler until focus has been moved back to main window — by clicking anywhere outside the iframe. This is the "expected" behaviour as by clicking the iframe, focus will move to iframe contents — a different window, so subsequent clicks are inside it. There might be way to workaround this such as calling window.focus() at the end of the provided handler but that will break normal tab/focus flow, therefore decided to not be included in the source code.
  • Moving focus to iframe via tab navigation also triggers window.blur consequently the faux-click-outside handler - no workaround found ATM.

@coveralls
Copy link

Coverage Status

Coverage decreased (-5.7%) to 60.92% when pulling 2ae5a23 on renatodeleao:fix/iframe-click-detection into b0caa85 on ndelvalle:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-5.7%) to 60.92% when pulling 2ae5a23 on renatodeleao:fix/iframe-click-detection into b0caa85 on ndelvalle:master.

@coveralls
Copy link

coveralls commented Aug 25, 2020

Coverage Status

Coverage decreased (-3.6%) to 63.095% when pulling a7ff8d1 on renatodeleao:fix/iframe-click-detection into e56b509 on ndelvalle:master.

@renatodeleao
Copy link
Collaborator Author

renatodeleao commented Aug 25, 2020

@ndelvalle since i was already looking at the code for our "programatic usage discussion" (220), i've gave it a try to fix the "iframe click issue", as i might have a situation on my app that might need it — a <Previewer> that is an iframe on the right with <Dropdowns /> on the left that won't close on outside click.

I've tried to squeeze the juice from the SO threads mentioned in #80. If you think this has "future" let me know and I will update the specs to keep code coverage on the green.

☮️

@renatodeleao renatodeleao marked this pull request as ready for review August 25, 2020 11:20
@ndelvalle
Copy link
Owner

@renatodeleao thanks for the contribution! To be honest the fix seems a bit hacky but I don't see a pretty way to work around this issue, so LGTM 👍

@renatodeleao
Copy link
Collaborator Author

@ndelvalle thanks for looking at it!
💯 hacky for sure, but hey it's web development, we're used to it 😛 !

I did a couple more searches but didn't find any cleaner contenders for this.
I'll do a couple more tests and update the specs for the window event before merge.

@renatodeleao renatodeleao force-pushed the fix/iframe-click-detection branch 2 times, most recently from 1b9bd2d to 4fdcb28 Compare August 27, 2020 17:05
Clicks on iframe don't bubble, so they are undetected by our
`document.documentElement` listeners. Workarounds in the SO threads and
dev internet fall into 3 categories:
- add pseudo-elements cover ups on iframes;
- wrap iframes on an extra <div> and capture the click on it;
- window.onblur + document.activeElement combo;

The last one seemed the less intrusive:
- it doesn't block clicks on iframe content itself;
- it doesn't add extra DIVs which could cause layout issues;
- it keeps performance impact to a minimum;

How it works:
The gist is that a click on an iframe will move `focus` to it. Since
iframe has its own `window` our main app window will blur: on that event
we can then check if document.activeElement is an IFRAME and execute the
handler considering it a "faux click outside".

Implementation details:
- adds a custom `vco:faux-iframe-click` eventName id to the provided
  eventsNames array, this will be used to perform conditional logic:
  - bind `blur` event to window instead of document.documentElement;
  - use `onFauxIframeClick` instead of default `onEvent` as handler wrapper;
- `onFauxIframeClick` doens't require the event path logic checks as it
  only uses `document.activeElement` and simply wraps handler execution
  into a setTimeout to workaround an issue with Firefox, that only sets
  `activeElement` correctly after blur, on the next tick (event loop).

Caveats:
- Click outside will be triggered once on iframe. Subsequent clicks will
  not execute the handler untill focus has been moved back to main window.
  This is the "expected" behaviour as by clicking the iframe, focus will
  move to iframe contents — a different window. There might be way
  to workaround this, by triggering `window.focus()` at the end of the
  provided handler but that will break normal tab/focus flow, therefore
  not included by default.
- Moving focus to iframe via tab navigation also triggers `window.blur`
  consequently the click-outside handler.
some form of empiric evidence of feature working
@renatodeleao renatodeleao self-assigned this Aug 27, 2020
@renatodeleao
Copy link
Collaborator Author

update 📣
So I've did a few more tests mostly cross-browser + iOS safari, and seem to be working alright.

Since this isn't "a real fix" and having in mind the #caveats section present in the description, I want to ask your opinion on enabling this behind a config flag, that we could set true by default, but playing defensive on possible edge cases.

function processDirectiveArguments(bindingValue) {
  return {
    ...
    isActive: !(bindingValue.isActive === false),
    useIframeFauxClick: !!(bindingValue.useIframeFauxClick === true)
  }
}


function bind(el, { value }) {
...
  el[HANDLERS_PROPERTY] = ['vco:faux-iframe-click', ...events].map(
    (eventName) => {
      const isForIframe = i === 0 && useIframeFauxClick

      return {
        event: isForIframe ? 'blur' : eventName,
        ...

(e2e2bd1)

Also feedback on naming is welcomed 😅 .

@ndelvalle
Copy link
Owner

@renatodeleao I really like the idea of having it under a flag!, WDYT of just a isIframe attribute and having a little description with possible caveats or whatever in the readme?

}))
// Note: keep events array immutable, since events value defaults to
// EVENTS variable reference.
el[HANDLERS_PROPERTY] = ['vco:faux-iframe-click', ...events].map(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we handle the iframe case separately? If it has the iframe flag, attach only our custom event listener and discard all events and early return.
If it does not have the iframe case, use the standard code? WDYT?

Copy link
Collaborator Author

@renatodeleao renatodeleao Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Not sure if I got it correctly, as in, even if we got the iframe flag to true we still need to add the other listeners on bind() to keep regular behaviour (clicks outside that are not on an iframe?).

Because of the caveats now documented in readme and edge cases that we might not be aware of related.

Implementation details:
- Adds `detectIframe` config option
- Move the whole fauxIframeClick event object under a separated if clause and merge it with the regular eventsObject array if detectIframe is true
- Documented new config flag and caveats on readme
- Updated specs accordingly
@renatodeleao
Copy link
Collaborator Author

@ndelvalle I've simplified (i guess 😅 ) the code and avoided so many ternaries — 5530376

  • In the end i've went with detectIframe for name: the reason is that isIframe felt like a special option if we were binding the directive to iframe element itself (<iframe v-click-outside>) and that's not the case. But let me know what you think!
  • Added caveats section to the README.md
  • Update tests around the new flag

@ndelvalle ndelvalle merged commit a6c7e1b into ndelvalle:master Aug 29, 2020
@ndelvalle
Copy link
Owner

@renatodeleao thanks for the great work! 🙌🏻🙌🏻🙌🏻

@ndelvalle
Copy link
Owner

@renatodeleao v-click-outside 3.1.0 published 🎉

@renatodeleao
Copy link
Collaborator Author

🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Click outside doesn't work on iframe click
3 participants