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

Support for dynamically rendered iframe like twitter widget #10

Open
melker opened this issue Jan 17, 2022 · 22 comments
Open

Support for dynamically rendered iframe like twitter widget #10

melker opened this issue Jan 17, 2022 · 22 comments
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone

Comments

@melker
Copy link

melker commented Jan 17, 2022

Support for dynamically added iframes like:

<a class="twitter-timeline" data-tweet-limit=5 href="someTwitterUrlHere"></a>
<script async src="//platform.twitter.com/widgets.js" charset="utf-8" data-cookiecategory="twitter" type="text/plain"></script>

Ref: orestbida/cookieconsent#195

@orestbida orestbida added this to the v1.1 milestone Feb 3, 2022
@orestbida orestbida added enhancement New feature or request good first issue Good for newcomers labels Feb 3, 2022
@orestbida orestbida modified the milestones: v1.1, v2.0.0 Nov 13, 2022
@MaluNoPeleke
Copy link

Is there already a testable solution for this to provide feedback?

@orestbida
Copy link
Owner

@MaluNoPeleke, it is possible but not ideal.

Unlike traditional iframes, twitter widgets handle their width/height dynamically, which doesn't fit well with iframemanager. You need to specify a fixed height/width to avoid content jumping when the iframe is loaded.

Demo on stackblitz.

@MoritzLost
Copy link

+1 for this feature request for me. We have a lot of use-cases for external services that aren't loaded as iframes, but as custom scripts. For example, we often include custom Google Maps oder Mapbox maps using Leaflet or Mapbox GL. In this case, the maps are created using those JS frameworks, not as an iframe. But loading the bitmap or vector tiles requires consent, so we need a placeholder with the option to consent.

In my opinion, this feature can be implemented in a really basic way, it doesn't need a lot of functionality:

  • I don't need iframemanager to manage scripts for me, that can be handled by cookieconsent. When used standalone, the necessary script can be inserted using a callback function, as in the example.
  • I also don't think iframemanager needs to care about its own size / aspect-ratio (maybe just set a min-height but that's it). For maps, the size is predefined anyway using a fixed height or aspect ratio, and the placeholder can just fill that space. For things like the Twitter feed, it's impossible to calculate how much space that's going to take up beforehand, so it's perfectly fine and acceptable if the content jumps after consent is given.

Basically, all I would need to consider this feature complete is the ability to tell iframemanager to insert the consent placeholder for a given category in a specific element. Currently this is not possible, so I would have to build my own interface to match iframemanager's interface for the use-cases described above.

As a reference, take klaro's contextual consent feature, which works in a similar way: https://klaro.org/docs/tutorials/contextual_consent

@orestbida
Copy link
Owner

Basically, all I would need to consider this feature complete is the ability to tell iframemanager to insert the consent placeholder for a given category in a specific element. Currently this is not possible, so I would have to build my own interface to match iframemanager's interface for the use-cases described above.

Mhm, I'm not sure I understand. If you have — say — 2 twitter embeds, why would you want to have 2 different consent placeholders even though they belong to the same service?

@MoritzLost
Copy link

@orestbida Sorry, maybe I didn't explain it well. I don't want to have two different placeholders. I want to use iframemanager as a placeholder for external content that isn't delivered in the form of an iframe.

For example, I am working on a map component that is powered by Mapbox and initialized through JavaScript using mapbox-gl. In this case, there's no iframe involved, since the map is initialized by my custom JavaScript code. Since loading Mapbox tiles requires consent, I need a placeholder with consent buttons. However, after consent is given, I don't need an actual <iframe> on the page, since the map is initialized by my JS callback.

I can actually get really close to what I want with iframemanager:

const initializeMap = () => {
    // JS code that initializes the map using mapbox-gl
}

im.run({
    onChange({ changedServices, eventSource }) {
        if (eventSource.action == 'accept' && eventSource.service == 'mapbox') {
            initializeMap();
        }
    },
})

