Add scopedQuerySelector helper #7260

jridgewell · 2017-01-30T22:19:26Z

This fixes (and forbids future) element scoped #querySelector calls. Anytime you select a descendant of a descendant (div div, or, really, * *), your matching against the entire document then filtering to elements inside the scope. This is not what you'd expect.

var parent = document.createElement('div');

var element = document.createElement('span');
parent.appendChild(element);

var a = document.createElement('a');
element.appendChild(a);

// WHAT!?
element.querySelector('div a'); // => a

Instead, we'll use the :scope CSS selector where supported (everywhere but IE), and will quickly mutate and un-mutate the element in IE to accomplish a global, unique scope selector.

scopedQuerySelector(element, 'div a'); // => null

Fixes a selector bug in <amp-analytics>. /cc @avimehta
/cc @tlong2

dvoytenko · 2017-01-31T18:52:37Z

src/dom.js

-  });
+
+  // Only IE.
+  const unique = `i-amphtml-scoped-${Math.random()}`;


That includes the '.' (dot). Is it prudent to have it there given selectors below?

Good catch, changed to use Date.now

dvoytenko · 2017-01-31T19:09:31Z

src/dom.js

+ * @param {string} selector
+ * @return {?Element}
+ */
+export function scopedQuerySelector(root, selector) {


We initially decided to hold this off because of https://bugs.chromium.org/p/chromium/issues/detail?id=222028

This isn't due to :scope, but that there are now two selectors to match against (.selector1 vs .selector1 .selector2).

I'm not requiring scopedQuerySelector where we can avoid it (it's fine if you're doing scope.querySelector('selector')). The only time it's necessary is when there's a descendant selector included (scope.querySelector('parent descendant')), which is already slow.

dvoytenko · 2017-01-31T21:13:33Z

extensions/amp-ad/0.1/amp-ad-custom.js

@@ -136,7 +136,7 @@ export class AmpAdCustom extends AMP.BaseElement {

      // Get the parent body of this amp-ad element. It could be the body of
      // the main document, or it could be an enclosing iframe.
-      const body = ancestorElementsByTag(this.element, 'BODY')[0];
+      const body = closestByTag(this.element, 'BODY');


Hmm. This doesn't look right. Though I guess not the subject to your PR. Would you mind leaving a TODO here for me to investigate?

I'll split out of the PR.

dvoytenko · 2017-01-31T21:21:11Z

extensions/amp-analytics/0.1/visibility-impl.js

@@ -177,7 +181,7 @@ export function getElement(ampdoc, selector, analyticsEl, selectionMethod) {
    // Only tag names are supported currently.
    foundEl = closestByTag(analyticsEl, selector);
  } else if (selectionMethod == 'scope') {
-    foundEl = analyticsEl.parentElement.querySelector(selector);
+    foundEl = scopedQuerySelector(analyticsEl.parentElement, selector);


There's the same thing in the analytics-root. This code is actually waiting to be killed in preference of that. Can you change there as well?

dvoytenko · 2017-01-31T21:23:20Z

src/custom-element.js

@@ -1362,8 +1362,9 @@ function createBaseCustomElementClass(win) {
     * @package @final @this {!Element}
     */
    getRealChildren() {
-      return dom.childElements(this, element =>
-        !isInternalOrServiceNode(element));
+      return dom.childElementByTag(this, '*').filter(element => {


This is relatively unclear that ByTag can do wildcards... E.g. does ELement.getElementsByTagName do wildcards?

Another question: is querying here really faster than first-child -> siblings walk?

I was after code size here, not speed. I'll split out the childElements changes.

jridgewell

PTAL.

jridgewell · 2017-01-31T23:59:28Z

extensions/amp-analytics/0.1/visibility-impl.js

@@ -177,7 +181,7 @@ export function getElement(ampdoc, selector, analyticsEl, selectionMethod) {
    // Only tag names are supported currently.
    foundEl = closestByTag(analyticsEl, selector);
  } else if (selectionMethod == 'scope') {
-    foundEl = analyticsEl.parentElement.querySelector(selector);
+    foundEl = scopedQuerySelector(analyticsEl.parentElement, selector);


cramforce · 2017-02-01T01:29:38Z

src/dom.js

-  });
+
+  // Only IE.
+  const unique = `i-amphtml-scoped-${Date.now()}`;


better to increment an integer. This can very much be invoked twice in the same milli

It's removed immediately, so it'll still be unique. Really, all the class name needs is "i-amphtml-scoped".

cramforce · 2017-02-01T01:30:26Z

src/dom.js

+    scopeSelectorSupported = isScopeSelectorSupported(root);
+  }
+  if (scopeSelectorSupported) {
+    return toArray(root./*OK*/querySelectorAll(`:scope ${selector}`));


Do we really need to toArray?

If they call #filter, etc.

Are there existing consumers that do that?

Only one. Updated it to a for loop.

Anytime you select a descendant of a descendant (`div div`, or, really, `* *`), your matching against the entire document then filtering to elements inside the scope. This is **not** what you'd expect. Fixes a selector bug in `<amp-analytics>`.

jridgewell · 2017-02-01T23:13:00Z

Ping @dvoytenko

* Eliminate uses of childElement and childElements * Forbid uses of buggy querySelector Anytime you select a descendant of a descendant (`div div`, or, really, `* *`), your matching against the entire document then filtering to elements inside the scope. This is **not** what you'd expect. Fixes a selector bug in `<amp-analytics>`. * Add tests * Use `Date.now` to avoid `.` class selector conflict * Merge latest master * Linting * Fix types * Fix merge conflict * Return NodeList * Fix tests

jridgewell requested review from dvoytenko and camelburrito January 30, 2017 22:19

jridgewell mentioned this pull request Jan 30, 2017

Adding sub-anchor support to <amp-auto-ads> #7218

Merged

jridgewell force-pushed the querySelector-scope branch from d13d181 to f05d61e Compare January 30, 2017 22:21

dvoytenko suggested changes Jan 31, 2017

View reviewed changes

jridgewell force-pushed the querySelector-scope branch from 5d9a2af to dcf0a09 Compare January 31, 2017 23:57

jridgewell commented Feb 1, 2017

View reviewed changes

cramforce requested changes Feb 1, 2017

View reviewed changes

jridgewell added 8 commits January 31, 2017 20:44

Eliminate uses of childElement and childElements

b4f4ce3

Forbid uses of buggy querySelector

938bde4

Anytime you select a descendant of a descendant (`div div`, or, really, `* *`), your matching against the entire document then filtering to elements inside the scope. This is **not** what you'd expect. Fixes a selector bug in `<amp-analytics>`.

Add tests

52ae4dd

Use Date.now to avoid . class selector conflict

2d233db

Merge latest master

80dea25

Linting

b1f0ed5

Fix types

33d6057

Fix merge conflict

bc6c26e

jridgewell force-pushed the querySelector-scope branch from 9c0989f to bc6c26e Compare February 1, 2017 01:45

Return NodeList

96f2033

jridgewell force-pushed the querySelector-scope branch from f4e6699 to 2ac19e9 Compare February 1, 2017 21:39

cramforce approved these changes Feb 1, 2017

View reviewed changes

Fix tests

14990b7

jridgewell force-pushed the querySelector-scope branch from 2ac19e9 to 14990b7 Compare February 1, 2017 22:42

dvoytenko approved these changes Feb 1, 2017

View reviewed changes

jridgewell merged commit 57810d9 into ampproject:master Feb 2, 2017

jridgewell mentioned this pull request May 1, 2017

Implement form verification (async form validation) #9054

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scopedQuerySelector helper #7260

Add scopedQuerySelector helper #7260

jridgewell commented Jan 30, 2017 •

edited

Loading

dvoytenko Jan 31, 2017

jridgewell Jan 31, 2017

dvoytenko Jan 31, 2017

jridgewell Jan 31, 2017

dvoytenko Jan 31, 2017

jridgewell Jan 31, 2017

dvoytenko Jan 31, 2017

jridgewell Jan 31, 2017

dvoytenko Jan 31, 2017

jridgewell Jan 31, 2017

jridgewell left a comment

jridgewell Jan 31, 2017

cramforce Feb 1, 2017

jridgewell Feb 1, 2017

cramforce Feb 1, 2017

jridgewell Feb 1, 2017

cramforce Feb 1, 2017

jridgewell Feb 1, 2017

jridgewell commented Feb 1, 2017

Add scopedQuerySelector helper #7260

Add scopedQuerySelector helper #7260

Conversation

jridgewell commented Jan 30, 2017 • 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

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

jridgewell left a 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

jridgewell commented Feb 1, 2017

jridgewell commented Jan 30, 2017 •

edited

Loading