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

HTML Credits #6331

Merged
merged 11 commits into from
Mar 22, 2018
Merged

HTML Credits #6331

merged 11 commits into from
Mar 22, 2018

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Mar 14, 2018

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
Copy link

Signed CLA is on file.

@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.

🌍 🌎 🌏

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

hpinkos commented Mar 15, 2018

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

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>');
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we just use &copy; for the copyright symbol now?

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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)');
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit, but html -> HTML

@@ -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 link
Contributor

Choose a reason for hiding this comment

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

Copy paste typos in deprecation errors throughout


function getElement(credit) {
var html = credit.html;
html = xss(html);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

mramato commented Mar 17, 2018

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.

var div = document.createElement('span');
div.innerHTML = html;

if (div.children.length === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

if (defined(creditToId[key])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

mramato commented Mar 17, 2018

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
Copy link
Contributor

mramato commented Mar 17, 2018

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
Copy link
Contributor Author

hpinkos commented Mar 19, 2018

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

DeveloperError) {
'use strict';

var nextCreditId = 0;
var creditToId = {};

function createDomNode(html) {
var span = document.createElement('span');
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@hpinkos
Copy link
Contributor Author

hpinkos commented Mar 20, 2018

@mramato I think this is ready now

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

@mramato
Copy link
Contributor

mramato commented Mar 20, 2018

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
Copy link
Contributor

mramato commented Mar 20, 2018

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

@hpinkos
Copy link
Contributor Author

hpinkos commented Mar 20, 2018

@mramato fixed

@hpinkos
Copy link
Contributor Author

hpinkos commented Mar 20, 2018

Whoops, broke one of the tests. One sec

@hpinkos
Copy link
Contributor Author

hpinkos commented Mar 20, 2018

@mramato ready

@mramato
Copy link
Contributor

mramato commented Mar 20, 2018

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
Copy link
Contributor Author

hpinkos commented Mar 20, 2018

@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
Copy link
Contributor

mramato commented Mar 20, 2018

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

@mramato
Copy link
Contributor

mramato commented Mar 21, 2018

@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)

image

@hpinkos
Copy link
Contributor Author

hpinkos commented Mar 22, 2018

@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
Copy link
Contributor Author

hpinkos commented Mar 22, 2018

@mramato this is ready now

@mramato
Copy link
Contributor

mramato commented Mar 22, 2018

Thanks!

@mramato mramato merged commit 08848b4 into master Mar 22, 2018
@mramato mramato deleted the html-credits branch March 22, 2018 16:25
@lilleyse
Copy link
Contributor

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.
credits

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.

4 participants