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.

Sign up for GitHub

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

Jump to bottom

fix: propagate layout call to all children of InspectableWebContentsViewViews #39994

Merged
merged 1 commit into from Sep 28, 2023
Merged

fix: propagate layout call to all children of InspectableWebContentsViewViews #39994

merged 1 commit into from Sep 28, 2023

Conversation

marekharanczyk
Copy link
Contributor

@marekharanczyk marekharanczyk commented Sep 27, 2023 •
edited by ckerr

When BrowserView bounds are set from js, those might not trigger layout immediately, sometimes propagating InvalidateLayout call to parent. View is marked as needing layout, expecting to receive it from parent on next layout call. The problem is that BrowserView's view is added as child of InspectableWebContentsViews which does not call setBounds (which would trigger layout) on all of it's children when doing it's layout, so it skips propagating Layout call to its children BrowserViews views, even though those were marked as needing layout.
Call base class View::Layout which will iterate over views' children and call Layout on those that were marked as needing them.

Unfortunately I failed to create a spec test that would repro this behavior. It seems user click on button (like in gist in bug) might be needed to repro correctly.

Fixes #39993.

Checklist

  • npm test passes

Release Notes

Notes: Fixed BrowserView.setBounds() calls not painting view in new bounds in some cases.

@marekharanczyk
Propagate layout call to all children of InspectableWebContentsViewVi…
0641e2b 
…ews.

When BrowserView bounds are set from js, those might not trigger layout
immediately, sometimes propagating InvalidateLayout call to parent.
View is marked as needing layout, expecting to receive it from parent on
next layout call. The problem is that BrowserView's view is added as child
of InspectableWebContentsViews which does not call setBounds (which
would trigger layout) on all of it's children when doing it's layout,
so it skips propagating Layout call to its children BrowserViews views,
even though those were marked as needing layout.
Call base class View::Layout which will iterate over views' children
and call Layout on those that were marked as needing them.

Fixes electron#39993.
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 27, 2023
@marekharanczyk marekharanczyk changed the title Propagate layout call to all children of InspectableWebContentsViewViews. fix: Propagate layout call to all children of InspectableWebContentsViewViews. Sep 27, 2023
codebytere
codebytere approved these changes Sep 27, 2023
View reviewed changes
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

It looks like this is considered a best practice upstream in overridden View::Layout() implementations in Chromium so this looks good to me. Verified to address the problem on non-macOS platforms with the gist in the originating issue.

I think it might be possible to test this with desktopCapturer screenshot color matching but I won't block on it because it's likely to be a pain and likely faster done by someone more familiar with that zone of the codebase.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. labels Sep 27, 2023
@codebytere codebytere changed the title fix: Propagate layout call to all children of InspectableWebContentsViewViews. fix: propagate layout call to all children of InspectableWebContentsViewViews Sep 27, 2023
@marekharanczyk
Copy link
Contributor Author

I tried doing this, unfortunately this test passes without a fix while the gist does not work properly, so there must be more to it:

    it('updates bounds immidiately', async () => {
      const display = screen.getPrimaryDisplay();
      w.show();
      w.setBounds(display.bounds);
      await w.loadURL('data:text/html,<body style="background-color:rgb(255,0,0);"></body>')

      view = new BrowserView();
      w.addBrowserView(view);
      await view.webContents.loadURL('data:text/html,<body style="background-color:rgb(0,255,0);"></body>');

      const pageColor = "#FF0000";
      const viewColor = "#00FF00";
      const bounds1 = { x: 0, y: 0, width: 200, height: 200 };
      const bounds2 = { x: 400, y: 400, width: 200, height: 200 };
      const capturePoint = { x: 100, y: 100 };

      view.setBounds(bounds1);

      expect(view.getBounds()).to.deep.equal(bounds1);
      await setTimeout(100);
      let screenCapture = await captureScreen();
      let captureColor = getPixelColor(screenCapture, capturePoint);
      console.log(captureColor);
      expect(areColorsSimilar(captureColor, viewColor)).to.be.true();

      view.setBounds(bounds2);

      expect(view.getBounds()).to.deep.equal(bounds2);
      await setTimeout(100);
      screenCapture = await captureScreen();
      captureColor = getPixelColor(screenCapture, capturePoint);
      console.log(captureColor);
      expect(areColorsSimilar(captureColor, pageColor)).to.be.true();

      view.setBounds(bounds1);

      expect(view.getBounds()).to.deep.equal(bounds1);
      await setTimeout(100);
      screenCapture = await captureScreen();
      captureColor = getPixelColor(screenCapture, capturePoint);
      console.log(captureColor);
      expect(areColorsSimilar(captureColor, viewColor)).to.be.true();
    });

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 28, 2023
@jkleinsc jkleinsc merged commit 94585f5 into electron:main Sep 28, 2023
23 of 24 checks passed
@release-clerk
Copy link

release-clerk bot commented Sep 28, 2023 •
edited by ckerr

Release Notes Persisted

Fixed BrowserView.setBounds() calls not painting view in new bounds in some cases.

This was referenced Sep 28, 2023
Merged
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Sep 28, 2023

I have automatically backported this PR to "25-x-y", please check out #40035

@trop trop bot added in-flight/25-x-y and removed target/25-x-y PR should also be added to the "25-x-y" branch. labels Sep 28, 2023
@trop
Copy link
Contributor

trop bot commented Sep 28, 2023

I have automatically backported this PR to "26-x-y", please check out #40036

@trop
Copy link
Contributor

trop bot commented Sep 28, 2023

I have automatically backported this PR to "27-x-y", please check out #40037

@trop trop bot added in-flight/26-x-y in-flight/27-x-y merged/26-x-y PR was merged to the "26-x-y" branch. merged/27-x-y PR was merged to the "27-x-y" branch. and removed target/26-x-y PR should also be added to the "26-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. in-flight/26-x-y in-flight/27-x-y labels Sep 28, 2023
@trop trop bot added merged/25-x-y PR was merged to the "25-x-y" branch. and removed in-flight/25-x-y labels Sep 29, 2023
@FrankenApps
Copy link

Just wanted to add, that this also fixes: #35149

MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
@marekharanczyk
fix: propagate layout call to all children of `InspectableWebContents…
8a46e5f 
…ViewViews` (electron#39994)

Propagate layout call to all children of InspectableWebContentsViewViews.

When BrowserView bounds are set from js, those might not trigger layout
immediately, sometimes propagating InvalidateLayout call to parent.
View is marked as needing layout, expecting to receive it from parent on
next layout call. The problem is that BrowserView's view is added as child
of InspectableWebContentsViews which does not call setBounds (which
would trigger layout) on all of it's children when doing it's layout,
so it skips propagating Layout call to its children BrowserViews views,
even though those were marked as needing layout.
Call base class View::Layout which will iterate over views' children
and call Layout on those that were marked as needing them.

Fixes electron#39993.
@sbenmoussati sbenmoussati mentioned this pull request Feb 15, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@codebytere codebytere codebytere approved these changes

Assignees
No one assigned
Labels
merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. merged/27-x-y PR was merged to the "27-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: BrowserView.setBounds is not appliead immediately on first calls.
4 participants
@marekharanczyk @FrankenApps @jkleinsc @codebytere