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

Use console warning rather than DOM element to indicate missing stylesheet #6261

Merged
merged 3 commits into from
Jun 8, 2018

Conversation

lbud
Copy link
Contributor

@lbud lbud commented Mar 1, 2018

This uses CSSOM interfaces to check for the presence of a .mapboxgl-map CSS rule upon map creation and display a warning in the console if it's missing, rather than adding and hiding a DOM element (ref #5785, #5359 (comment)).

Fixes #5786.

Having to stub console.warn so many places in the unit tests feels weird, and treacherous for future writers of tests that include Map — if there's a better way to do this I'm all ears!

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • manually test the debug page

src/ui/map.js Outdated
@@ -1388,12 +1387,20 @@ class Map extends Camera {
return [width, height];
}

_hasCSS(): boolean {
for (const sheet of (window.document.styleSheets: any)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How fast is this if a page has a bajillion CSS rules?

If iterating over them all in JS is expensive, another approach would be:

  • Add .mapboxgl-canary { background-color: salmon; } to mapbox-gl.css. (Unfortunately canary is not a CSS color keyword so I chose another animal.)
  • Map adds <div class="mapboxgl-canary" style="visibility: hidden;"/> and then checks the computed background color.

@lbud
Copy link
Contributor Author

lbud commented Mar 2, 2018

Oof, looks like there was a bug (possibly introduced in Chrome 64) causing cross-origin stylesheets not only to be missing a cssRules property but to error upon calling that getter (https://bugs.chromium.org/p/chromium/issues/detail?id=813826). Discovered while digging through Studio investigating #6261 (comment).

@lbud lbud force-pushed the missing-css-warn branch from 4aa6315 to 29eeb69 Compare March 2, 2018 22:42
@lbud
Copy link
Contributor Author

lbud commented Mar 2, 2018

Switched to checking the computed color of a hidden div as suggested in #6261 (comment) (thank you @jfirebaugh!), both for performance and because of #6261 (comment).

@jfirebaugh
Copy link
Contributor

Having to stub console.warn so many places in the unit tests feels weird, and treacherous for future writers of tests that include Map — if there's a better way to do this I'm all ears!

Agreed. I can think of a couple possible approaches:

  • Actually add a mapbox-gl.css <link> element to the test DOM. This would be the most "real-world" way to do it, but I don't think it'll work because AFAIK jsdom does not fully implement CSS and just has a stub implementation of getComputedStyle.
  • Move the canary check into a private method, and create a common createMap function used by all tests that stubs it out.

@lbud lbud force-pushed the missing-css-warn branch from 29eeb69 to 9a1d9e9 Compare March 7, 2018 23:43
@lbud lbud force-pushed the missing-css-warn branch from 9a1d9e9 to 7a88593 Compare June 7, 2018 19:34
@lbud lbud force-pushed the missing-css-warn branch from f9f9e64 to a0a4199 Compare June 7, 2018 21:47
@lbud lbud requested a review from jfirebaugh June 7, 2018 23:15
src/ui/map.js Outdated
_detectMissingCSS(): void {
const computedColor = window.getComputedStyle(this._missingCSSCanary).getPropertyValue('background-color');
if (computedColor !== 'rgb(250, 128, 114)') {
warnOnce('Missing Mapbox GL JS CSS: map may not display correctly.');
Copy link
Contributor

Choose a reason for hiding this comment

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

This page appears to be missing CSS declarations for Mapbox GL JS, which may cause the map to display incorrectly. Please ensure your page includes mapbox-gl.css, as described in https://www.mapbox.com/mapbox-gl-js/api/.

@lbud lbud merged commit e6a0604 into master Jun 8, 2018
@lbud lbud deleted the missing-css-warn branch June 8, 2018 00:19
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.

2 participants