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

Selecting a particular sequence of view options causes graph to shrink to the size of one node #1082

Open
kdahlquist opened this issue Jan 23, 2024 · 13 comments

Comments

@kdahlquist
Copy link
Collaborator

kdahlquist commented Jan 23, 2024

Do this on either production or beta 7.0.8:

  • Load Demo number 2
  • From the View menu, select small
  • select medium
  • select large
  • select fit to viewport and the graph will shrink to the size of one node.

It may not have to be this exact sequence to make this happen.

@dondi
Copy link
Owner

dondi commented Apr 2, 2024

Starting (actual) window size will determine the initial View setting, so @ceciliazaragoza will retry this with a smaller initial window

@ceciliazaragoza
Copy link
Collaborator

ceciliazaragoza commented Apr 4, 2024

I tried starting with a small viewport size and then increasing, but I am still not receiving the issue on local. However, I notice that whenever I choose Fit to Viewport on beta, the graph size always decreases to one node, even without choosing other view sizes first. If I change to another view size and then back to Fit to Viewport, I do not receive the issue, though. I am receiving these error messages in the console:
image

@ntran18
Copy link
Collaborator

ntran18 commented Oct 17, 2024

Verified that I have the same issue with @ceciliazaragoza. This issue is hard to debug on local machine.

@ceciliazaragoza
Copy link
Collaborator

Because this issue is only happening on the deployed beta, it must be an issue with communication between the client laptop and the server. However, this is hard to test without deploying my code. The issue probably has something to do with this code below because there is no issue when switching between other viewport sizes, and the functions synchronizeViewportSizeFit() and requestWindowDimensions() are only called with Fit to Viewport.
image

@dondi
Copy link
Owner

dondi commented Nov 6, 2024

After looking at this, it was [re-]discovered that the deployment observation is due to how the deployed site is running a static web page with an iframe containing the actual GRNsight app. When we are working locally, we don’t have the iframe and we are running the app directly in a window (this can be simulated on the deployed version by looking at the iframe src and opening a browser window directly with that as the URL)