The problem is that iframemanager always inserts an <iframe> into the page and forces it to overlay the existing content. I can solve this by hiding the iframe (or wrapper element) using JS, but that's not a very clean solution.

So to summarize, I want to use the placeholder provided by iframemanager, but I don't need iframemanager to actually insert an iframe in some cases.


More broadly: We're trying to use cookieconsent and iframemanager as a general, GDPR-compliant consent manager solution. Having placeholders for external content is very important – but not every external content is loaded in the form of an iframe. Being able to use the same placeholders for external content regardless of delivery method (iframe or custom JS code) would solve this. This would require an option or configuration that tells iframemanager not to insert an iframe for specific placeholders.

I don't even think this needs to be an additional configuration option. The service definition already includes a embedUrl config – maybe just allow users to leave this empty and skip inserting the iframe is this is the case? Or if you don't like that approach, maybe this can be controlled through another data-* attribute on the placeholder element, data-no-iframe or something like that?

@orestbida
Copy link
Owner

orestbida commented Jul 5, 2023

@MoritzLost, there is a custom widgets section which describes how to do this. You use the onAccept and onReject methods to define how the markup is generated and destroyed.

You can find 3 custom widget examples in the /demo dir:

Mapbox should be very similar to leaflet.

Edit: here's also a demo which shows how to connect iframemanager <-> cookieconsent

@MoritzLost
Copy link

@orestbida Thank you, I totally missed that! Sorry for the confusion. Now it's working fine, I'm triggering a CustomEvent in the global onChange callback (and the onAccept callback as well) and using that to initialize my map once consent is given. Then I set div.hidden = true in the onAccept callback.

This works and now the map is displayed correctly!


As a sidenote, I was seeing an issue where I get a flash of black before the overlay is hidden (see screen recording below).

Screen.Recording.2023-07-06.at.15.21.06.mov

Setting div.hidden = true fixed that. I think that would be a sensible solution to put into iframemanager itself. That would also be good for accessibility, since hidden removes the placeholder from the accessibility tree completely – right now the placeholder is just hidden using opacity and visibility. Should I create a separate issue for that?

@orestbida
Copy link
Owner

I'm not sure why you are setting the div as hidden, when the generated widget is appended inside the div.

Perhaps this issue is specific to Mapbox, as I don't see it in the above 3 mentioned custom widgets.

@orestbida
Copy link
Owner

Perhaps I'm lucky, but I don't see the mentioned issue in this basic example (tested in Chrome and Firefox):

<style>
    .mapbox {
        height: 100%;
        width: 100%;
    }
</style>

<div data-service="mapbox" data-autoscale>
    <div data-placeholder>
        <div id="map" class="mapbox"></div>
    </div>
</div>
{
    /**
     * @param {HTMLDivElement} div
     */
    onAccept: async (div) => {
        await loadScript('https://api.mapbox.com/mapbox-gl-js/v2.9.1/mapbox-gl.js');
        await im.childExists({childProperty: 'mapboxgl'});
    
        mapboxgl.accessToken = 'YOUR_TOKEN';
    
        const map = new mapboxgl.Map({
            container: 'map',
            style: 'mapbox://styles/mapbox/streets-v11'
        });
    
        await map.once('load');
    
        // Manually show placeholder
        div.classList.add('show-ph');
    },
    
    /**
     * @param {HTMLDivElement} serviceDiv
     */
    onReject: (iframe, serviceDiv) => {
        serviceDiv.lastElementChild.firstElementChild.remove();
    },

    languages: {
        // ...
    }
}

@MoritzLost
Copy link

MoritzLost commented Jul 6, 2023

@orestbida The map is not rendered inside the placeholder in my case. The code that initializes the map is in a separate file, I don't want to have that inside the iframemanager callback and I don't want to couple the map code to iframemanager. The map code just listens to the CustomEvent emitted inside the onAccept callback so it knows it's ok to load the map. That's why I have separate elements for the placeholder and the mount point for the map:

