[atoms] fix text node children are always considered as displayed #16284

[joerg1985]

User description

🔗 Related Issues

Should fix #16284

💥 What does this PR do?

Call the displayed check for children before considering them displayed.

🔧 Implementation Notes

I am not to deep into JS, so someone should double check this.

💡 Additional Considerations

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix

---

Description

• Fix text node children visibility check in DOM atoms

• Add proper displayed check for nested elements

• Rename parameter for clarity in isShown_ function

• Add test case for nested text node visibility

---

Diagram Walkthrough

flowchart LR
  A["isShown_ function"] --> B["displayedFn parameter"]
  B --> C["Nested element check"]
  C --> D["Text node visibility"]
  D --> E["Test validation"]

Loading

File Walkthrough

	Relevant files
Bug fix	dom.js Fix nested element visibility checks                                          javascript/atoms/dom.js Rename parentsDisplayedFn parameter to displayedFn for clarity Add visibility and displayed checks for nested elements Fix logic to properly check children element visibility +17/-7
Tests	shown_test.html Add test for nested text node                                                        javascript/atoms/test/shown_test.html Add new test case testNestedTextNode Create HTML element with hidden nested text content +9/-0

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Behavior Change The visibility checks were moved from isShown_ into the displayed helper and now include visibility: hidden|collapse and content-visibility: hidden. This broadens the definition of “not shown.” Confirm this does not regress cases where zero-sized but visible elements (e.g., SVGs or elements with visible descendants) should still be considered shown. bot.dom.isShown = function (elem, opt_ignoreOpacity) { /** * Determines whether an element or its parents have `display: none` or similar CSS properties 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') // Any element with hidden/collapsed visibility is not shown. || (bot.dom.getEffectiveStyle(elem, 'visibility') == 'hidden') || (bot.dom.getEffectiveStyle(elem, 'visibility') == 'collapse') || (bot.dom.getEffectiveStyle(elem, 'content-visibility') == 'hidden')) { return false; } } Ancestor/Descendant Check displayedFn is now used both for ancestor and descendant checks, including inside zero-size handling. Ensure this does not incorrectly hide elements when descendants are not displayed but should allow ancestor to be considered shown due to other visible children or layout nuances. return true; } // A vertical or horizontal SVG Path element will report zero width or // height but is "shown" if it has a positive stroke-width. if (bot.dom.isElement(e, 'PATH') && (rect.height > 0 || rect.width > 0)) { var strokeWidth = bot.dom.getEffectiveStyle(e, 'stroke-width'); return !!strokeWidth && (parseInt(strokeWidth, 10) > 0); } if (!displayedFn(e)) { return false; } // Zero-sized elements should still be considered to have positive size // if they have a child element or text node with positive size, unless // the element has an 'overflow' style of 'hidden'. return bot.dom.getEffectiveStyle(e, 'overflow') != 'hidden' &&

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Ignore empty or whitespace-only text nodes Refine the visibility check to ignore empty or whitespace-only text nodes when determining if a parent element is shown. javascript/atoms/dom.js [530-534] return bot.dom.getEffectiveStyle(e, 'overflow') != 'hidden' && goog.array.some(e.childNodes, function (n) { - return n.nodeType == goog.dom.NodeType.TEXT || + return (n.nodeType == goog.dom.NodeType.TEXT && !!n.nodeValue && !!n.nodeValue.trim()) || (bot.dom.isElement(n) && positiveSize(n)); }); Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a subtle bug where an element containing only whitespace text could be considered visible, and the proposed fix using trim() is accurate and improves the function's correctness.	Medium
Update

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Behavior Change The new visibility/display checks are now applied to descendant nodes as well via the injected displayedFn. Verify this does not regress cases where zero-sized containers with hidden descendants were previously considered shown (e.g., SVGs or elements relying on overflow behavior). // Any element with hidden/collapsed visibility is not shown. var visibility = bot.dom.getEffectiveStyle(e, 'visibility'); if (visibility == 'collapse' || visibility == 'hidden') { return false; } if (!displayedFn(e)) { return false; } Naming Clarity The parameter rename to displayedFn broadens semantics from "parents" to "ancestors or descendants". Confirm downstream callers pass a function compatible with this widened scope and that JSDoc matches actual usage. * @param {function(!Element):boolean} displayedFn a function that's used * to tell if the chain of ancestors or descendants are all shown. * @return {boolean} Whether or not the element is visible. * @private */ bot.dom.isShown_ = function (elem, ignoreOpacity, displayedFn) { if (!bot.dom.isElement(elem)) { Test Coverage Gap New logic affects OPTION/OPTGROUP and image map branches; ensure added tests cover these paths with hidden ancestors/descendants to validate the new displayedFn propagation. // Option or optgroup is shown iff enclosing select is shown (ignoring the // select's opacity). if (bot.dom.isElement(elem, goog.dom.TagName.OPTION) || bot.dom.isElement(elem, goog.dom.TagName.OPTGROUP)) { var select = /**@type {Element}*/ (goog.dom.getAncestor(elem, function (e) { return bot.dom.isElement(e, goog.dom.TagName.SELECT); })); return !!select && bot.dom.isShown_(select, true, displayedFn); } // Image map elements are shown if image that uses it is shown, and // the area of the element is positive. var imageMap = bot.dom.maybeFindImageMap_(elem); if (imageMap) { return !!imageMap.image && imageMap.rect.width > 0 && imageMap.rect.height > 0 && bot.dom.isShown_( imageMap.image, ignoreOpacity, displayedFn); }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Refactor visibility logic for clarity Refactor the code to eliminate duplicate visibility and display checks. The checks added to the positiveSize helper function are already performed in the main bot.dom.isShown_ function, reducing maintainability. Examples: javascript/atoms/dom.js [531-539] // Any element with hidden/collapsed visibility is not shown. var visibility = bot.dom.getEffectiveStyle(e, 'visibility'); if (visibility == 'collapse' || visibility == 'hidden') { return false; } if (!displayedFn(e)) { return false; } Solution Walkthrough: Before: bot.dom.isShown_ = function (elem, ignoreOpacity, displayedFn) { // ... // Visibility and display checks for the main element var visibility = bot.dom.getEffectiveStyle(elem, 'visibility'); if (visibility == 'collapse' || visibility == 'hidden') { return false; } if (!displayedFn(elem)) { return false; } // ... function positiveSize(e) { // ... size checks ... // DUPLICATE checks for child elements are added here var visibility = bot.dom.getEffectiveStyle(e, 'visibility'); if (visibility == 'collapse' || visibility == 'hidden') { return false; } if (!displayedFn(e)) { return false; } // ... recursive check on child nodes return goog.array.some(e.childNodes, function (n) { return ... (bot.dom.isElement(n) && positiveSize(n)); }); } // ... }; After: bot.dom.isShown_ = function (elem, ignoreOpacity, displayedFn) { // ... // Visibility and display checks for the main element var visibility = bot.dom.getEffectiveStyle(elem, 'visibility'); if (visibility == 'collapse' || visibility == 'hidden') { return false; } if (!displayedFn(elem)) { return false; } // ... function hasVisibleChild(e) { // A new helper to check children without duplicating logic return goog.array.some(e.childNodes, function (n) { if (n.nodeType == goog.dom.NodeType.TEXT) return true; // Recursively call the main function for children if (bot.dom.isElement(n)) return bot.dom.isShown_(n, ignoreOpacity, displayedFn); return false; }); } function positiveSize(e) { // ... size checks ... // Use the new helper instead of duplicating checks return bot.dom.getEffectiveStyle(e, 'overflow') != 'hidden' && hasVisibleChild(e); } // ... }; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the PR introduces duplicate visibility and display checks within the positiveSize helper function, which harms code clarity and maintainability in the complex bot.dom.isShown_ function.	Medium
General	Avoid redundant checks on element To avoid redundant checks, wrap the visibility and displayedFn(e) calls inside the positiveSize function with a condition to ensure they only run for child elements, not the initial top-level element. javascript/atoms/dom.js [531-539] -// Any element with hidden/collapsed visibility is not shown. -var visibility = bot.dom.getEffectiveStyle(e, 'visibility'); -if (visibility == 'collapse' || visibility == 'hidden') { - return false; +if (e != elem) { + // Any element with hidden/collapsed visibility is not shown. + var visibility = bot.dom.getEffectiveStyle(e, 'visibility'); + if (visibility == 'collapse' || visibility == 'hidden') { + return false; + } + + if (!displayedFn(e)) { + return false; + } } -if (!displayedFn(e)) { - return false; -} - Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the visibility and display checks inside the positiveSize function are redundant for the initial element, as they are already performed in the outer bot.dom.isShown_ function, and proposes a valid optimization.	Low
More