add shadow dom v1 support to DOM atoms #5762
Conversation
| "* /deep/ #neverShownDiv")); | ||
|
|
||
| // neverShown is in an older shadow DOM, and thus isn't displayed | ||
| assertFalse(bot.dom.isShown(neverShownDiv)); |
There was a problem hiding this comment.
FYI i removed this test because AFAIK its invalid per the old spec and doesn't apply in the new spec (as createShadowRoot has gone).
this test relies on the fact chrome once allowed you to create a shadow root multiple times, which was removed shortly after. now it just throws an exception or doesn't do anything.
|
unfortunately the tests in most of the tests in there are to assert that "older shadow roots" work correctly. but there is no such thing as an "older shadow root", it was deprecated long ago and throws exceptions in most or all browsers. i tried to strip those out so we can at least have a passing build, but it leaves us with a mess of a test file/suite. so i think if we can at least agree we shouldn't be testing non-existent browser features (removed v0 things only chrome had temporarily), its better i just try to rewrite that test file. |
|
With this patch the test is still broken for me (in chrome): |
|
@barancev to be honest those tests are mostly useless in newer chrome. most of the coverage in there is over this concept of older shadow roots (multiple roots). its not even supported anymore by spec or by browsers. i think the tests just need rewriting completely in that file. ill give that a go tomorrow if i get chance |
43081j
force-pushed
the
visible-text-shadow
branch
3 times, most recently
from
70aadcf to
ae758b0
Compare
|
im unable to get these builds working. i rewrote the tests but locally im having trouble running the build, so i was hoping to see if they pass in CI. though it seems CI builds fail because "chrome is not reachable". can anyone take a look at this? maybe its because chrome 66 was released? |
|
Test results: To run a single test you can start a debug server with a command |
|
ah i wasn't aware you could do that, will give it a go. but you can see the CI builds fail for seemingly unrelated reasons (to my changes). we should probably still figure that out.. |
|
those tests pass now. though CI builds fail still, due to focus tests. they pass locally, so i guess its something to do with the fact the window isn't focused on CI builds maybe. not too sure. also, see my comments in the code review when you have a min if you can |
javascript/atoms/dom.js
Outdated
| } | ||
| } while (elem && elem.nodeType != goog.dom.NodeType.ELEMENT); |
There was a problem hiding this comment.
i removed this loop because elem was never modified, so it would either run once or forever... guess we never ended up with this being as truthy
There was a problem hiding this comment.
I suspect there should be parent instead of elem, looks like a bug.
There was a problem hiding this comment.
we have some recursion below to deal with traversing the tree anyway, so i guess this removal is fine.
javascript/atoms/dom.js
Outdated
| @@ -601,7 +603,7 @@ bot.dom.isShown = function(elem, opt_ignoreOpacity) { | |||
| return false; | |||
| } | |||
| var parent = bot.dom.getParentElement(e); | |||
| return !parent || displayed(parent); | |||
| return parent && displayed(parent); | |||
There was a problem hiding this comment.
does this reversal seem right? it seems before we were assuming a parentless element was visible. we should be assuming the opposite
There was a problem hiding this comment.
Yes, it's correct, an element that has no parent is a top-level element (a body or an svg document), and it is always visible, no matter of its properties.
There was a problem hiding this comment.
an element without a parentNode could be a disconnected node.
also, as far as you go up the chain, there will be a parentNode, until it is #document.
body has a parentNode of document, and we check that elsewhere (we return true somewhere else when node === document). so i dont see a situation where we would end up with a parentless node that is still somehow part of the document.
There was a problem hiding this comment.
This line needs to be reverted: we're walking up parent elements not nodes.
document.body.parentElement === document.documentElement
document.documentElement.parentElement === nullThere was a problem hiding this comment.
we're walking up parent nodes. this calls getParentNodeInComposedDom which (as seen below) uses parentNode
There was a problem hiding this comment.
The line immediately preceding this one:
var parent = bot.dom.getParentElement(e);
getParentElement returns the parent element or null.
There was a problem hiding this comment.
ah my mistake, it shouldn't really be doing either to be honest. an element without a parent is more likely to be a disconnected element than document, its not the best assertion really.
we are better off asserting that it is document or not. if it isn't we do this line as is, otherwise we return true (we do this elsewhere already as part of this, but this line will run first).
There was a problem hiding this comment.
in actual fact @jleyba why do we have this branch at all? getParentNodeInComposedDom works without shadow DOM support (it checks for support internally). why not drop this else completely and use the same function?
There was a problem hiding this comment.
I don't remember the historical reasoning for parentElement vs parentNode, but checking that everything is under a document sounds reasonable to me (pending my other comments for that branch)
There was a problem hiding this comment.
yes and that check already happens as part of the shadow branch. no changes are needed in that case, just the else needs dropping.
javascript/atoms/dom.js
Outdated
| if (parent.shadowRoot && node.assignedSlot !== undefined) { | ||
| // Can be null on purpose, meaning it has no parent as | ||
| // it hasn't yet been slotted | ||
| parent = node.assignedSlot ? node.assignedSlot.parentNode : null; |
There was a problem hiding this comment.
why not just
return node.assignedSlot ? node.assignedSlot.parentNode : null;
There was a problem hiding this comment.
consistency, the existing code didn't do an early return so i didn't either. if you'd rather have an early return, i'll change the other conditions below too.
javascript/atoms/dom.js
Outdated
| @@ -601,7 +603,7 @@ bot.dom.isShown = function(elem, opt_ignoreOpacity) { | |||
| return false; | |||
| } | |||
| var parent = bot.dom.getParentElement(e); | |||
| return !parent || displayed(parent); | |||
| return parent && displayed(parent); | |||
There was a problem hiding this comment.
This line needs to be reverted: we're walking up parent elements not nodes.
document.body.parentElement === document.documentElement
document.documentElement.parentElement === null
javascript/atoms/dom.js
Outdated
| return !parent || displayed(parent); | ||
| } | ||
|
|
||
| if (parent && (parent.nodeType == goog.dom.NodeType.DOCUMENT || |
There was a problem hiding this comment.
Please add some tests to check that nodes within a HTML comment are not shown
javascript/atoms/dom.js
Outdated
| } | ||
|
|
||
| if (parent && (parent.nodeType == goog.dom.NodeType.DOCUMENT || | ||
| parent.nodeType == goog.dom.NodeType.DOCUMENT_FRAGMENT)) { |
There was a problem hiding this comment.
If you're going to require elements be attached to a document to be visible, then wouldn't anything in a document fragment (an incomplete document) not be visible?
Also, can an element be the child of CData? Are those cases handled properly? (you've opened pandora's box by touching one of the gnarliest pieces of code in Selenium...)
javascript/atoms/dom.js
Outdated
|
|
||
| var parent = bot.dom.getParentNodeInComposedDom(e); | ||
|
|
||
| if (parent instanceof ShadowRoot) { |
There was a problem hiding this comment.
If we're going to standardize on a single definition of displayed, then this needs to be
if (bot.dom.IS_SHADOW_DOM_ENABLED && parent instanceof ShadowRoot) {Otherwise the ShadowRoot reference will trigger an error in older browsers
javascript/atoms/dom.js
Outdated
| @@ -601,7 +603,7 @@ bot.dom.isShown = function(elem, opt_ignoreOpacity) { | |||
| return false; | |||
| } | |||
| var parent = bot.dom.getParentElement(e); | |||
| return !parent || displayed(parent); | |||
| return parent && displayed(parent); | |||
There was a problem hiding this comment.
I don't remember the historical reasoning for parentElement vs parentNode, but checking that everything is under a document sounds reasonable to me (pending my other comments for that branch)
|
|
||
| var parent = bot.dom.getParentNodeInComposedDom(e); | ||
|
|
||
| if (bot.dom.IS_SHADOW_DOM_ENABLED && (parent instanceof ShadowRoot)) { |
There was a problem hiding this comment.
@jleyba take a look at this now. so we no longer need an if/else as the parent node traversal already checks if shadow dom v0 or v1 is supported.
and this means now we automatically get the document/shadowRoot assertion further down and disconnected elements are falsey.
javascript/atoms/dom.js
Outdated
| if (bot.dom.getEffectiveStyle(e, 'display') == 'none') { | ||
| // Any element with a display style equal to 'none' or that has an ancestor | ||
| // with display style equal to 'none' is not shown. | ||
| displayed = function(e) { |
There was a problem hiding this comment.
nit: remove the var displayed; declaration above and change this to:
/**
* @param {?Node} e
* @return {boolean}
*/
function displayed(e) {The extra type annotations are annoying, but helps improve the compiler's accuracy in type checking
javascript/atoms/dom.js
Outdated
| // Any element with a display style equal to 'none' or that has an ancestor | ||
| // with display style equal to 'none' is not shown. | ||
| displayed = function(e) { | ||
| if (bot.dom.isElement(e) && bot.dom.getEffectiveStyle(e, 'display') == 'none') { |
There was a problem hiding this comment.
Should this be
!bot.dom.isElement(e) || bot.dom.getEffectiveStyle(e, 'display') == 'none'
Since this function should only be called when checking the visibility of a parent, we're assuming the DOM is well-formed and e will never be a text node.
There was a problem hiding this comment.
i would rather keep it as is to be honest. if its an element, we check the style, otherwise we continue traversing. we shouldn't really be making assumptions on what can and can't be a parent, but instead just traversing what we are given IMO.
in cases when its not an element, we continue traversing. its very simple this way with no assumptions.
43081j
changed the title
43081j
force-pushed
the
visible-text-shadow
branch
from
0618019 to
8da6a65
Compare
|
this seems ready now, one last look over it if you could? current CI build fails for seemingly unrelated reasons. looks like the failing tests are all focus related, i can reproduce them locally by having chrome unfocused (certain events dont fire if chrome has no focus as a whole). |
|
much appreciated. thanks to you all for the help, reviews, etc. this'll be of great use to a whole bunch of people |
ghost
mentioned this pull request
Xin the preceding checkbox, I verify that I have signed the Contributor License AgreementThis fixes #5761.
Though it raises another issue: that in several places we only support shadow dom v0, rather than the v1 spec. most of our tests use v0 too, so we don't test against v1 apis like
attachShadowetc.