<div class="location-map">
    <div class="location-map__placeholder" data-service="mapbox" data-widget></div>
    <div class="location-map__mount" data-map-mount></div>
</div>

location-map is used as a wrapper and provides the map dimensions. The placeholder and map mount are both set to fill the container using position: absolute. Once consent is given, the placeholder is hidden and the map is initialized inside the data-map-mount element. Maybe setting the placeholder to position: absolute is causing the flash of black background? I can't use fixed width/height because the dimensions are dynamic and based on the viewport, and the placeholder needs to occupy the same space the rendered map will.

@MoritzLost
Copy link

@orestbida Ok, I tried to put the placeholder inside the element with data-widget="mapbox", but it doesn't work. The black placeholder doesn't go away, it stays there after interacting with one of the buttons.

I'm having trouble implementing this as intended because I don't understand how the setIframe method is supposed to be used. What does it do exactly, and what element should I pass it if I'm not using an iframe? Can I skip calling it at all?


Some additional small issues I noticed, let me know if you want me to create separate issues for those:

  • The data-autoscale option doesn't work well because it uses height: auto instead of height: 100%. This should probably be heigth: 100%? Or how is this intended to work?
  • I noticed the placeholder includes a tiny spinning element at the top right. It's invisible by default, but is briefly visible during the transition after clicking one of the buttons. A vestigial loading indicator? Should either be removed or fixed.

@orestbida
Copy link
Owner

The plugin was never designed to handle these types of custom use-cases that you're trying to achieve (data-service div wrapped in your own div, and with your custom logic handling).

You need to use a specific structure for this to work as intended. I posted the full example with mapbox in a comment above and I don't face any of the mentioned issues.

The data-autoscale option doesn't work well because it uses height: auto ...

Can you share a demo?

For the sake of clarity: you either use data-autoscale or data-widget, not both.

I noticed the placeholder includes a tiny spinning element at the top right ...

It should be at the bottom right of the div element.

It's invisible by default, but is briefly visible during the transition after clicking one of the buttons.

Yes, to show that "something" is loading. If the widget/iframe loads incredibly quickly, then the loader might not show at all or show for an extremely brief moment, which is normal. Why do you want it removed?

@MoritzLost
Copy link

MoritzLost commented Jul 7, 2023

The plugin was never designed to handle these types of custom use-cases that you're trying to achieve (data-service div wrapped in your own div, and with your custom logic handling).

@orestbida Fair enough. I've given it another try – now I'm following the recommended structure and it's working as intended. Some details below.

<div class="location-map">
    <div class="location-map__placeholder" data-service="mapbox" data-widget>
        <div class="location-map__mount" data-placeholder data-map-mount></div>
    </div>
</div>
onAccept: div => {
    initializeMap();
    div.classList.add('show-ph');
},

Sidenote, why is the class called show-ph? Isn't the consent overlay the placeholder, and the class reveals the actual content beneath the placeholder?

However, now I'm definitely seeing the issue with the flash of black. I think this is the expected behaviour. All the examples in the demo show the same behaviour. If you set everything to Don't ask again and then reload the page, you still see the overlay briefly before the actual content has loaded. That's because all the examples assume that the content will be loaded asynchronously, so the overlay will be visible for at least a couple of seconds. Since we initialize the map synchronously, the overlay only briefly flashes during the page-load, then is immediately hidden when my callback function runs.

Is there a way around this?


For the sake of clarity: you either use data-autoscale or data-widget, not both.

Without data-widget, iframemanager forces a specific aspect ratio using the padding-top method on the ::before method. But my map doesn't have a fixed aspect ratio – the width is always 100% of the viewport, but the height is set dynamically based on the viewport height:

height: min(80vh, 32rem);

The consent overlay needs to cover that space. I think the correct/intended solution is to use data-widget and then set width: 100% and height: 100%?

By the way, it's a bit inconvenient that iframemanager uses all: unset, this forces me to use !important or use a more specific selector everywhere I need to overwrite something (like in this case).

It should be at the bottom right of the div element.

