Fix widget styling issues by using shadow DOM#3065
Fix widget styling issues by using shadow DOM#3065
Conversation
| let widgetFrame = document.createElement('iframe'); | ||
| let shadowHost = document.createElement("div"); | ||
| let shadowRoot = shadowHost.attachShadow({ mode: "closed" }); | ||
| shadowRoot.appendChild(widgetFrame); |
There was a problem hiding this comment.
I could replace the iframe with the shadowHost, but didn't see a benefit to doing that in the spirit of minimizing change. Let me know if you think I should.
There was a problem hiding this comment.
There is probably no point to keeping the iframe but there is also no harm in it? And we do need some way to select the correct placeholder in tests, but we could use the placeholder's text or className or something else inside the shadow root to do it instead. I don't have a preference either way, but maybe testing against widgets will present reasons for removing the iframe?
There was a problem hiding this comment.
Sounds good. I'll leave it as is for now.
ghostwords
added
the
widgets
ghostwords
left a comment
ghostwords
left a comment
There was a problem hiding this comment.
Thank you! This round of feedback is about testing updates. I still have to test against real-world widget examples.
| ), widgetsJson) | ||
|
|
||
| def switch_to_shadow_host_frame(self, selector): | ||
| shadow_host = self.find_el_by_css("div.widget-shadow-host") |
There was a problem hiding this comment.
Rather than exposing an identifier for tests, let's loop over all the divs and look for one with a shadow host that contains the iframe we want. (The line above assumes the first matching div will be the one we want, which seems to work at the moment but may not in the future.)
Something like,
def switch_to_placeholder_frame(self, selector):
for el in self.driver.find_elements(By.CSS_SELECTOR, "div"):
try:
shadow_root = el.shadow_root
except NoSuchShadowRootException:
continue
iframe = shadow_root.find_element(By.CSS_SELECTOR, selector)
self.driver.switch_to.frame(iframe)
return
raise NoSuchElementException("Failed to find placeholder frame")There was a problem hiding this comment.
That works, thank you!
…dow root must be open for Selenium access
lenacohen
force-pushed
the
widget-iframe-inside-shadow-dom
branch
from
02e4cc9 to
ca9d736
Compare
Closed shadow roots interfere with testing, and possibly accessibility: https://jschof.dev/posts/2024/4/web-components-and-you-7/. We don't need to prevent the placeholder from being accessed by outside JS to prevent the known UI bugs
As it adds unwanted space below the placeholder on livestream.eff.org
ghostwords
force-pushed
the
widget-iframe-inside-shadow-dom
branch
from
7390807 to
42ee2b4
Compare
ghostwords
changed the title
|
Is there anything we can do for cases like https://www.twz.com/sea/ukraine-situation-report-russian-navy-creating-new-drone-regiments-in-wake-of-enemy-successes where our placeholder is newly hidden because it no longer overrides anything with a lower z-index? Using shadow DOM isolates placeholder styles from page styles, which is great. Our z-index no longer working is less great though. While it's nice to no longer overlap navbars, I think it's a poor tradeoff if it means our placeholders can now be more easily hidden by site error messages. |
2 participants
Address widget replacement overlapping with content by ensuring that the parent node height is at least as large as the min height of the widget