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

Coreify specific layout.js, CE, and BE functions #34810

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions src/base-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
import {ActionTrust, DEFAULT_ACTION} from './core/constants/action-constants';
import {Layout, LayoutPriority} from './layout';
import {Services} from './services';
import {applyFillContent, dispatchCustomEvent} from './core/dom';
import {devAssert, user, userAssert} from './log';
import {dispatchCustomEvent} from './core/dom';
import {getData, listen, loadPromise} from './event-helper';
import {getMode} from './mode';
import {isArray} from './core/types';
Expand Down Expand Up @@ -869,22 +869,14 @@ export class BaseElement {
}

/**
* Configures the supplied element to have a "fill content" layout. The
* exact interpretation of "fill content" depends on the element's layout.
*
* If `opt_replacedContent` is specified, it indicates whether the "replaced
* content" styling should be applied. Replaced content is not allowed to
* have its own paddings or border.
* See src/core/dom.js for full description.
*
* @param {!Element} element
* @param {boolean=} opt_replacedContent
* @public @final
*/
applyFillContent(element, opt_replacedContent) {
element.classList.add('i-amphtml-fill-content');
if (opt_replacedContent) {
element.classList.add('i-amphtml-replaced-content');
}
applyFillContent(element, opt_replacedContent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense for applyFillContent to be defined locally in this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Still relevant, since we don't really want other code using this instead of element.applyFillContent. Including it in core kinda implies it's fine, whereas no one will go out of their way to import it from here (except the compiler)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'm fine with that

}

/**
Expand Down
34 changes: 33 additions & 1 deletion src/core/dom/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@
* limitations under the License.
*/

import {childNodes, matches} from './query';
import {dict} from '../types/object';
import {matches} from './query';
import {isInternalOrServiceNode} from '../layout';
import {toWin} from '../window';

const HTML_ESCAPE_CHARS = {
Expand Down Expand Up @@ -536,3 +537,34 @@ export function dispatchCustomEvent(node, name, opt_data, opt_options) {
export function containsNotSelf(parent, child) {
return child !== parent && parent.contains(child);
}

/**
* Configures the supplied element to have a "fill content" layout. The
* exact interpretation of "fill content" depends on the element's layout.
*
* If `opt_replacedContent` is specified, it indicates whether the "replaced
* content" styling should be applied. Replaced content is not allowed to
* have its own paddings or border.
*
* @param {!Element} element
* @param {boolean=} opt_replacedContent
* @public @final
*/
export function applyFillContent(element, opt_replacedContent) {
element.classList.add('i-amphtml-fill-content');
rcebulko marked this conversation as resolved.
Show resolved Hide resolved
if (opt_replacedContent) {
element.classList.add('i-amphtml-replaced-content');
}
}

/**
* Returns the original nodes of the custom element without any service
* nodes that could have been added for markup. These nodes can include
* Text, Comment and other child nodes.
*
* @param {!Node} element
* @return {!Array<!Node>}
*/
export function getRealChildNodes(element) {
return childNodes(element, (node) => !isInternalOrServiceNode(node));
}
41 changes: 41 additions & 0 deletions src/core/layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* 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 {devAssertElement} from './assert';

/**
* Returns "true" for internal AMP nodes or for placeholder elements.
* @param {!Node} node
* @return {boolean}
*/
export function isInternalOrServiceNode(node) {
if (node.nodeType !== Node.ELEMENT_NODE) {
return false;
}
node = devAssertElement(node);

if (node.tagName.toLowerCase().startsWith('i-')) {
return true;
}
if (
node.hasAttribute('placeholder') ||
node.hasAttribute('fallback') ||
node.hasAttribute('overflow')
) {
return true;
}
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! More core! I think it would make sense for this whole file to live in src/core/dom/layout, with the only exception being the applyStaticLayout helpers that have dependencies on the experiment system

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! But that requires changing 100s of files import paths and so I'd rather do it separately since my main goal is to unblock #34770

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, this is only used in custom-element. What's the motivation for moving it into core, rather than closer to CE?

Copy link
Contributor

Choose a reason for hiding this comment

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

Created #34818 to unblock any dependencies on layout; diff looks like 2ee75c4...cc4d48b excluding the import updates and actual moves

Copy link
Member Author

@samouri samouri Jun 10, 2021

Choose a reason for hiding this comment

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

Compiler will need to run it, and we don't want compiler to need an instance of CustomElement to run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just a helper in custom-element.js then?

33 changes: 7 additions & 26 deletions src/custom-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
Layout,
LayoutPriority,
applyStaticLayout,
isInternalElement,
isLoadingAllowed,
} from './layout';
import {MediaQueryProps} from './core/dom/media-query-props';
Expand All @@ -47,6 +46,7 @@ import {getIntersectionChangeEntry} from './utils/intersection-observer-3p-host'
import {getMode} from './mode';
import {getSchedulerForDoc} from './service/scheduler';
import {isExperimentOn} from './experiments';
import {isInternalOrServiceNode} from './core/layout';
import {rethrowAsync} from './core/error';
import {setStyle} from './core/dom/style';
import {shouldBlockOnConsentByMeta} from './consent';
Expand Down Expand Up @@ -1897,14 +1897,15 @@ function createBaseCustomElementClass(win, elementConnectedCallback) {
}

/**
* Returns the original nodes of the custom element without any service
* nodes that could have been added for markup. These nodes can include
* Text, Comment and other child nodes.
* @return {!Array<!Node>}
* See dom.getRealChildNodes for the full description.
* TODO: migrate to the fn in dom.js.
* @deprecated
* @package @final
*
* @return {!Array<!Node>}
*/
getRealChildNodes() {
return query.childNodes(this, (node) => !isInternalOrServiceNode(node));
return dom.getRealChildNodes(this);
rcebulko marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -2171,26 +2172,6 @@ function assertNotTemplate(element) {
devAssert(!element.isInTemplate_, 'Must never be called in template');
}

/**
* Returns "true" for internal AMP nodes or for placeholder elements.
* @param {!Node} node
* @return {boolean}
*/
function isInternalOrServiceNode(node) {
if (isInternalElement(node)) {
return true;
}
if (
node.tagName &&
(node.hasAttribute('placeholder') ||
node.hasAttribute('fallback') ||
node.hasAttribute('overflow'))
) {
return true;
}
return false;
}

/**
* Creates a new custom element class prototype.
*
Expand Down
10 changes: 0 additions & 10 deletions src/layout.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,16 +173,6 @@ export function isLayoutSizeFixed(layout) {
return layout == Layout.FIXED || layout == Layout.FIXED_HEIGHT;
}

/**
* Whether the tag is an internal (service) AMP tag.
* @param {!Node|string} tag
* @return {boolean}
*/
export function isInternalElement(tag) {
const tagName = typeof tag == 'string' ? tag : tag.tagName;
return tagName && tagName.toLowerCase().startsWith('i-');
}

/**
* Parses the CSS length value. If no units specified, the assumed value is
* "px". Returns undefined in case of parsing error.
Expand Down