fluent-dom: Simplify DOM Overlays #71
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stasm
changed the title
stasm
force-pushed
the
overlay-only-allowed
branch
from
74279a9
to
0ff6d0b
Compare
zbraniecki
approved these changes
Aug 30, 2017
Pike
approved these changes
Aug 30, 2017
fluent-dom/src/overlay.js
Outdated
| * The goal of overlay is to move the children of `translationElement` | ||
| * into `sourceElement` such that `sourceElement`'s own children are not | ||
| * replaced, but only have their text nodes and their attributes modified. | ||
| * Sanitize the `translationElement` according to the following rules: |
There was a problem hiding this comment.
translationFragment 'cause that's what the paramater is.
fluent-dom/src/overlay.js
Outdated
| const result = translationElement.ownerDocument.createDocumentFragment(); | ||
|
|
||
| // Take one node from translationElement at a time and check it against | ||
| function overlay(translationFragment, sourceElement) { |
fluent-dom/src/overlay.js
Outdated
| } | ||
| // If a child of the same type exists in sourceElement, use it as the base | ||
| // for the resultChild. | ||
| const sourceChild = popChild(sourceElement, childNode.localName); |
There was a problem hiding this comment.
I know I said the opposite, but how about shiftElementOfName ?
fluent-dom/src/overlay.js
Outdated
| for (const attr of Array.from(translationElement.attributes)) { | ||
| if (isAttrNameAllowed(attr.name, sourceElement)) { | ||
| sourceElement.setAttribute(attr.name, attr.value); | ||
| // Explicitly ignore nested HTML. |
There was a problem hiding this comment.
This comment means well, but you still have to know what the code does per spec.
How about // Serialize to a single TextNode
fluent-dom/test/overlay_test.js
Outdated
| assert.equal(element.innerHTML, 'FOO <em>BAR</em> BAZ'); | ||
| }); | ||
|
|
||
| test('allowed element with nested HTML', function() { |
There was a problem hiding this comment.
... with forbidden nested HTML'.
I read this as "allowed ..yadda yadda"
Make DOM overlay logic less complex and more predictable by making the
following changes:
- Only children of the white-listed types are allowed now. It's not possible
anymore to put elements of other types in the source HTML to make exceptions.
- The identity of the source element's children is explicitly not kept
anymore. This allows us to treat the translation DocumentFragment as the
reference for iteration over child nodes.
- The overlay function is also no longer recursive. Any nested HTML
will be lost and only its textContent will be preserved.
stasm
force-pushed
the
overlay-only-allowed
branch
from
462cb70
to
40be935
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a WIP. I'm working on a second commit which changes the underlying logic even further.This PR reduces the complexity of the overlay logic by making the following changes:
Only children of the white-listed types are allowed now. It's not possible anymore to put elements of other types in the source HTML to make exceptions.
The identity of the source element's children is explicitly not kept anymore. This allows us to treat the translation
DocumentFragmentas the reference for iteration over child nodes.The
overlayfunction is also no longer recursive. Any nested HTML will be lost and only itstextContentwill be preserved.