HTML Credits #6331

hpinkos · 2018-03-14T17:49:24Z

Other web mapping platforms like Leaflet and OpenLayers define attributions as HTML strings. This changes our Credit to do the same. HTML allows for much more flexibility than only exposing options for setting the text, image source and URL link. For example, will allow users to create credits like
"My source is from some website with some license", something that is frequently needed for proper attribution, but not currently supported with our Credit.

Summary of changes:

Modified Credit to accept an html string instead of an option object
Added xxs.js library for sanitizing the HTML before creating a DOM element from it
Since Credit no longer differentiates between 'text' credits and 'image' credits, modified CreditDisplay to add all credits to the same credit container
To keep the Cesium credit first, created a separate CreditDisplay._cesiumCreditContainer to add the Cesium credit

cesium-concierge · 2018-03-14T17:49:26Z

@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, so you can review this at any time.

⚠️ I noticed that a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version. Once you do, please confirm by commenting on this pull request.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

hpinkos · 2018-03-14T17:56:28Z

Source/ThirdParty/xss.js

+         value.substr(0, 8) === 'https://' ||
+         value.substr(0, 7) === 'mailto:' ||
+         value.substr(0, 4) === 'tel:' ||
+         value.substr(0, 11) === 'data:image/' ||


I added this line and wanted to point out this change.

We have a few image credits like the Cesium and Bing credits that use <img src="data:image/png;base64,....."/>. xss was removing this. There wasn't a way to whitelist it through the API so I added a line to allow it. I couldn't find any reason why a data URI would lead to a security vulnerability, but I wanted to double check.

I added a comment to the top of the file so anyone updating this in the future knows that this file was altered from the original source.

I would prefer we not modify this, both because it's third-party code and because even if we don't think there are security implications, there may be. I don't know why we embed the Cesium and Bing credits anyway, they can just be external images like everything else.

If you are postive there are no security implications, then open an issue/PR in the xss respository and have it merged into the upstream project first.

hpinkos · 2018-03-15T20:09:44Z

@mramato can you review when you have some time? thanks!

mramato · 2018-03-17T14:41:13Z

Source/Scene/MapboxImageryProvider.js

-        text: 'Improve this map',
-        link: 'https://www.mapbox.com/map-feedback/'
-    })];
+    var defaultCredit = new Credit('<a href="https://www.mapbox.com/about/maps/" target="_blank">© Mapbox © OpenStreetMap</a> <a href="https://www.mapbox.com/map-feedback/">Improve this map</a>');


Shouldn't we just use © for the copyright symbol now?

This is also incorrect, see https://www.mapbox.com/help/how-attribution-works/#text-attribution

mramato · 2018-03-17T14:43:59Z

Source/Core/CesiumTerrainProvider.js