It is now, maybe that was related to the other layout methods I tried.

Yes, to show that "something" is loading. If the widget/iframe loads incredibly quickly, then the loader might not show at all or show for an extremely brief moment, which is normal. Why do you want it removed?

Got it, thanks – in my current use-case, I'm hiding the consent overlay immediately. The map is displayed pretty much immediately in our case because our JS code that initialies the map is already loaded at this point (and it contains mapboxgl as it's installed via NPM and compiled together with our JS). But the loading indicator doesn't hurt in any case.

@orestbida
Copy link
Owner

Sidenote, why is the class called show-ph? Isn't the consent overlay the placeholder, and the class reveals the actual content beneath the placeholder?

"ph" (placeholder) refers to the div with the data-placeholder attribute, although I also am not fond of this name.

However, now I'm definitely seeing the issue with the flash of black.

Mhm, shouldn't it be a transition from black -> iframe rather than a flash? This is what I see:

2023-07-08.05-24-24.mp4

By the way, it's a bit inconvenient that iframemanager uses all: unset, this forces me to use !important or use a more specific selector everywhere I need to overwrite something (like in this case).

This is (sadly) needed, as there are badly coded websites, that apply global styles that might ruin the layout/look of the plugins generated markup.

The consent overlay needs to cover that space. I think the correct/intended solution is to use data-widget and then set width: 100% and height: 100%?

I will need to look this up in more detail, as currently it breaks some of the demo embeds.

@MoritzLost
Copy link

Mhm, shouldn't it be a transition from black -> iframe rather than a flash? This is what I see:

@orestbida I'm seeing a hard transition, for some reason I'm not seeing the placeholder or the transition.

Screen.Recording.2023-07-10.at.09.57.48.mov

Without the transition and with the callback adding the ph-show class immediately, it's more of a flash of black.

Just to rule out any issues with my implementation, I tried switching to a fixed width/height and making my callback asynchronous with some delay:

onAccept: div => new Promise(resolve => {
    triggerConsentManagerEvent('mapbox', true);
    setTimeout(() => resolve(div.classList.add('show-ph')), 2000);
}),

But the loading indicator is still not showing, though I do get a short transition now:

Screen.Recording.2023-07-10.at.10.14.38.mov

Any ideas why? What is triggering the loading indicator and the transition? The only difference between the demos and my implementation is that I'm not using im.childExists, can that have something to do with it?


I will need to look this up in more detail, as currently it breaks some of the demo embeds.

I've just set width: 100% !important and height: 100% !important on the placeholder, instead of setting a fixed width/height as in the demos – shouldn't make that much of a difference? Anyway, fixed width & height rarely makes sense for responsive website, so this should be addressed if it is an issue.

"ph" (placeholder) refers to the div with the data-placeholder attribute, although I also am not fond of this name.

Agreed – in my mind the consent overlay is the 'placeholder', so in the callback I'm hiding the placeholder and showing the actual content.

@MoritzLost
Copy link

@orestbida Is it possible to somehow skip displaying the black placeholder if consent has already been given? I know my map will load almost instantly, so it will always look like an error if I get a briefly visible black overlay.

@orestbida
Copy link
Owner

orestbida commented Jul 10, 2023

@MoritzLost, you need to make sure that the mapbox widget is fully loaded before toggling the show-ph class. Also the triggerConsentManagerEvent function needs to be async or return something promise-like.

The simplest way is to use the async/await syntax:

async function triggerConsentManagerEvent(service, enable) {
    // ...    

    const map = new mapboxgl.Map({...});

    // important
    await map.once('load');
}
onAccept: async (div) => {
    await triggerConsentManagerEvent('mapbox', true);
    div.classList.add('show-ph');
}

The only difference between the demos and my implementation is that I'm not using im.childExists, can that have something to do with it?

Nah, this method is a 'safeguard', used to make sure that an object or dom element actually exists, before accessing it. When dynamically loading a script, it may occur that although the script is fully loaded (downloaded), it might not yet be fully evaluated/executed. childExists does basically this: check every x ms, until it (object/dom element) exists.

