From f99eb3b09b9a6d0390f0b1ea48ae642ae57c6ee9 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Sat, 23 Feb 2019 02:07:37 -0500 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Extract=20CSS=20module=20(?= =?UTF-8?q?#21007)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Extract CSS module Extracts CSS specific functions from `src/dom.js` into `src/css.js`. * Fix build * Move assertIsName into css module * Move test --- build-system/dep-check-config.js | 2 +- build-system/eslint-rules/no-import-rename.js | 4 +- build-system/eslint-rules/query-selector.js | 2 +- build-system/tasks/presubmit-checks.js | 6 - .../amp-access/0.1/amp-access-server-jwt.js | 2 +- .../amp-access/0.1/amp-access-server.js | 2 +- .../amp-date-picker/0.1/amp-date-picker.js | 2 +- extensions/amp-form/0.1/amp-form.js | 2 +- .../0.1/amp-gwd-animation-impl.js | 4 +- .../0.1/amp-lightbox-gallery.js | 2 +- extensions/amp-story/0.1/amp-story.js | 2 +- extensions/amp-story/0.1/animation.js | 4 +- extensions/amp-story/0.1/page-advancement.js | 3 +- extensions/amp-story/0.1/progress-bar.js | 3 +- extensions/amp-story/1.0/amp-story.js | 2 +- extensions/amp-story/1.0/animation.js | 6 +- extensions/amp-story/1.0/page-advancement.js | 3 +- extensions/amp-story/1.0/progress-bar.js | 3 +- .../0.1/amp-video-docking.js | 12 +- .../amp-web-push/0.1/amp-web-push-config.js | 2 +- .../0.1/amp-web-push-permission-dialog.js | 2 +- .../amp-web-push/0.1/web-push-service.js | 3 +- src/css.js | 124 ++++++++++++++++ src/dom.js | 132 +++--------------- src/font-stylesheet-timeout.js | 2 +- src/service/navigation.js | 2 +- src/shadow-embed.js | 2 +- test/unit/test-css.js | 46 ++++++ test/unit/test-dom.js | 43 ++---- 29 files changed, 236 insertions(+), 188 deletions(-) create mode 100644 src/css.js create mode 100644 test/unit/test-css.js diff --git a/build-system/dep-check-config.js b/build-system/dep-check-config.js index 3c95bd67b742c..92311b0353cb7 100644 --- a/build-system/dep-check-config.js +++ b/build-system/dep-check-config.js @@ -86,7 +86,7 @@ exports.rules = [ 'src/sanitizer.js->third_party/caja/html-sanitizer.js', 'extensions/amp-viz-vega/**->third_party/vega/vega.js', 'extensions/amp-viz-vega/**->third_party/d3/d3.js', - 'src/dom.js->third_party/css-escape/css-escape.js', + 'src/css.js->third_party/css-escape/css-escape.js', 'src/shadow-embed.js->third_party/webcomponentsjs/ShadowCSS.js', 'third_party/timeagojs/timeago.js->' + 'third_party/timeagojs/timeago-locales.js', diff --git a/build-system/eslint-rules/no-import-rename.js b/build-system/eslint-rules/no-import-rename.js index 62f444d84df28..d83a55b1520f0 100644 --- a/build-system/eslint-rules/no-import-rename.js +++ b/build-system/eslint-rules/no-import-rename.js @@ -44,9 +44,11 @@ const imports = { 'setStyle', 'setStyles', ], - 'src/dom': [ + 'src/css': [ 'escapeCssSelectorIdent', 'escapeCssSelectorNth', + ], + 'src/dom': [ 'scopedQuerySelector', 'scopedQuerySelectorAll', ], diff --git a/build-system/eslint-rules/query-selector.js b/build-system/eslint-rules/query-selector.js index be5daa7692e45..4aaa1c1906200 100644 --- a/build-system/eslint-rules/query-selector.js +++ b/build-system/eslint-rules/query-selector.js @@ -146,7 +146,7 @@ module.exports = function(context) { context.report({ node: expression, message: 'Each selector value must be escaped by ' + - 'escapeCssSelectorIdent in src/dom.js', + 'escapeCssSelectorIdent in src/css.js', }); } diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js index 1c7f6093b566e..4108676a75437 100644 --- a/build-system/tasks/presubmit-checks.js +++ b/build-system/tasks/presubmit-checks.js @@ -816,12 +816,6 @@ const forbiddenTermsSrcInclusive = { 'extensions/amp-bind/0.1/bind-expr-impl.js', ], }, - 'scopeSelectorForTesting': { - message: 'scopeSelector is not intended to be used outside of dom.js', - whitelist: [ - 'src/dom.js', - ], - }, '[^.]loadPromise': { message: 'Most users should use BaseElement…loadPromise.', whitelist: [ diff --git a/extensions/amp-access/0.1/amp-access-server-jwt.js b/extensions/amp-access/0.1/amp-access-server-jwt.js index 32d98c85ae287..ddd5ad6fa11b6 100644 --- a/extensions/amp-access/0.1/amp-access-server-jwt.js +++ b/extensions/amp-access/0.1/amp-access-server-jwt.js @@ -25,7 +25,7 @@ import { } from '../../../src/url'; import {dev, user, userAssert} from '../../../src/log'; import {dict} from '../../../src/utils/object'; -import {escapeCssSelectorIdent} from '../../../src/dom'; +import {escapeCssSelectorIdent} from '../../../src/css'; import {fetchDocument} from '../../../src/document-fetcher'; import {getMode} from '../../../src/mode'; import {isArray} from '../../../src/types'; diff --git a/extensions/amp-access/0.1/amp-access-server.js b/extensions/amp-access/0.1/amp-access-server.js index 245e58808f31a..4ffe4f62420b0 100644 --- a/extensions/amp-access/0.1/amp-access-server.js +++ b/extensions/amp-access/0.1/amp-access-server.js @@ -18,7 +18,7 @@ import {AccessClientAdapter} from './amp-access-client'; import {Services} from '../../../src/services'; import {dev, devAssert} from '../../../src/log'; import {dict} from '../../../src/utils/object'; -import {escapeCssSelectorIdent} from '../../../src/dom'; +import {escapeCssSelectorIdent} from '../../../src/css'; import {fetchDocument} from '../../../src/document-fetcher'; import {isExperimentOn} from '../../../src/experiments'; import {isProxyOrigin, removeFragment} from '../../../src/url'; diff --git a/extensions/amp-date-picker/0.1/amp-date-picker.js b/extensions/amp-date-picker/0.1/amp-date-picker.js index b7a8b2efb3476..decfbd89ec92f 100644 --- a/extensions/amp-date-picker/0.1/amp-date-picker.js +++ b/extensions/amp-date-picker/0.1/amp-date-picker.js @@ -27,7 +27,6 @@ import {Services} from '../../../src/services'; import {batchFetchJsonFor} from '../../../src/batched-json'; import { closestAncestorElementBySelector, - escapeCssSelectorIdent, isRTL, iterateCursor, scopedQuerySelector, @@ -40,6 +39,7 @@ import {createSingleDatePicker} from './single-date-picker'; import {dashToCamelCase} from '../../../src/string'; import {dev, user, userAssert} from '../../../src/log'; import {dict} from '../../../src/utils/object'; +import {escapeCssSelectorIdent} from '../../../src/css'; import {once} from '../../../src/utils/function'; import {requireExternal} from '../../../src/module'; diff --git a/extensions/amp-form/0.1/amp-form.js b/extensions/amp-form/0.1/amp-form.js index 1a14336587ae1..4829143a192a7 100644 --- a/extensions/amp-form/0.1/amp-form.js +++ b/extensions/amp-form/0.1/amp-form.js @@ -37,7 +37,6 @@ import { ancestorElementsByTag, childElementByAttr, createElementWithAttributes, - escapeCssSelectorIdent, iterateCursor, removeElement, tryFocus, @@ -46,6 +45,7 @@ import {createCustomEvent} from '../../../src/event-helper'; import {createFormDataWrapper} from '../../../src/form-data-wrapper'; import {deepMerge, dict} from '../../../src/utils/object'; import {dev, devAssert, user, userAssert} from '../../../src/log'; +import {escapeCssSelectorIdent} from '../../../src/css'; import { formOrNullForElement, getFormAsObject, diff --git a/extensions/amp-gwd-animation/0.1/amp-gwd-animation-impl.js b/extensions/amp-gwd-animation/0.1/amp-gwd-animation-impl.js index 800ccdb6a9531..ab7e2f80cea68 100644 --- a/extensions/amp-gwd-animation/0.1/amp-gwd-animation-impl.js +++ b/extensions/amp-gwd-animation/0.1/amp-gwd-animation-impl.js @@ -16,13 +16,13 @@ import {createCustomEvent} from '../../../src/event-helper'; import {dev, user} from '../../../src/log'; import {dict, hasOwn} from '../../../src/utils/object'; +import {escapeCssSelectorIdent} from '../../../src/css'; +import {installServiceInEmbedScope} from '../../../src/service'; import { - escapeCssSelectorIdent, scopedQuerySelector, waitForBodyPromise, waitForChild, } from '../../../src/dom'; -import {installServiceInEmbedScope} from '../../../src/service'; import {toArray} from '../../../src/types'; /** diff --git a/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js b/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js index f67e4cd9146e0..42bf80752b7b3 100644 --- a/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js +++ b/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js @@ -33,13 +33,13 @@ import { closest, closestAncestorElementBySelector, elementByTag, - escapeCssSelectorIdent, scopedQuerySelector, scopedQuerySelectorAll, } from '../../../src/dom'; import {clamp} from '../../../src/utils/math'; import {dev, devAssert, user, userAssert} from '../../../src/log'; import {dict} from '../../../src/utils/object'; +import {escapeCssSelectorIdent} from '../../../src/css'; import {getData, isLoaded, listen} from '../../../src/event-helper'; import { getElementServiceForDoc, diff --git a/extensions/amp-story/0.1/amp-story.js b/extensions/amp-story/0.1/amp-story.js index 0224ce84c4827..9e1ba96e78eba 100644 --- a/extensions/amp-story/0.1/amp-story.js +++ b/extensions/amp-story/0.1/amp-story.js @@ -75,7 +75,6 @@ import { childElementByTag, childElements, closest, - escapeCssSelectorIdent, matches, removeElement, scopedQuerySelectorAll, @@ -89,6 +88,7 @@ import { import {debounce} from '../../../src/utils/rate-limit'; import {dev, devAssert, user} from '../../../src/log'; import {dict} from '../../../src/utils/object'; +import {escapeCssSelectorIdent} from '../../../src/css'; import {findIndex} from '../../../src/utils/array'; import {getDetail} from '../../../src/event-helper'; import {getMode} from '../../../src/mode'; diff --git a/extensions/amp-story/0.1/animation.js b/extensions/amp-story/0.1/animation.js index e252f04a96a01..9abe81a059797 100644 --- a/extensions/amp-story/0.1/animation.js +++ b/extensions/amp-story/0.1/animation.js @@ -29,12 +29,12 @@ import { } from '../../amp-animation/0.1/web-animation-types'; import {assertDoesNotContainDisplay, setStyles} from '../../../src/style'; import {dev, devAssert, user, userAssert} from '../../../src/log'; +import {escapeCssSelectorIdent} from '../../../src/css'; +import {map, omit} from '../../../src/utils/object'; import { - escapeCssSelectorIdent, scopedQuerySelector, scopedQuerySelectorAll, } from '../../../src/dom'; -import {map, omit} from '../../../src/utils/object'; import {timeStrToMillis, unscaledClientRect} from './utils'; /** const {string} */ diff --git a/extensions/amp-story/0.1/page-advancement.js b/extensions/amp-story/0.1/page-advancement.js index bf9cf3dd43d07..bf0c70234799f 100644 --- a/extensions/amp-story/0.1/page-advancement.js +++ b/extensions/amp-story/0.1/page-advancement.js @@ -15,8 +15,9 @@ */ import {Services} from '../../../src/services'; import {VideoEvents} from '../../../src/video-interface'; -import {closest, escapeCssSelectorIdent} from '../../../src/dom'; +import {closest} from '../../../src/dom'; import {dev, user} from '../../../src/log'; +import {escapeCssSelectorIdent} from '../../../src/css'; import {hasTapAction, timeStrToMillis} from './utils'; import {listenOnce} from '../../../src/event-helper'; import {map} from '../../../src/utils/object'; diff --git a/extensions/amp-story/0.1/progress-bar.js b/extensions/amp-story/0.1/progress-bar.js index 9b15135a819b4..1aeb885cde9a1 100644 --- a/extensions/amp-story/0.1/progress-bar.js +++ b/extensions/amp-story/0.1/progress-bar.js @@ -16,9 +16,10 @@ import {POLL_INTERVAL_MS} from './page-advancement'; import {Services} from '../../../src/services'; import {dev, devAssert} from '../../../src/log'; -import {escapeCssSelectorNth, scopedQuerySelector} from '../../../src/dom'; +import {escapeCssSelectorNth} from '../../../src/css'; import {hasOwn, map} from '../../../src/utils/object'; import {scale, setImportantStyles} from '../../../src/style'; +import {scopedQuerySelector} from '../../../src/dom'; /** diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index 7658b2f21c596..b418a066b843e 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -85,7 +85,6 @@ import { childElements, closest, createElementWithAttributes, - escapeCssSelectorIdent, isRTL, scopedQuerySelectorAll, } from '../../../src/dom'; @@ -99,6 +98,7 @@ import { import {debounce} from '../../../src/utils/rate-limit'; import {dev, devAssert, user} from '../../../src/log'; import {dict} from '../../../src/utils/object'; +import {escapeCssSelectorIdent} from '../../../src/css'; import {findIndex} from '../../../src/utils/array'; import {getConsentPolicyState} from '../../../src/consent'; import {getDetail} from '../../../src/event-helper'; diff --git a/extensions/amp-story/1.0/animation.js b/extensions/amp-story/1.0/animation.js index c14bbf658af1d..da1637f5ab61e 100644 --- a/extensions/amp-story/1.0/animation.js +++ b/extensions/amp-story/1.0/animation.js @@ -28,13 +28,13 @@ import { } from '../../amp-animation/0.1/web-animation-types'; import {assertDoesNotContainDisplay, setStyles} from '../../../src/style'; import {dev, devAssert, user, userAssert} from '../../../src/log'; +import {escapeCssSelectorIdent} from '../../../src/css'; +import {getPresetDef, setStyleForPreset} from './animation-presets'; +import {map, omit} from '../../../src/utils/object'; import { - escapeCssSelectorIdent, scopedQuerySelector, scopedQuerySelectorAll, } from '../../../src/dom'; -import {getPresetDef, setStyleForPreset} from './animation-presets'; -import {map, omit} from '../../../src/utils/object'; import {timeStrToMillis, unscaledClientRect} from './utils'; /** const {string} */ diff --git a/extensions/amp-story/1.0/page-advancement.js b/extensions/amp-story/1.0/page-advancement.js index bceaecb9c1ab6..0f36b7227d6aa 100644 --- a/extensions/amp-story/1.0/page-advancement.js +++ b/extensions/amp-story/1.0/page-advancement.js @@ -24,11 +24,12 @@ import {AdvancementMode} from './story-analytics'; import {Services} from '../../../src/services'; import {TAPPABLE_ARIA_ROLES} from '../../../src/service/action-impl'; import {VideoEvents} from '../../../src/video-interface'; -import {closest, escapeCssSelectorIdent, matches} from '../../../src/dom'; +import {closest, matches} from '../../../src/dom'; import {dev, user} from '../../../src/log'; import { embeddedComponentSelectors, } from './amp-story-embedded-component'; +import {escapeCssSelectorIdent} from '../../../src/css'; import {hasTapAction, timeStrToMillis} from './utils'; import {listenOnce} from '../../../src/event-helper'; diff --git a/extensions/amp-story/1.0/progress-bar.js b/extensions/amp-story/1.0/progress-bar.js index dcf610db8b8a2..e47fe734ac7e8 100644 --- a/extensions/amp-story/1.0/progress-bar.js +++ b/extensions/amp-story/1.0/progress-bar.js @@ -16,9 +16,10 @@ import {POLL_INTERVAL_MS} from './page-advancement'; import {Services} from '../../../src/services'; import {dev, devAssert} from '../../../src/log'; -import {escapeCssSelectorNth, scopedQuerySelector} from '../../../src/dom'; +import {escapeCssSelectorNth} from '../../../src/css'; import {hasOwn, map} from '../../../src/utils/object'; import {scale, setImportantStyles} from '../../../src/style'; +import {scopedQuerySelector} from '../../../src/dom'; /** diff --git a/extensions/amp-video-docking/0.1/amp-video-docking.js b/extensions/amp-video-docking/0.1/amp-video-docking.js index a7ed0d010fb35..b24171ef0dd1e 100644 --- a/extensions/amp-video-docking/0.1/amp-video-docking.js +++ b/extensions/amp-video-docking/0.1/amp-video-docking.js @@ -41,17 +41,17 @@ import { } from '../../../src/event-helper'; import {dev, devAssert, user, userAssert} from '../../../src/log'; import {dict} from '../../../src/utils/object'; -import { - escapeCssSelectorIdent, - isRTL, - removeElement, - scopedQuerySelector, -} from '../../../src/dom'; +import {escapeCssSelectorIdent} from '../../../src/css'; import {getInternalVideoElementFor} from '../../../src/utils/video'; import {getServiceForDoc} from '../../../src/service'; import {htmlFor, htmlRefs} from '../../../src/static-template'; import {installStylesForDoc} from '../../../src/style-installer'; import {isFiniteNumber} from '../../../src/types'; +import { + isRTL, + removeElement, + scopedQuerySelector, +} from '../../../src/dom'; import {layoutRectLtwh, moveLayoutRect} from '../../../src/layout-rect'; import {mapRange} from '../../../src/utils/math'; import {once} from '../../../src/utils/function'; diff --git a/extensions/amp-web-push/0.1/amp-web-push-config.js b/extensions/amp-web-push/0.1/amp-web-push-config.js index eb5f798bf12f3..07b16e6bf3a2d 100644 --- a/extensions/amp-web-push/0.1/amp-web-push-config.js +++ b/extensions/amp-web-push/0.1/amp-web-push-config.js @@ -16,7 +16,7 @@ import {CONFIG_TAG, SERVICE_TAG, TAG} from './vars'; import {dev, user, userAssert} from '../../../src/log'; -import {escapeCssSelectorIdent} from '../../../src/dom'; +import {escapeCssSelectorIdent} from '../../../src/css'; import {getServiceForDoc} from '../../../src/service'; import {parseUrlDeprecated} from '../../../src/url'; diff --git a/extensions/amp-web-push/0.1/amp-web-push-permission-dialog.js b/extensions/amp-web-push/0.1/amp-web-push-permission-dialog.js index 354c15cd4d29e..d5f50bfef02b5 100644 --- a/extensions/amp-web-push/0.1/amp-web-push-permission-dialog.js +++ b/extensions/amp-web-push/0.1/amp-web-push-permission-dialog.js @@ -16,7 +16,7 @@ import {NotificationPermission, StorageKeys} from './vars'; import {WindowMessenger} from './window-messenger'; -import {escapeCssSelectorIdent} from '../../../src/dom'; +import {escapeCssSelectorIdent} from '../../../src/css'; import {getMode} from '../../../src/mode'; import { parseQueryString, diff --git a/extensions/amp-web-push/0.1/web-push-service.js b/extensions/amp-web-push/0.1/web-push-service.js index 09786a13d040e..7caabc3e9fbd6 100644 --- a/extensions/amp-web-push/0.1/web-push-service.js +++ b/extensions/amp-web-push/0.1/web-push-service.js @@ -21,9 +21,10 @@ import {Services} from '../../../src/services'; import {WebPushWidgetVisibilities} from './amp-web-push-widget'; import {WindowMessenger} from './window-messenger'; import {dev, user} from '../../../src/log'; -import {escapeCssSelectorIdent, openWindowDialog} from '../../../src/dom'; +import {escapeCssSelectorIdent} from '../../../src/css'; import {getMode} from '../../../src/mode'; import {installStylesForDoc} from '../../../src/style-installer'; +import {openWindowDialog} from '../../../src/dom'; import {parseQueryString, parseUrlDeprecated} from '../../../src/url'; /** @typedef {{ diff --git a/src/css.js b/src/css.js new file mode 100644 index 0000000000000..7a781da6a0c7b --- /dev/null +++ b/src/css.js @@ -0,0 +1,124 @@ +/** + * Copyright 2019 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {cssEscape} from '../third_party/css-escape/css-escape'; +import {devAssert} from './log'; + +/** + * Asserts that name is just an alphanumeric word, and does not contain + * advanced CSS selector features like attributes, psuedo-classes, class names, + * nor ids. + * @param {string} name + */ +export function assertIsName(name) { + devAssert(/^[\w-]+$/.test(name)); +} + + +/** + * @type {boolean|undefined} + */ +let scopeSelectorSupported; + +/** + * @param {boolean|undefined} val + * @visibleForTesting + */ +export function setScopeSelectorSupportedForTesting(val) { + scopeSelectorSupported = val; +} + +/** + * Test that the :scope selector is supported and behaves correctly. + * @param {!Element} el + * @return {boolean} + */ +export function isScopeSelectorSupported(el) { + if (scopeSelectorSupported !== undefined) { + return scopeSelectorSupported; + } + + return (scopeSelectorSupported = testScopeSelector(el)); +} + +/** + * Test that the :scope selector is supported and behaves correctly. + * @param {!Element} el + * @return {boolean} + */ +function testScopeSelector(el) { + try { + const doc = el.ownerDocument; + const testElement = doc.createElement('div'); + const testChild = doc.createElement('div'); + testElement.appendChild(testChild); + // NOTE(cvializ, #12383): Firefox's implementation is incomplete, + // therefore we test actual functionality of`:scope` as well. + return testElement./*OK*/querySelector(':scope div') === testChild; + } catch (e) { + return false; + } +} + +/** + * Prefixes a selector for ancestor selection. Splits in subselectors and + * applies prefix to each. + * + * e.g. + * ``` + * prependSelectorsWith('div', '.i-amphtml-scoped'); + * // => '.i-amphtml-scoped div' + * prependSelectorsWith('div, ul', ':scope'); + * // => ':scope div, :scope ul' + * prependSelectorsWith('div, ul', 'article >'); + * // => 'article > div, article > ul' + * ``` + * + * @param {string} selector + * @param {string} distribute + * @return {string} + */ +export function prependSelectorsWith(selector, distribute) { + return selector.replace(/^|,/g, `$&${distribute} `); +} + +/** + * Escapes an ident (ID or a class name) to be used as a CSS selector. + * + * See https://drafts.csswg.org/cssom/#serialize-an-identifier. + * + * @param {string} ident + * @return {string} + */ +export function escapeCssSelectorIdent(ident) { + return cssEscape(ident); +} + +/** + * Escapes an ident in a way that can be used by :nth-child() psuedo-class. + * + * See https://github.com/w3c/csswg-drafts/issues/2306. + * + * @param {string|number} ident + * @return {string} + */ +export function escapeCssSelectorNth(ident) { + const escaped = String(ident); + // Ensure it doesn't close the nth-child psuedo class. + devAssert(escaped.indexOf(')') === -1); + return escaped; +} + diff --git a/src/dom.js b/src/dom.js index 36465df95c7fd..c92b940311cf4 100644 --- a/src/dom.js +++ b/src/dom.js @@ -15,7 +15,11 @@ */ import {Deferred} from './utils/promise'; -import {cssEscape} from '../third_party/css-escape/css-escape'; +import { + assertIsName, + isScopeSelectorSupported, + prependSelectorsWith, +} from './css'; import {dev, devAssert} from './log'; import {dict} from './utils/object'; import {onDocumentReady, whenDocumentReady} from './document-ready'; @@ -320,7 +324,7 @@ export function ancestorElements(child, predicate) { * @return {!Array} */ export function ancestorElementsByTag(child, tagName) { - assertIsTagName(tagName); + assertIsName(tagName); tagName = tagName.toUpperCase(); return ancestorElements(child, el => { return el.tagName == tagName; @@ -403,8 +407,7 @@ export function childNodes(parent, callback) { * @return {?Element} */ export function childElementByAttr(parent, attr) { - // Yah, it's supposed to be an attr and not a tag name. But same code. - assertIsTagName(attr); + assertIsName(attr); return scopedQuerySelector/*OK*/(parent, `> [${attr}]`); } @@ -416,8 +419,7 @@ export function childElementByAttr(parent, attr) { * @return {?Element} */ export function lastChildElementByAttr(parent, attr) { - // Yah, it's supposed to be an attr and not a tag name. But same code. - assertIsTagName(attr); + assertIsName(attr); return lastChildElement(parent, el => { return el.hasAttribute(attr); }); @@ -431,8 +433,7 @@ export function lastChildElementByAttr(parent, attr) { * @return {!NodeList} */ export function childElementsByAttr(parent, attr) { - // Yah, it's supposed to be an attr and not a tag name. But same code. - assertIsTagName(attr); + assertIsName(attr); return scopedQuerySelectorAll/*OK*/(parent, `> [${attr}]`); } @@ -444,7 +445,7 @@ export function childElementsByAttr(parent, attr) { * @return {?Element} */ export function childElementByTag(parent, tagName) { - assertIsTagName(tagName); + assertIsName(tagName); return scopedQuerySelector/*OK*/(parent, `> ${tagName}`); } @@ -456,7 +457,7 @@ export function childElementByTag(parent, tagName) { * @return {!NodeList} */ export function childElementsByTag(parent, tagName) { - assertIsTagName(tagName); + assertIsName(tagName); return scopedQuerySelectorAll/*OK*/(parent, `> ${tagName}`); } @@ -485,74 +486,10 @@ export function matches(el, selector) { * @return {?Element} */ export function elementByTag(element, tagName) { - assertIsTagName(tagName); + assertIsName(tagName); return element./*OK*/querySelector(tagName); } -/** - * Asserts that tagName is just an alphanumeric word, and does not contain - * advanced CSS selector features like attributes, psuedo-classes, class names, - * nor ids. - * @param {string} tagName - */ -function assertIsTagName(tagName) { - devAssert(/^[\w-]+$/.test(tagName)); -} - -/** - * @type {boolean|undefined} - * @visibleForTesting - */ -let scopeSelectorSupported; - -/** - * @param {boolean|undefined} val - * @visibleForTesting - */ -export function setScopeSelectorSupportedForTesting(val) { - scopeSelectorSupported = val; -} - -/** - * Test that the :scope selector is supported and behaves correctly. - * @param {!Element} parent - * @return {boolean} - */ -function isScopeSelectorSupported(parent) { - const doc = parent.ownerDocument; - try { - const testElement = doc.createElement('div'); - const testChild = doc.createElement('div'); - testElement.appendChild(testChild); - // NOTE(cvializ, #12383): Firefox's implementation is incomplete, - // therefore we test actual functionality of`:scope` as well. - return testElement./*OK*/querySelector(':scope div') === testChild; - } catch (e) { - return false; - } -} - -/** - * Prefixes a selector for ancestor selection. Splits in subselectors and - * applies prefix to each. - * - * e.g. - * ``` - * scopeSelector('.i-amphtml-scoped', 'div'); // .i-amphtml-scoped div - * scopeSelector(':scope', 'div, ul'); // :scope div, :scope ul - * scopeSelector('article >', 'div, ul'); // article > div, article > ul - * ``` - * - * @param {string} distribute - * @param {string} selector - * @return {string} - */ -function scopeSelector(distribute, selector) { - return selector.replace(/^|,/g, `$&${distribute} `); -} - -export const scopeSelectorForTesting = scopeSelector; - /** * Finds all elements that matche `selector`, scoped inside `root` * for user-agents that do not support native scoping. @@ -566,7 +503,7 @@ export const scopeSelectorForTesting = scopeSelector; function scopedQuerySelectionFallback(root, selector) { const unique = 'i-amphtml-scoped'; root.classList.add(unique); - const scopedSelector = scopeSelector(`.${unique}`, selector); + const scopedSelector = prependSelectorsWith(selector, `.${unique}`); const elements = root./*OK*/querySelectorAll(scopedSelector); root.classList.remove(unique); return elements; @@ -580,11 +517,8 @@ function scopedQuerySelectionFallback(root, selector) { * @return {?Element} */ export function scopedQuerySelector(root, selector) { - if (scopeSelectorSupported == null) { - scopeSelectorSupported = isScopeSelectorSupported(root); - } - if (scopeSelectorSupported) { - return root./*OK*/querySelector(scopeSelector(':scope', selector)); + if (isScopeSelectorSupported(root)) { + return root./*OK*/querySelector(prependSelectorsWith(selector, ':scope')); } // Only IE. @@ -600,11 +534,9 @@ export function scopedQuerySelector(root, selector) { * @return {!NodeList} */ export function scopedQuerySelectorAll(root, selector) { - if (scopeSelectorSupported == null) { - scopeSelectorSupported = isScopeSelectorSupported(root); - } - if (scopeSelectorSupported) { - return root./*OK*/querySelectorAll(scopeSelector(':scope', selector)); + if (isScopeSelectorSupported(root)) { + return root./*OK*/querySelectorAll( + prependSelectorsWith(selector, ':scope')); } // Only IE. @@ -754,34 +686,6 @@ export function isRTL(doc) { return dir == 'rtl'; } - -/** - * Escapes an ident (ID or a class name) to be used as a CSS selector. - * - * See https://drafts.csswg.org/cssom/#serialize-an-identifier. - * - * @param {string} ident - * @return {string} - */ -export function escapeCssSelectorIdent(ident) { - return cssEscape(ident); -} - -/** - * Escapes an ident in a way that can be used by :nth-child() psuedo-class. - * - * See https://github.com/w3c/csswg-drafts/issues/2306. - * - * @param {string|number} ident - * @return {string} - */ -export function escapeCssSelectorNth(ident) { - const escaped = String(ident); - // Ensure it doesn't close the nth-child psuedo class. - devAssert(escaped.indexOf(')') === -1); - return escaped; -} - /** * Escapes `<`, `>` and other HTML charcaters with their escaped forms. * @param {string} text diff --git a/src/font-stylesheet-timeout.js b/src/font-stylesheet-timeout.js index c7a568a4f67a2..214cc86b64fae 100644 --- a/src/font-stylesheet-timeout.js +++ b/src/font-stylesheet-timeout.js @@ -14,7 +14,7 @@ * limitations under the License. */ -import {escapeCssSelectorIdent} from './dom'; +import {escapeCssSelectorIdent} from './css'; import {isExperimentOn} from './experiments'; import {onDocumentReady} from './document-ready'; import {urls} from './config'; diff --git a/src/service/navigation.js b/src/service/navigation.js index 3dd7e0cafd39c..d5dc1b020bf62 100644 --- a/src/service/navigation.js +++ b/src/service/navigation.js @@ -17,13 +17,13 @@ import {Services} from '../services'; import { closestAncestorElementBySelector, - escapeCssSelectorIdent, isIframed, openWindowDialog, tryFocus, } from '../dom'; import {dev, user, userAssert} from '../log'; import {dict} from '../utils/object'; +import {escapeCssSelectorIdent} from '../css'; import { getExtraParamsUrl, shouldAppendExtraParams, diff --git a/src/shadow-embed.js b/src/shadow-embed.js index 59fc03b902194..5b393a9d551a5 100644 --- a/src/shadow-embed.js +++ b/src/shadow-embed.js @@ -25,11 +25,11 @@ import { import { childElementsByTag, closestNode, - escapeCssSelectorIdent, iterateCursor, removeElement, } from './dom'; import {dev, devAssert} from './log'; +import {escapeCssSelectorIdent} from './css'; import {installCssTransformer} from './style-installer'; import {setInitialDisplay, setStyle} from './style'; import {toArray, toWin} from './types'; diff --git a/test/unit/test-css.js b/test/unit/test-css.js new file mode 100644 index 0000000000000..f4d58d88eb495 --- /dev/null +++ b/test/unit/test-css.js @@ -0,0 +1,46 @@ +/** + * Copyright 2015 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {escapeCssSelectorIdent, prependSelectorsWith} from '../../src/css'; + +describe('CSS', () => { + + describe('escapeCssSelectorIdent', () => { + + it('should escape', () => { + expect(escapeCssSelectorIdent('a b')).to.equal('a\\ b'); + }); + }); + + describe('scopeSelector', () => { + + it('concats simple', () => { + expect(prependSelectorsWith('div', '.i-amphtml-scoped')) + .to.equal('.i-amphtml-scoped div'); + }); + + it('concats multiple selectors (2)', () => { + expect(prependSelectorsWith('div,ul', ':scope')) + .to.equal(':scope div,:scope ul'); + }); + + it('concats multiple selectors (4)', () => { + expect(prependSelectorsWith('div,ul,ol,section', 'div >')) + .to.equal('div > div,div > ul,div > ol,div > section'); + }); + + }); +}); diff --git a/test/unit/test-dom.js b/test/unit/test-dom.js index 0c1039a33008f..61da3e4d7fab3 100644 --- a/test/unit/test-dom.js +++ b/test/unit/test-dom.js @@ -18,6 +18,7 @@ import * as dom from '../../src/dom'; import {BaseElement} from '../../src/base-element'; import {createAmpElementForTesting} from '../../src/custom-element'; import {loadPromise} from '../../src/event-helper'; +import {setScopeSelectorSupportedForTesting} from '../../src/css'; import {toArray} from '../../src/types'; @@ -30,31 +31,10 @@ describes.sandboxed('DOM', {}, env => { }); afterEach(() => { - dom.setScopeSelectorSupportedForTesting(undefined); + setScopeSelectorSupportedForTesting(undefined); sandbox.restore(); }); - describe('scopeSelector', () => { - - const {scopeSelectorForTesting: scopeSelector} = dom; - - it('concats simple', () => { - expect(scopeSelector('.i-amphtml-scoped', 'div')) - .to.equal('.i-amphtml-scoped div'); - }); - - it('concats multiple selectors (2)', () => { - expect(scopeSelector(':scope', 'div,ul')) - .to.equal(':scope div,:scope ul'); - }); - - it('concats multiple selectors (4)', () => { - expect(scopeSelector('div >', 'div,ul,ol,section')) - .to.equal('div > div,div > ul,div > ol,div > section'); - }); - - }); - it('should remove all children', () => { const element = document.createElement('div'); element.appendChild(document.createElement('div')); @@ -378,7 +358,7 @@ describes.sandboxed('DOM', {}, env => { it('childElementByTag should find first match', testChildElementByTag); it('childElementByTag should find first match (polyfill)', () => { - dom.setScopeSelectorSupportedForTesting(false); + setScopeSelectorSupportedForTesting(false); testChildElementByTag(); }); @@ -405,7 +385,7 @@ describes.sandboxed('DOM', {}, env => { it('childElementsByTag should find first match', testChildElementsByTag); it('childElementsByTag should find first match (polyfill)', () => { - dom.setScopeSelectorSupportedForTesting(false); + setScopeSelectorSupportedForTesting(false); testChildElementsByTag(); }); @@ -436,7 +416,7 @@ describes.sandboxed('DOM', {}, env => { it('childElementByAttr should find first match', testChildElementByAttr); it('childElementByAttr should find first match', () => { - dom.setScopeSelectorSupportedForTesting(false); + setScopeSelectorSupportedForTesting(false); testChildElementByAttr(); }); @@ -467,7 +447,7 @@ describes.sandboxed('DOM', {}, env => { it('childElementsByAttr should find all matches', testChildElementsByAttr); it('childElementsByAttr should find all matches', () => { - dom.setScopeSelectorSupportedForTesting(false); + setScopeSelectorSupportedForTesting(false); testChildElementsByAttr(); }); @@ -561,7 +541,7 @@ describes.sandboxed('DOM', {}, env => { it('scopedQuerySelector should find first match', testScopedQuerySelector); it('scopedQuerySelector should find first match (polyfill)', () => { - dom.setScopeSelectorSupportedForTesting(false); + setScopeSelectorSupportedForTesting(false); testScopedQuerySelector(); }); @@ -588,7 +568,7 @@ describes.sandboxed('DOM', {}, env => { testScopedQuerySelectorAll); it('scopedQuerySelectorAll should find all matches (polyfill)', () => { - dom.setScopeSelectorSupportedForTesting(false); + setScopeSelectorSupportedForTesting(false); testScopedQuerySelectorAll(); }); @@ -906,13 +886,6 @@ describes.sandboxed('DOM', {}, env => { }); }); - describe('escapeCssSelectorIdent', () => { - - it('should escape', () => { - expect(dom.escapeCssSelectorIdent('a b')).to.equal('a\\ b'); - }); - }); - describe('escapeHtml', () => { it('should tolerate empty string', () => { expect(dom.escapeHtml('')).to.equal('');