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

Update atoms sha #1673

Merged
merged 1 commit into from Oct 13, 2022
Merged

Update atoms sha #1673

merged 1 commit into from Oct 13, 2022

Conversation

AutomatedTester
Copy link
Contributor

@AutomatedTester AutomatedTester commented Jul 8, 2022

@whimboo
Copy link
Contributor

whimboo commented Jul 8, 2022

I think it would be good to say that this change is required to get the ShadowRoot support into the exported atoms.

@jgraham and @shs96c do you have any objections?

Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

So this an unknown amount of behvaiour change to the existing spec. It seems like chromedriver updated to ec9202eceb8b8e334ceee76e4670154c1aaf89a6 which isn't either the spec revision or the proposed new revision.

I think this probably requires explicit buy-in from all current remote-end implementors that they're OK with the update cc @sadym-chromium (nad I"m not sure who to ask for WebKit these days).

@jgraham
Copy link
Member

jgraham commented Jul 11, 2022

diff --git a/javascript/atoms/dom.js b/javascript/atoms/dom.js
index f0a3b84da5..d4a9aae978 100644
--- a/javascript/atoms/dom.js
+++ b/javascript/atoms/dom.js
@@ -48,9 +48,9 @@ bot.dom.IS_SHADOW_DOM_ENABLED = (typeof ShadowRoot === 'function');
 
 /**
  * Retrieves the active element for a node's owner document.
- * @param {!(Node|Window)} nodeOrWindow The node whose owner document to get
+ * @param {(!Node|!Window)} nodeOrWindow The node whose owner document to get
  *     the active element for.
- * @return {Element} The active element, if any.
+ * @return {?Element} The active element, if any.
  */
 bot.dom.getActiveElement = function(nodeOrWindow) {
   var active = goog.dom.getActiveElement(
@@ -166,7 +166,7 @@ bot.dom.getAttribute = bot.dom.core.getAttribute;
 /**
  * List of elements that support the "disabled" attribute, as defined by the
  * HTML 4.01 specification.
- * @private {!Array.<goog.dom.TagName>}
+ * @private {!Array.<!goog.dom.TagName>}
  * @const
  * @see http://www.w3.org/TR/html401/interact/forms.html#h-17.12.1
  */
@@ -290,6 +290,21 @@ bot.dom.isFileInput = function(element) {
 };
 
 
+/**
+ * @param {!Element} element The element to check.
+ * @param {string} inputType The type of input to check.
+ * @return {boolean} Whether the element is an input with specified type.
+ */
+bot.dom.isInputType = function(element, inputType) {
+  if (bot.dom.isElement(element, goog.dom.TagName.INPUT)) {
+    var type = element.type.toLowerCase();
+    return type == inputType;
+  }
+
+  return false;
+};
+
+
 /**
  * @param {!Element} element The element to check.
  * @return {boolean} Whether the element is contentEditable.
@@ -331,7 +346,15 @@ bot.dom.isContentEditable = function(element) {
  * @return {boolean} Whether the element accepts user-typed text.
  */
 bot.dom.isEditable = function(element) {
-  return (bot.dom.isTextual(element) || bot.dom.isFileInput(element)) &&
+  return (bot.dom.isTextual(element) ||
+          bot.dom.isFileInput(element) ||
+          bot.dom.isInputType(element, 'range') ||
+          bot.dom.isInputType(element, 'date') ||
+          bot.dom.isInputType(element, 'month') ||
+          bot.dom.isInputType(element, 'week') ||
+          bot.dom.isInputType(element, 'time') ||
+          bot.dom.isInputType(element, 'datetime-local') ||
+          bot.dom.isInputType(element, 'color')) &&
       !bot.dom.getProperty(element, 'readOnly');
 };
 
@@ -548,45 +571,47 @@ bot.dom.isShown_ = function(elem, ignoreOpacity, parentsDisplayedFn) {
  * @return {boolean} Whether or not the element is visible.
  */
 bot.dom.isShown = function(elem, opt_ignoreOpacity) {
-  var displayed;
-
-  if (bot.dom.IS_SHADOW_DOM_ENABLED) {
-    // 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.getEffectiveStyle(e, 'display') == 'none') {
+  /**
+   * Determines whether an element or its parents have `display: none` set
+   * @param {!Node} e the element
+   * @return {!boolean}
+   */
+  function displayed(e) {
+    if (bot.dom.isElement(e)) {
+      var elem = /** @type {!Element} */ (e);
+      if (bot.dom.getEffectiveStyle(elem, 'display') == 'none') {
         return false;
       }
-      var parent;
-      do {
-        parent = bot.dom.getParentNodeInComposedDom(e);
-        if (parent instanceof ShadowRoot) {
-          if (parent.host.shadowRoot != parent) {
-            // There is a younger shadow root, which will take precedence over
-            // the shadow this element is in, thus this element won't be
-            // displayed.
-            return false;
-          } else {
-            parent = parent.host;
-          }
-        } else if (parent && (parent.nodeType == goog.dom.NodeType.DOCUMENT ||
-            parent.nodeType == goog.dom.NodeType.DOCUMENT_FRAGMENT)) {
-          parent = null;
-        }
-      } while (elem && elem.nodeType != goog.dom.NodeType.ELEMENT);
-      return !parent || displayed(parent);
-    };
-  } else {
-    // 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.getEffectiveStyle(e, 'display') == 'none') {
+    }
+
+    var parent = bot.dom.getParentNodeInComposedDom(e);
+
+    if (bot.dom.IS_SHADOW_DOM_ENABLED && (parent instanceof ShadowRoot)) {
+      if (parent.host.shadowRoot !== parent) {
+        // There is a younger shadow root, which will take precedence over
+        // the shadow this element is in, thus this element won't be
+        // displayed.
         return false;
+      } else {
+        parent = parent.host;
       }
-      var parent = bot.dom.getParentElement(e);
-      return !parent || displayed(parent);
-    };
+    }
+
+    if (parent && (parent.nodeType == goog.dom.NodeType.DOCUMENT ||
+        parent.nodeType == goog.dom.NodeType.DOCUMENT_FRAGMENT)) {
+      return true;
+    }
+
+    // Child of DETAILS element is not shown unless the DETAILS element is open
+    // or the child is a SUMMARY element.
+    if (parent && bot.dom.isElement(parent, goog.dom.TagName.DETAILS) &&
+        !parent.open && !bot.dom.isElement(e, goog.dom.TagName.SUMMARY)) {
+      return false;
+    }
+
+    return !!parent && displayed(parent);
   }
+
   return bot.dom.isShown_(elem, !!opt_ignoreOpacity, displayed);
 };
 
@@ -645,10 +670,11 @@ bot.dom.getOverflowState = function(elem, opt_region) {
       if (container == htmlElem) {
         return true;
       }
-      // An element cannot overflow an element with an inline display style.
+      // An element cannot overflow an element with an inline or contents display style.
       var containerDisplay = /** @type {string} */ (
           bot.dom.getEffectiveStyle(container, 'display'));
-      if (goog.string.startsWith(containerDisplay, 'inline')) {
+      if (goog.string.startsWith(containerDisplay, 'inline') ||
+          (containerDisplay == 'contents')) {
         return false;
       }
       // An absolute-positioned element cannot overflow a static-positioned one.
@@ -1149,7 +1175,9 @@ bot.dom.appendVisibleTextLinesFromTextNode_ = function(textNode, lines,
   }
 
   if (textTransform == 'capitalize') {
-    text = text.replace(/(^|\s)(\S)/g, function() {
+    // the unicode regex ending with /gu does not work in IE
+    var re = goog.userAgent.IE ? /(^|\s|\b)(\S)/g : /(^|[^\d\p{L}\p{S}])([\p{Ll}|\p{S}])/gu;
+    text = text.replace(re, function() {
       return arguments[1] + arguments[2].toUpperCase();
     });
   } else if (textTransform == 'uppercase') {
@@ -1243,12 +1271,22 @@ bot.dom.getOpacityNonIE_ = function(elem) {
  */
 bot.dom.getParentNodeInComposedDom = function(node) {
   var /**@type {Node}*/ parent = node.parentNode;
+
+  // Shadow DOM v1
+  if (parent && parent.shadowRoot && node.assignedSlot !== undefined) {
+    // Can be null on purpose, meaning it has no parent as
+    // it hasn't yet been slotted
+    return node.assignedSlot ? node.assignedSlot.parentNode : null;
+  }
+
+  // Shadow DOM V0 (deprecated)
   if (node.getDestinationInsertionPoints) {
     var destinations = node.getDestinationInsertionPoints();
     if (destinations.length > 0) {
-      parent = destinations[destinations.length - 1];
+      return destinations[destinations.length - 1];
     }
   }
+
   return parent;
 };
 
@@ -1272,16 +1310,22 @@ bot.dom.appendVisibleTextLinesFromNodeInComposedDom_ = function(
   } else if (bot.dom.isElement(node)) {
     var castElem = /** @type {!Element} */ (node);
 
-    if (bot.dom.isElement(node, 'CONTENT')) {
+    if (bot.dom.isElement(node, 'CONTENT') || bot.dom.isElement(node, 'SLOT')) {
       var parentNode = node;
       while (parentNode.parentNode) {
         parentNode = parentNode.parentNode;
       }
       if (parentNode instanceof ShadowRoot) {
-        // If the element is <content> and we're inside a shadow DOM then just 
+        // If the element is <content> and we're inside a shadow DOM then just
         // append the contents of the nodes that have been distributed into it.
         var contentElem = /** @type {!Object} */ (node);
-        goog.array.forEach(contentElem.getDistributedNodes(), function(node) {
+        var shadowChildren;
+        if (bot.dom.isElement(node, 'CONTENT')) {
+          shadowChildren = contentElem.getDistributedNodes();
+        } else {
+          shadowChildren = contentElem.assignedNodes();
+        }
+        goog.array.forEach(shadowChildren, function(node) {
           bot.dom.appendVisibleTextLinesFromNodeInComposedDom_(
               node, lines, shown, whitespace, textTransform);
         });
@@ -1336,8 +1380,10 @@ bot.dom.isNodeDistributedIntoShadowDom = function(node) {
     elemOrText = /** @type {!Text} */ (node);
   }
   return elemOrText != null &&
-      elemOrText.getDestinationInsertionPoints &&
-      elemOrText.getDestinationInsertionPoints().length > 0;
+      (elemOrText.assignedSlot != null ||
+        (elemOrText.getDestinationInsertionPoints &&
+        elemOrText.getDestinationInsertionPoints().length > 0)
+      );
 };

@whimboo
Copy link
Contributor

whimboo commented Aug 9, 2022

ping @sadym-chromium for getting feedback from Google.

@sadym-chromium
Copy link
Contributor

Adding @vladimir-nechaev who is working on ChromeDriver Classic

@sadym-chromium
Copy link
Contributor

diff --git a/javascript/atoms/dom.js b/javascript/atoms/dom.js

Thanks for providing the diff!

@vladimir-nechaev
Copy link

vladimir-nechaev commented Aug 12, 2022

I have updated the atoms to a6b161a159c3d581b130f03a2e6e35f577f38dec and run the code through the unit and integration tests.
Looks to be working fine: https://chromium-review.googlesource.com/c/chromium/src/+/3827746
Currently running WPT.

@vladimir-nechaev
Copy link

WPT show no difference. Looks to be ok.

@whimboo
Copy link
Contributor

whimboo commented Aug 12, 2022

@gsnedders do you know who is working on safaridriver these days and could review this change as well?

@whimboo
Copy link
Contributor

whimboo commented Aug 26, 2022

ping @gsnedders and @shs96c. Thanks!

@whimboo
Copy link
Contributor

whimboo commented Aug 31, 2022

Looks like @patrickangle is the one to ask here for the safaridriver related update. Please let us know if that update is fine with you. Thanks!

Copy link

@patrickangle patrickangle left a comment

Choose a reason for hiding this comment

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

This seems fine.

index.html Outdated Show resolved Hide resolved
@@ -4346,28 +4346,28 @@ <h3 id=shadow-root>Shadow Roots</h3>
is an abstraction used to identify a <a>shadow root</a>
when it is transported via the <a href="#protocol">protocol</a>,
between <a>remote</a> and <a>local</a> ends.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we split the whitespace changes into a different commit?

@whimboo
Copy link
Contributor

whimboo commented Sep 9, 2022

@AutomatedTester mind updating your PR regarding the recent review comments?

@whimboo
Copy link
Contributor

whimboo commented Sep 23, 2022

Hi @jgraham as it looks like everyone is fine with the update of the atoms. Can we get it merged?

@whimboo
Copy link
Contributor

whimboo commented Oct 13, 2022

Thanks James! I've triggered a try build in our CI with the proposed changes of the atoms included. If all tests pass we can merge this PR:

https://treeherder.mozilla.org/jobs?repo=try&revision=efddca836d3e1fda59ea3f8cf5bf0d9b3497a576

@whimboo
Copy link
Contributor

whimboo commented Oct 13, 2022

Thanks James! I've triggered a try build in our CI with the proposed changes of the atoms included. If all tests pass we can merge this PR:

https://treeherder.mozilla.org/jobs?repo=try&revision=efddca836d3e1fda59ea3f8cf5bf0d9b3497a576

Everything is fine here. I'm going ahead and merge the PR. Thanks @AutomatedTester!

@whimboo whimboo merged commit 0707ea4 into master Oct 13, 2022
@whimboo whimboo deleted the atoms branch October 13, 2022 08:11
github-actions bot added a commit that referenced this pull request Oct 13, 2022
SHA: 0707ea4
Reason: push, by @whimboo

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to ZRDKPoWeR/webdriver that referenced this pull request Oct 13, 2022
SHA: 0707ea4
Reason: push, by @pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to xjc90s/webdriver that referenced this pull request Oct 13, 2022
SHA: 0707ea4
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to soloinovator/webdriver that referenced this pull request Oct 13, 2022
SHA: 0707ea4
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants