Popouts: Sometimes stylesheets don't get copied over correctly to new window

Juliakas commented 2 weeks ago

Describe the bug

Sometimes when styling components and combining shorthand properties with css variables and adding other related longhand properties to override something, document.stylesheet produces unexpected results that are then copied over to popout window and wrong styles are getting applied.

One example being:

.border-test {
  --border-color: red;
  border: 5px solid var(--border-color);
  border-bottom: 5px solid blue;
  height: 20px;
  width: 100px;
}

In main window it looks like so:

And when popping out to new window, this gets produced upon inspecting:

One interesting thing is that it works correctly when I use static value instead of variable or when I remove border-bottom.

This seems to be intended spec behavior that serializes cssText property like that. I found this chromium rejected issue: https://issues.chromium.org/issues/40804066 and spec was linked to indicate intended behavior: https://drafts.csswg.org/css-variables/#pending-substitution-value

To Reproduce

Add a simple div to a widget that can popout with a classname that defines the following styles:

.border-test {
  --border-color: red;
  border: 5px solid var(--border-color);
  border-bottom: 5px solid blue;
  height: 20px;
  width: 100px;
}

Open application, note applied borders for that div inside a widget, all 4 of them are applied with bottom one being different color.
Popout that widget.
Notice how only bottom border is applied.
Open devtools, inspect that div and note that stylesheets have deformed and lost information on remaining 3 borders.

Expected Behavior

It is expected that when popping out widgets to a new window, all styles should be preserved as is and should not lose information. In this case it is expected that all borders retain same css properties as it had in main window.

Screenshots

No response

Desktop (please complete the applicable information)

OS: Windows 11 Browser: Microsoft Edge Browser version: Version 126.0.2592.68 (Official build) (64-bit) Appui version: 4.15.0 (reproduced on source)

Additional context

I debugged source at CopyStyles.ts and noticed deformation was happening there. In my case I worked around this issue by being very verbose with longhand css properties and it seemed to work, but it can still cause issues as we just haven't discovered problems in other areas, maybe even in external code that we can't control.

mayank99 commented 1 week ago

I was looking into this issue, and it's wild that this is considered to be "working as intended" by the spec and by browsers. I'm not sure that there is even a way around it.

@Juliakas What components have you faced this issue in? Is it only in your own custom CSS? We can at least use the longhands in library code.

Juliakas commented 1 week ago

I guess the explanation could be that css variables could be anything (either color, number of pixels, border style keyword) and when converting to longhands it is not clear how to parse them, shorthands are very lenient on what order you write your properties like border: solid 1px blue or border: thick double #32a1ce. But it is still strange... why not simply preserve whatever was written in <style> tags or something.

So far only noticed in my own component, I needed different border stylings to achieve effect I wanted.

The only other idea I have is instead of parsing document.stylesheets is cloning all <style> and <link rel="stylesheet"> tags in <head> to another window. I have no idea if there is any other place where styles can be hidden (apart from inline).

mayank99 commented 1 week ago

The only other idea I have is instead of parsing document.stylesheets is cloning all <style> and <link rel="stylesheet"> tags in <head> to another window.

Yeah I was thinking this too; we would need to ensure there is no extra network request happening when cloning a <link>.

I have no idea if there is any other place where styles can be hidden (apart from inline).

There is document.adoptedStyleSheets and styles inside shadow DOM. I guess both can be solved if they are re-initialized for every new window. We can investigate this together with #892.

mayank99 commented 1 week ago

Discussed a two-pronged approach with @GerardasB today after looking at CopyStyles.ts.

Here's the rough code that I think could be used to handle <style> and <link> without relying on the problematic cssText. It uses ownerNode and importNode.

Array.from(document.styleSheets).forEach(({ ownerNode }) => {
  const clonedNode = targetDoc.importNode(ownerNode, true);
  targetDoc.head.appendChild(clonedNode);
});

For document.adoptedStyleSheets, I think we'll need to continue using cssText; I can't think of a way around it.

Array.from(document.adoptedStyleSheets).forEach((styleSheet) => {
  const newStyleSheet = new targetDoc.defaultView.CSSStyleSheet();
  newStyleSheet.replaceSync(stringifyStyleSheet(styleSheet));
  targetDoc.adoptedStyleSheets.push(newStyleSheet);
});

function stringifyStyleSheet(stylesheet) {
  return Array.from(stylesheet.cssRules)
    .map(rule => rule.cssText || "")
    .join("\n")
}

And for shadow DOM internal styles, I've proposed that AppUI emit a custom event whenever reparenting happens; it could be named like something like appui:reparent. Components like ProgressRadial can listen to this event and re-run any required code for adding styles.

Juliakas commented 1 week ago

For styleSheets this sounds nice, will need to make sure it handles everything right.

I was reading and playing around with adoptedStyleSheets and also couldn't figure out a way. I guess they are needed for shadow dom anyway, which will be reconstructed.

What actually happens with shadow dom styles when its dom gets transferred? Did you figure out why they disappear?

mayank99 commented 6 days ago

Just to clarify, I'm talking about document.adoptedStyleSheets, which aren't related to shadow DOM. We used this feature in iTwinUI v2, and might use it again in the future in iTwinUI v3.

For shadow DOM we also use adoptedStyleSheets, but that's a different issue (#892). It will need some more investigation, but in any case I don't expect AppUI to automatically handle it, which is why I suggested the coordination approach using a custom event.

iTwin / appui