Conversation
bc1122c to
33f26f5
Compare
src/dom.js
Outdated
| // 8 | 2 = 0x0A | ||
| // 2 - preceeding | ||
| // 8 - contains | ||
| if (pos & 0x0A) { |
There was a problem hiding this comment.
// file scope
const PRECEDING_OR_CONTAINS =
Node.DOCUMENT_POSITION_PRECEDING | Node.DOCUMENT_POSITION_CONTAINS;if (pos & PRECEDING_OR_CONTAINS) {|
|
||
| // Sort in document order. | ||
| this.sortInDomOrder_(this.elements_); | ||
| this.sortInDomOrder_(); |
There was a problem hiding this comment.
Nit: I liked seeing what's being sorted. Update comment at least?
There was a problem hiding this comment.
There's no need to pass instance variables to an instance function.
There was a problem hiding this comment.
Just a readability nit. I.e. what are we sorting? All elements? Some elements? Something else?
src/service/fixed-layer.js
Outdated
| }); | ||
| this.elements_.splice(i, 1); | ||
| removed.push(fe); | ||
| return fe; |
There was a problem hiding this comment.
Do we prevent an element from being added more than once? Looking at setupElement_ it doesn't seem like it.
There was a problem hiding this comment.
Yes, we attempt to find the element's fe. If it's found, we update, else we create a new one.
There was a problem hiding this comment.
amphtml/src/service/fixed-layer.js
Line 493 in 33f26f5
Does adding a single element twice with different position result in two fes?
There was a problem hiding this comment.
Undid these changes.
f52e71c to
8780d7d
Compare
fbf861e to
3a064f7
Compare
* Cleanup fixed-layer * Extract DOM order comparator * Undo removeElement changes * Fix lint
domOrderComparatorfrom 🐛 Chrome 70 array.sort uses stable TimSort which exposed bunch of bugs #18777matcheshelper function insrc/dom.js