Wait for amp selector to load before submissions#6951
Wait for amp selector to load before submissions#6951mkhatib wants to merge 3 commits intoampproject:masterfrom
Conversation
extensions/amp-form/0.1/amp-form.js
Outdated
| const usedDeps = this.form_.querySelectorAll(EXTERNAL_DEPS.join(',')); | ||
| const depsDict = {}; | ||
| const depsPromises = []; | ||
| for (let i = usedDeps.length - 1; i >= 0; i--) { |
There was a problem hiding this comment.
you should not have to loop through all matching elements.
You can loop through EXTERNAL_DEPS and check of getElementsByTagName(tag).length > 0 then wait for the tag.
There was a problem hiding this comment.
I guess now we only have one dependency getElementsByTagName is fine. Not sure if querySelectorAll would be faster than multiple getElementsByTagName in the future.
Will update this to use getElementsByTagName.
There was a problem hiding this comment.
you will still have to do a tagName read with queryseletorall in a loop (atleast one per the tag in the array). I still think what i suggested is slightly better and clean
3e9e1b5 to
096abbd
Compare
|
@camelburrito PTAL 👀 |
| const tagName = EXTERNAL_DEPS[i]; | ||
| const depIsUsed = this.form_.getElementsByTagName(tagName).length !== 0; | ||
| if (depIsUsed && depsDict[tagName] === undefined) { | ||
| // TODO(mkhatib, #7032): Implement a way to wait for an element to be built to |
There was a problem hiding this comment.
@dvoytenko I prefer to do this in a separate PR if that's ok. lmk.
There was a problem hiding this comment.
I think that the implementation would be drastically different and we should consider doing build promise work first.
There was a problem hiding this comment.
sgtm. will leave this PR until I have something for the build promise.
| this.actions_.installActionHandler( | ||
| this.form_, this.actionHandler_.bind(this)); | ||
| // Wait for amp-selector to finish loading if the current form is using it. | ||
| this.whenDependenciesReady_().then(() => { |
There was a problem hiding this comment.
I'm not sure it makes sense to wait to install action handler here, esp since we have other listeners that could override the behavior. Should, instead, action handler be installed right away, but block on dependencies?
There was a problem hiding this comment.
esp since we have other listeners that could override the behavior
You mean other submit listeners? This would still be very similar situation with blocking on dependencies there are still other listeners.
If you meant other action handlers yeah if some other code install an action listener on the form element these submit events will be gone from the queue.
| const tagName = EXTERNAL_DEPS[i]; | ||
| const depIsUsed = this.form_.getElementsByTagName(tagName).length !== 0; | ||
| if (depIsUsed && depsDict[tagName] === undefined) { | ||
| // TODO(mkhatib, #7032): Implement a way to wait for an element to be built to |
There was a problem hiding this comment.
I think that the implementation would be drastically different and we should consider doing build promise work first.
| */ | ||
| function checkUserValidity(element, propagate = false) { | ||
| // If this is not a field type with checkValidity don't do anything. | ||
| if (!element.checkValidity) { |
There was a problem hiding this comment.
Is the assumption that amp-selector somehow will implement this interface? Do we have issues that would track this work? In particular required is very sensible for amp-selector.
There was a problem hiding this comment.
Yes I filed an issue to track this #6930
Fixes #6927
This depends on #6929