@@ -167,8 +167,8 @@ define([
                    var deprecationLinkText = 'Check out the new high-resolution Cesium World Terrain';
                    var deprecationLink = 'https://cesium.com/blog/2018/03/01/introducing-cesium-world-terrain/';
                    that._tileCredits = [
-                        new Credit({ text: deprecationText, showOnScreen: true }),
-                        new Credit({ text: deprecationLinkText, link: deprecationLink, showOnScreen: true })
+                        new Credit(deprecationText, true),


This was only two credits because of non-HTML credit limitations. You can merge it into a single one now.

mramato · 2018-03-17T14:47:04Z

Source/Core/Credit.js

+        var key;
+        if (typeof html !== 'string') {
+            var options = defaultValue(html, defaultValue.EMPTY_OBJECT);
+            deprecationWarning('Credit options', 'The options paramater has been deprecated and will be removed in Cesium 1.46.  Instead, pass in an html string (or a string of text)');


Minor nit, but html -> HTML

mramato · 2018-03-17T14:47:46Z

Source/Core/Credit.js

@@ -137,6 +170,7 @@ define([
         */
        imageUrl : {
            get : function() {
+                deprecationWarning('Credit.text', 'Credit.text is deprecated and will be removed in Cesium 1.46.  Instead, use Credit.html to get the credit content.');


Copy paste typos in deprecation errors throughout

mramato · 2018-03-17T14:56:33Z

Source/Core/Credit.js

+
+    function getElement(credit) {
+        var html = credit.html;
+        html = xss(html);


What are we using for a whitelist here? Because it looks like not even the <b> tag works?

Turns out the fact that <b> tags don't work is because createDomNode doesn't work, not because of xss.

I'm just using the default whitelist https://github.com/leizongmin/js-xss/blob/master/lib/default.js

I'll look into the createDomNode thing

mramato · 2018-03-17T14:59:12Z

Source/Core/Credit.js

-            }
-            element.className = 'cesium-credit-text';
+    function createDomNode(html) {
+        var div = document.createElement('span');


I don't think this can't be a span because a span technically can't have child elements.

a span technically can't have child elements

Are you sure? I couldn't find any sources that say that. I use a span because it's inherently inline, where a div is a block element. Plus we were putting everything into a span already and it's worked fine.

Here's a good answer on the subject with links back to the spec https://stackoverflow.com/a/11314736

span can only have "phrasing elements", so I was only half-right but using a div is still the correct way to go. If it's trivial to use an inline div, then we should, but we could probably get away with a span element if we had to.

mramato · 2018-03-17T14:59:33Z

The Bing key warning message can now be improved since we can have inline links and other styling.

EDIT: Perhaps add a little bold or other larger print.

mramato · 2018-03-17T15:03:49Z

Source/Core/Credit.js

+        var div = document.createElement('span');
+        div.innerHTML = html;
+
+        if (div.children.length === 1) {


This check is bad, I'm not sure you need it at all but if you're trying to avoid an extra wrapper then it should be childNodes not children

You can test with This entire credit should be <b>visible</b>

mramato · 2018-03-17T15:10:20Z

Source/Core/Credit.js

+            key = JSON.stringify([text, imageUrl, link]);
+        } else {
+            key = html;
+        }

        if (defined(creditToId[key])) {


Do we even need creditToId anymore? Can't we just use the html as the id directly?

credit.id is a number and is used as the index of the credit in the array of credits in CreditDisplay. So we still need this.

mramato · 2018-03-17T15:21:41Z

Looks like there is some missing test coverage, particular in the deprecated code.

It's probably also a good idea to expose createDomNode as a private function for testing it directly.

It also looks like a data attribution link appears in the bottom of the tests at some point and doesn't go away.

mramato · 2018-03-17T15:24:17Z

Thanks @hpinkos! This will be a big improvement. That's it for my initial review, but I actually didn't review Credit.js or CreditDisplay.js too closely yet, once all my initial feedback is addressed I'll give this another look.

Thanks again!

hpinkos · 2018-03-19T18:42:05Z

@mramato ready for another look. I addressed all of your comments.

mramato · 2018-03-20T00:46:16Z

Source/Core/CesiumTerrainProvider.js

                    var deprecationLinkText = 'Check out the new high-resolution Cesium World Terrain';
                    var deprecationLink = 'https://cesium.com/blog/2018/03/01/introducing-cesium-world-terrain/';
                    that._tileCredits = [
-                        new Credit({ text: deprecationText, showOnScreen: true }),
-                        new Credit({ text: deprecationLinkText, link: deprecationLink, showOnScreen: true })
+                        new Credit('<span>' + deprecationText + '</span> <a href="' + deprecationLink + '" target="_blank">' + deprecationLinkText + '</a>', true)


I think we should have createDomNode automatically add target='_blank' to all credit links (then we can remove them all from this PR)

mramato · 2018-03-20T00:51:24Z

Source/Core/Credit.js

        DeveloperError) {
    'use strict';

    var nextCreditId = 0;
    var creditToId = {};

+    function createDomNode(html) {
+        var span = document.createElement('span');


I thought we were going to make this a div? Also, this function no longer seems necessary since it's only used in the equally small getElement below.

In fact, getElement can also be moved into the element getter, since the whole thing is trivial.

hpinkos · 2018-03-20T14:18:31Z

@mramato I think this is ready now

Changed span to div
Automatically add target="_blank" to all a tags

mramato · 2018-03-20T16:09:39Z

One last bug. Switching to Sentinel-2 (or any of the 3 ion imagery assets in the BaseLayerPicker) does not show the correct credit. The ion logo isn't there and the other attribution gets shown on screen instead of in the lightbox.

mramato · 2018-03-20T16:10:33Z

I also tweaked the Bing/Mapbox signup credits to make them more obvious.

hpinkos · 2018-03-20T19:15:41Z

@mramato fixed

hpinkos · 2018-03-20T19:38:26Z

Whoops, broke one of the tests. One sec

hpinkos · 2018-03-20T20:32:44Z

@mramato ready

mramato · 2018-03-20T21:21:13Z

Sorry, found one more issue. The ion Sandcastle examples are triggering deprecation warnings: http://localhost:8080/Apps/Sandcastle/index.html?src=Blue%20Marble.html&label=ion%20Assets

EDIT: Actually, anything using ion assets.

hpinkos · 2018-03-20T21:36:53Z

@mramato that will be fixed when we change the attributions being sent from the ion server. I'm wasn't sure if it's something we should work around here.

mramato · 2018-03-20T21:58:40Z

Makes sense, since we're putting that change in by tomorrow, I'll just hold off on merging this (but otherwise looks ready).

mramato · 2018-03-21T22:19:39Z

@hpinkos one more thing, loading ion demos, such as http://localhost:8080/Apps/Sandcastle/index.html?src=Sentinel-2.html&label=ion%20Assets results in the logo being push up above Data attribution instead of next to it (even though there are no other credits)

hpinkos · 2018-03-22T00:19:53Z

@mramato that's a problem with the credit being sent from the ion server. I submitted a fix for it. I guess we should wait to merge this until that goes through.

hpinkos · 2018-03-22T15:32:00Z

@mramato this is ready now

mramato · 2018-03-22T16:25:45Z

Thanks!

lilleyse · 2018-03-23T19:51:02Z

Kind of a minor thing, so feel free to disregard.

The Bing key message that is printed to the console includes the html tags, but would look better without them.

hpinkos commented Mar 14, 2018

View reviewed changes

mramato reviewed Mar 17, 2018

View reviewed changes

html credits

3f6e682

hpinkos force-pushed the html-credits branch from f1a4bed to 3f6e682 Compare March 19, 2018 18:16

test coverage

24a89f8

hpinkos and others added 2 commits March 19, 2018 16:02

fix buildModuleUrl for node

a1c93c2

Merge branch 'master' into html-credits

62e797f

mramato reviewed Mar 20, 2018

View reviewed changes

Fix Credit.js formatting.

ef54816

mramato reviewed Mar 20, 2018

View reviewed changes

cleanup

002ab19

Bold the Bing/Mapbox signup credits and make them links.

60f98ae

fix ion imagery credit

080392c

fix specs

440a9e6

Merge branch 'master' into html-credits

664df19

fix styling

6e53d5c

mramato merged commit 08848b4 into master Mar 22, 2018

mramato deleted the html-credits branch March 22, 2018 16:25

gberaudo mentioned this pull request Mar 27, 2018

Display correct credit information openlayers/ol-cesium#575

Closed

hpinkos mentioned this pull request Mar 27, 2018

Remove deprecated credit parameters #6373

Closed

tfili mentioned this pull request Apr 3, 2018

cesium_base regression in v1.44 #6411

Closed

kring mentioned this pull request Jun 8, 2018

Impossible to credit some sources properly in Cesium. #6280

Closed

HTML Credits #6331

HTML Credits #6331

Conversation

hpinkos commented Mar 14, 2018 • edited Loading

cesium-concierge commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpinkos commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented Mar 17, 2018

mramato commented Mar 17, 2018

hpinkos commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpinkos commented Mar 20, 2018

mramato commented Mar 20, 2018

mramato commented Mar 20, 2018

hpinkos commented Mar 20, 2018

hpinkos commented Mar 20, 2018

hpinkos commented Mar 20, 2018

mramato commented Mar 20, 2018 • edited Loading

hpinkos commented Mar 20, 2018

mramato commented Mar 20, 2018

mramato commented Mar 21, 2018

hpinkos commented Mar 22, 2018

hpinkos commented Mar 22, 2018

mramato commented Mar 22, 2018

lilleyse commented Mar 23, 2018

hpinkos commented Mar 14, 2018 •

edited

Loading

mramato commented Mar 17, 2018 •

edited

Loading

mramato commented Mar 20, 2018 •

edited

Loading