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

[bug][docs] <rh-button> second, empty button appearing in WebKit #2097

Closed
hellogreg opened this issue Dec 19, 2024 · 7 comments · Fixed by #2119
Closed

[bug][docs] <rh-button> second, empty button appearing in WebKit #2097

hellogreg opened this issue Dec 19, 2024 · 7 comments · Fixed by #2119
Assignees
Labels
bug Something isn't working docs Improvements or additions to documentation

Comments

@hellogreg
Copy link
Collaborator

hellogreg commented Dec 19, 2024

Describe the bug

At the docs site, a second, empty button typically appears next to <rh-button> in Mac and iOS Safari. This can be seen at the button demo page and for other elements where we're using rh-button (e.g., tooltip).

Word on the street is that this is an SSR-related issue.

Screenshots

Double button at docs site

Steps to reproduce

  1. Go to the rh-button demo page in Safari.
  2. Observe the buttons.

Expected behaviour

Only the real button should appear, not the impostor.

Operating System (OS)

macOS and iOS

Browser

Safari 18.2

@hellogreg hellogreg added bug Something isn't working docs Improvements or additions to documentation labels Dec 19, 2024
@hellogreg hellogreg added this to the 2025/Q1 — Cubone release milestone Dec 19, 2024
@hellogreg hellogreg changed the title [bug] <rh-button> second, empty button appearing in WebKit [bug][docs] <rh-button> second, empty button appearing in WebKit Dec 19, 2024
@hellogreg hellogreg moved this from Backlog to Todo in Red Hat Design System Dec 19, 2024
@hellogreg
Copy link
Collaborator Author

Not sure if this is related to #2055, but mentioning that here in case it is.

@adamjohnson
Copy link
Collaborator

If you view the Icon and Variants demos in Safari on main, you can see the following behavior:

rh-button icon style in safari double rendering
rh-button variants in safari double rendering

Duplicate button elements without text, duplicate icons.

These screenshots / behavior exist after #2055 has been merged.

@bennypowers
Copy link
Member

Same solution here as for icon and CTA

At this point I think this did require an SSR debug controller, which is unfortunate.

I'll try to make a repro and file on lit

@KonnorRogers
Copy link

KonnorRogers commented Jan 9, 2025

Hi there! I noticed @bennypowers posted this in the Lit GitHub and figured I'd chime in.

Double rendering like this is usually either:

A.) A timing issue withthe hydration support module
B.) Rendering mismatch

Since theres no errors on hydration mismatch ,its most likely A.

I noticed the following on this page:

https://ux.redhat.com/elements/button/demo/

<head>
<script type="module">
      // dsd polyfill needs to happen before hydration attempts
      // lit-element-hydrate-support needs to be included before lit is loaded
      import '/assets/javascript/dsd-polyfill.js';
      import '@lit-labs/ssr-client/lit-element-hydrate-support.js';
      import 'element-internals-polyfill';
      // include these, which are often internal to elements, to avoid SSR defer-hydration bugs
      import '@rhds/elements/rh-icon/rh-icon.js';
      import '@rhds/elements/rh-surface/rh-surface.js';
      // load up the demo and picker
      import '@rhds/elements/lib/elements/rh-context-picker/rh-context-picker.js';
      import '@rhds/elements/lib/elements/rh-context-demo/rh-context-demo.js';
</script>
</head>

The timings of imports parsing + execution is different across browsers and can't really be relied upon like this. I would try the following just to isolate if it is indeed the hydration module loading too late:

<script type="module">
window.componentsLoaded = new Promise(async (resolve) => {
  await import('import '/assets/javascript/dsd-polyfill.js');
  
  // I'm assuming order doesn't matter here?
  await Promise.allSettled([
    import('@lit-labs/ssr-client/lit-element-hydrate-support.js'),
    import('element-internals-polyfill');
  ])
  
  await Promise.allSettled([
      // include these, which are often internal to elements, to avoid SSR defer-hydration bugs
      import('@rhds/elements/rh-icon/rh-icon.js');
      import('@rhds/elements/rh-surface/rh-surface.js');
      // load up the demo and picker
      import('@rhds/elements/lib/elements/rh-context-picker/rh-context-picker.js');
      import('@rhds/elements/lib/elements/rh-context-demo/rh-context-demo.js');
  ])

  resolve()
})
</script>

And then for the components you load on the page, just make sure everything has loaded first.

<body>
  <script type="module">
    await window.componentsLoaded
    import('@rhds/elements/rh-button/rh-button.js');
  </script>
</body>

Its entirely possible this isn't the case, but I remember hitting similar timing issues in Rails with importmaps and figured I'd throw it out there.

@bennypowers
Copy link
Member

@KonnorRogers i suspect that what's happening here is that

  1. webkit loads up the definition of rh-button (asynchronously and out of order with the rest of the code
  2. then afterwards, webkit loads the lit-html hydration support module

If that's the case, then we should be able to do something like this:

<script type="module">
window.ssrSupportLoaded = new Promise(async (resolve) => {
  await import('import '/assets/javascript/dsd-polyfill.js');
  
  // I'm assuming order doesn't matter here?
  await Promise.allSettled([
    import('@lit-labs/ssr-client/lit-element-hydrate-support.js'),
    import('element-internals-polyfill');
  ])

  resolve()
})
</script>

<script type="module">
  await window.ssrSupportLoaded
  
  await Promise.allSettled([
    // put app dep imports here in any order
  ])
</script>

I haven't tested that, but WDYT, does that track with your understanding?

We found that simply loading rh-button higher up in the chain (i.e. just after lit-element-hydrate-support) did the trick for us

  <script type="module">
    // dsd polyfill needs to happen before hydration attempts
    // lit-element-hydrate-support needs to be included before lit is loaded
    import '/assets/javascript/dsd-polyfill.js';
    import '@lit-labs/ssr-client/lit-element-hydrate-support.js';
    import 'element-internals-polyfill';
    // include these, which are often internal to elements,
    // and are definitely required by context-picker and context-demo
    // to avoid SSR defer-hydration bugs and SSR double-rendering bugs
    import '@rhds/elements/rh-icon/rh-icon.js';
    import '@rhds/elements/rh-surface/rh-surface.js';
    import '@rhds/elements/rh-button/rh-button.js';
    import '@rhds/elements/rh-tooltip/rh-tooltip.js';
  </script>

@KonnorRogers
Copy link

KonnorRogers commented Jan 13, 2025

Your first code block is going to be much more reliable. The second one is kind of just hoping import order is respected by the browser (which im not 100% will always be the case)

I also think I read the script wrong the first time, you may even be able to do this:

<script type="module">
window.ssrSupportLoaded = new Promise(async (resolve) => {
- await import('import '/assets/javascript/dsd-polyfill.js');
  
  // I'm assuming order doesn't matter here?
  await Promise.allSettled([
+   import('/assets/javascript/dsd-polyfill.js'),
    import('@lit-labs/ssr-client/lit-element-hydrate-support.js'),
    import('element-internals-polyfill'),
  ])

  resolve()
})
</script>

<script type="module">
  await window.ssrSupportLoaded
  
  await Promise.allSettled([
    // put app dep imports here in any order
  ])
</script>

@bennypowers
Copy link
Member

i think you're right on both counts. Our problem is that we aren't building apps, we're shipping libraries, so we can't impose too many hard requirements on module loading (it's hard enough to get users to load modules in the first place)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working docs Improvements or additions to documentation
Projects
Status: Done ☑️
Status: Done ☑️
Development

Successfully merging a pull request may close this issue.

4 participants