@orestbida
Copy link
Owner

Is it possible to somehow skip displaying the black placeholder if consent has already been given?

Technically speaking you can, by checking if the cookie of the mapbox service is set; but its not a pretty solution.

Example (inside the head section):

<script>
    // check if "im_mapbox" cookie is set
    if(document.cookie.includes("im_mapbox="))
        document.documentElement.classList.add("im-mapbox");
</script>

<style>
    .im-mapbox div[data-service="mapbox"]{
        --im-bg: transparent;
    }
</style>

You also must handle the case when the service is rejected (remove the "im_mapbox" class).

@MoritzLost
Copy link

you need to make sure that the mapbox widget is fully loaded before toggling the show-ph class.

Ok, I think I found the issue. My map takes about ~200ms to fully initialize and load all tiles, and the transition is shorter than that. Thanks for the suggestions.

I could adjust my code to wait until all tiles are loaded before adding the ph-show class – but from a UX perspective, that is really a step backwards. I would much rather show the underlying map immediately. On a slower connection, the user will be able to see the map tiles as they are loaded, this is a much better indicator of the map loading than the placeholder. So the black overlay will be visible only very briefly, which looks buggy to the end-user.

I feel like this might be a common use-case – having a placeholder for content that will be visible immediately, so the placeholder shouldn't show up at all on consequent page loads. Would it be possible to add a configuration option for this, or a CSS property to customize this? That is, have a way to tell iframemanager not to show the overlay at all and instead reveal the underlying contents immediately if consent has already been given previously.


It was really difficult to debug this because of the obfuscated class names – .c-an .cll .c-la-b etc … what's going on there?

@orestbida
Copy link
Owner

This lib. was designed to be a more "elegant" approach to the usual "load->flash iframe" implementation, which is in contrast with what you're asking. IMO the current approach is the best option to handle all use cases, even though it makes the loading of external resources feel more "sluggish".

With that said, I will consider adding an attribute (and javascript option) to handle this particular preference/use-case. Please open a new issue so that we can better track this feature.

It was really difficult to debug this because of the obfuscated class names – .c-an .cll .c-la-b etc … what's going on there?

Both iframemanager v1.x.x and cookieconsent v2.x.x use short/cryptic classes, for the sake of a lighter plugin. Although they are gibberish to the users, they have a meaning for me as the author of the plugin.

@MoritzLost
Copy link

With that said, I will consider adding an attribute (and javascript option) to handle this particular preference/use-case. Please open a new issue so that we can better track this feature.

Fair enough – done! #45

Both iframemanager v1.x.x and cookieconsent v2.x.x use short/cryptic classes, for the sake of a lighter plugin. Although they are gibberish to the users, they have a meaning for me as the author of the plugin.

I very strongly disagree with this. The library has <500 lines of CSS, so a couple dozen selectors at best. Using readable class names would add a couple of kilobytes at best. And those can be compressed very well using DEFLATE, so we're talking miniscule amounts of bandwidth. For those few kilobytes the library is greatly improved:

  1. Other people than you can work on the library, submit PRs and suggest changes.
  2. Other people can read and understand what the library is doing, which would make it much easier to debug when something is going wrong, which will save both your users and you a lot of time.
  3. It's much easier to overwrite the styling of the library if the class names make sense. Right now it's a matter of trial-and-error and much guess-work to try and style anything differently that doesn't use one of the predefined custom properties.

My suggestion would be to use readable class names, ideally scoped to a namespace (like .im-*). To save on bytes, you can in turn drop all the div element selectors – they're not necessary at all.

@MaluNoPeleke
Copy link

@MaluNoPeleke, it is possible but not ideal.

Unlike traditional iframes, twitter widgets handle their width/height dynamically, which doesn't fit well with iframemanager. You need to specify a fixed height/width to avoid content jumping when the iframe is loaded.

Demo on stackblitz.

Is there any other problem than the fixed height/width issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants