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

fix: delay parsing htmlValue when RTE is attached but not rendered #7890

Merged
merged 4 commits into from
Oct 3, 2024

Conversation

DiegoCardoso
Copy link
Contributor

Description

In Firefox, the styles won't be computed properly if an element is attached to the DOM, but not rendered (e.g., it's placed as a child element of another element that does not define a slot). That leads to an issue with RTE if the component is added as a child of a Lit element. The reason is that a LitElement renders its DOM asynchronously, so for a brief moment, their children are attached while the template is not yet ready.

That's problematic for RTE while using dangerouslySetHtmlValue before the element is fully rendered in the page because this method uses the clipboard Quill feature, that in turn, uses getComputedStyle to parse the HTML snipped provided into the delta format used by Quill. Since getComputedStyle won't return the correct styles in the mentioned scenario, the parsed HTML will return unexpected results.

This fix aims to work around this issue, by checking whether the getComputedStyle returns empty and, if so, creates an IntersectionObserver that will be triggered once the element is visible and in the viewport to then proceed with the HTML parsing.

One downside of this approach is that in cases like the one described, the setting of the value property will happen asynchronously.

Fixes vaadin/flow-components#6653

Type of change

  • Bugfix
  • Feature

Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

There seems to be something wrong about the test: it passes locally on main branch using the Firefox Playwright (which has an older version of Firefox) but fails when running it manually in the latest Firefox.

Also, I was able to reproduce the same issue with the simpler case (see attach helper below):

  async function attach(shadow = false) {
    const parent = fixtureSync('<div></div>');
    if (shadow) {
      parent.attachShadow({ mode: 'open' });
    }
    parent.appendChild(rte);
    await nextRender();
    flushValueDebouncer();
  }

  it('should have the html value when attached to lazily rendered parent', async () => {
    await attach(true);
    console.warn(`display: ${getComputedStyle(rte).display}`); // shows "display: flex" in Playwright
    rte.dangerouslySetHtmlValue('<h1>Foo</h1>');
    rte.parentNode.shadowRoot.innerHTML = '<slot></slot>';
    flushValueDebouncer();

    expect(rte.htmlValue).to.equal('<h1>Foo</h1>');
  });

The above test also passes on main in Playwright but fails when running manually. Adding the console.warn shows that Firefox Playwright sets display: flex. Not really sure what to do about this until we upgrade Playwright 😕

packages/rich-text-editor/test/basic.common.js Outdated Show resolved Hide resolved
Copy link

sonarqubecloud bot commented Oct 1, 2024

@DiegoCardoso DiegoCardoso merged commit f5682ab into main Oct 3, 2024
9 checks passed
@DiegoCardoso DiegoCardoso deleted the fix/rich-text-editor/delay-html-parsing branch October 3, 2024 07:42
vaadin-bot pushed a commit that referenced this pull request Oct 3, 2024
…7890)

* fix: delay parsing htmlValue when RTE is attached but not rendered

* refactor: simplify test setup

* refactor: move attach tests to own file

* refactor: remove condition not needed
web-padawan pushed a commit that referenced this pull request Oct 3, 2024
vaadin-bot pushed a commit that referenced this pull request Oct 3, 2024
…7890)

* fix: delay parsing htmlValue when RTE is attached but not rendered

* refactor: simplify test setup

* refactor: move attach tests to own file

* refactor: remove condition not needed
web-padawan pushed a commit that referenced this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RichTextEditor loses newlines when placed inside a Lit component
4 participants