To address this bug without constant redeploys, we can set up this environment locally:

  • Prepare a dummy web page/site that references the local GRNsight copy inside of an iframe (see beta.html for specifics)

  • Edit the HOST_SITE constant to be the site hosting the dummy web page (e.g., http://localhost:3000 if the dummy web page is a React app)

  • With this setup, the message communication between update-app.js and iframe-coordination.js should now be entirely local. console.log statements can be used to follow the interaction

After this is done, next steps will then depend on what @ceciliazaragoza discovers

@ceciliazaragoza
Copy link
Collaborator

I was able to simulate this issue locally by creating a dummy-website and displaying GRNsight inside of an iframe like this:

<body>
    <p id="pageTitle">Testing 🤩 </p>
    <div id="graphPageContent">
        <iframe class="embedded-demo" src="http://localhost:5001"></iframe>
    </div>
</body>

Currently, my dummy-website looks like this.
Screenshot 2024-11-12 at 7 47 11 PM

However, I am getting some issues with loading the iframe correctly. My iframe element looks like this:

<iframe class="embedded-demo" src="http://localhost:5001"></iframe>

while the iframe in beta looks like this:

<iframe class="embedded-demo" src="https://grnsight.lmucs.org/beta/client" id="iFrameResizer0" scrolling="no" style="overflow: hidden; min-width: 1081px; height: 1317.45px; width: 1736.01px;"></iframe>

I am able to force the load so that the graph svg displays correctly by changing the viewport size of the graph.

@dondi
Copy link
Owner

dondi commented Nov 13, 2024

For the issue with the local iframe, you might need to temporarily revise instances of grnsight.lmucs.org into localhost:5001 (or similar)

@ceciliazaragoza
Copy link
Collaborator

ceciliazaragoza commented Nov 20, 2024

image I am still trying to get the graph to display correctly. The browser recognizes that http://localhost:8080 is the host, but the graph still looks like this when loaded, so there may be something else that is loading incorrectly that is causing the graph to not display.

However, I am able to correctly post the message to localhost:5001. I changed the code to be window.postMessage("message", HOST_SITE); instead of window.top.postMessage("message", HOST_SITE); which posts the message to http://localhost:5001. However, posting to localhost:8080 doesn't seem to work, which may mean the iframe is not being loaded correctly.

ceciliazaragoza added a commit that referenced this issue Jan 22, 2025
… since empty dimensions are being sent and propogated throughout code before they are received, and will add a Promise to ensure that wait for dimensions to be sent before running the rest of the code for displaying graph.
@ceciliazaragoza
Copy link
Collaborator

I have been able to find a solution that does not shrink the nodes to the size of one node, but I receive an error in the console that fit.container is accessing an "undefined width" attribute of grnState.dimensions. This is because I am no longer returning when the dimensions are null in fitContainer because returning causes the nodes to shrink to the size of one node. I will look into listening for when grnState.dimensions is updated to call fitContainer.

Image

Image

@dondi
Copy link
Owner

dondi commented Jan 22, 2025

@ceciliazaragoza and @dondi will find some time to step through the above scenario; meanwhile, knowing that the triggering action is the selection of Fit to Viewport, @ceciliazaragoza can do a deep dive into the calling sequence starting with that action to pinpoint exactly when/how the empty dimensions case comes up

@ceciliazaragoza
Copy link
Collaborator

ceciliazaragoza commented Jan 22, 2025

fit_to_viewport_demo.mov

Now you can fit to viewport without any errors in the console. Previously, the container.css width and height were only updated if the fitWidth and fitHeight were different than the current height, which caused the error of the nodes shrinking down to the size of one node since the width and height of the container were not updated. Instead, fitWidth and fitHeight are set to dimensions if dimensions are not undefined, else fitWidth and fitHeight are set to the current container.width() and container.height() respectively. This solves the concurrency issue of grnState.dimensions being undefined when fitContainer is first called since updateApp (which contains updateViewport and fitContainer) is called again after grnState.dimensions is updated, so eventually fitContainer will be called when grnState.dimensions is not undefined.

Question @dondi @kdahlquist : Does the behavior of the window expanding and shrinking when fit to window is enabled look as expected?

Updated:

const fitContainer = dimensions => {
        const fitWidth = dimensions ? dimensions.width - WIDTH_OFFSET: container.width();
        const fitHeight = dimensions ? dimensions.height - dimensions.top - HEIGHT_OFFSET: container.height();

        container.css({
            width: fitWidth,
            height: fitHeight
        });
    };

Previous:

 const fitContainer = (dimensions) => {
      if (!dimensions) {
          return; // First call; the next one should have dimensions filled in.
      }

      const fitWidth = dimensions.width - WIDTH_OFFSET;
      const fitHeight = dimensions.height - dimensions.top - HEIGHT_OFFSET;
      if (fitWidth !== container.width() || fitHeight !== container.height()) {
        container.css({
          width: fitWidth,
          height: fitHeight,
        });
      }
    };

@kdahlquist
Copy link
Collaborator Author

Question @dondi @kdahlquist : Does the behavior of the window expanding and shrinking when fit to window is enabled look as expected?

The behavior in the movie looks fine. I played around with the buggy version on beta to see what it is doing now. Make sure that it is still working properly when "Restrict graph to viewport" is selected.

I think that we may need to enforce a minimum size of the viewport when "fit to window" is selected. On beta 7.2.1, if you have "fit to window" selected, the viewport will shrink down to a horizontal or vertical line. I think the minimum size should be at least as big to show the D-pad and Zoom slider with a margin of one node width/height all around them. Allowing it to go smaller than that doesn't really make any sense.

ceciliazaragoza added a commit that referenced this issue Jan 29, 2025
…p is called again after the grnState.dimensions are set. while grnState.dimensions are undefined, leave the container as the current size in fitContainer, and finally update the size in fitContainer when grnState.dimensions is not undefined
ceciliazaragoza added a commit that referenced this issue Jan 29, 2025
@ceciliazaragoza
Copy link
Collaborator

As of right now, I am not experiencing that issue on my version because the minimum size of the browser window is reached that is larger than the size of the D-pad and Zoom slider. The previous Fit to Window functionality did not calculate the size of the window correctly which caused the size of the graph to be smaller than the minimum window size for the browser and into one line.

dondi added a commit that referenced this issue Jan 29, 2025
#1082: Fixed Fit to Window Functionality
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants