orefalo / svelte-splitpanes

MIT License
390 stars 19 forks source link

Resize bug #100

Open adriendst opened 2 months ago

adriendst commented 2 months ago

Hello, I am using the library and I noticed some malfunction. I don't know if they are already known, so I wanted to share them to you.

The first one is :

https://github.com/user-attachments/assets/56e311d3-d9c3-467b-af9e-c793f0b22087

In that case, when the top pane reach his minSize, he can still be minimized a little bit. Moreover, the when the top pane is smaller than his minSize, the bottom pane size reduce too. In the DOM we can see that the combined height of the three panes are over 100%.

The second one is :

https://github.com/user-attachments/assets/8a44ec5d-adc3-4ea1-ac5f-0a044bda8ce9

In that case, when the first pane reach the end of the splitPanes component, both the first and last pane shrinks. The combined width of the three panes are less than 100%.

I was wondering if you could look at them ? Don't hesitate to ask me for more information :) (both the videos have been recorded from https://orefalo.github.io/svelte-splitpanes/ )

orefalo commented 2 months ago

Morning @adriendst,

Thank you so much for reporting the issues. I knew about the first one, not the second.

I honestly don't know when I will free up time to look into those.

adriendst commented 2 months ago

Okay, let me know when you will get into it ! I did some adjustment in my own code to avoid those bugs for the moment

hopperelec commented 2 months ago

I've also experienced both of these issues while working on this PR in a separate repo. It is worth adding that there seems to be a discrepancy between what happens in Chromium- and Firefox-based browsers, which I and the other repo's maintainer have discussed more in the PR I linked.

orefalo commented 2 months ago

Identified the issue. it's a regression bug. may have to revert passive events.

anyways, stick to v0.8.2 for now

orefalo commented 2 months ago

found the bug. expect a release soon

orefalo commented 2 months ago

I believe this bug is resolved in version 8.0.6. Please try

hopperelec commented 2 months ago

I'll try it, thanks! By the way, are you intentionally sticking with the 8 major versioning?

hopperelec commented 2 months ago

The first issue seems to be fixed, but I'm still having an issue which most closely resembles the second issue. I'm not entirely sure what's causing it- as far as I understand, I'm not using any of the sizing parameters in any extraordinary ways- but if I set the initial size at all, then:

hopperelec commented 2 months ago

After a bit more testing, this seems to actually be an issue with reactivity. I'll try and fix the issue in the PR and hopefully I'll then have a better idea of exactly what it is.

hopperelec commented 2 months ago

I don't recall getting this before, but I am now noticing these errors. image They only happen in Chromium and if the size has been set. If the issue occurs, they happen at the same time as the size changing to 0, but they also happen even if the issue doesn't occur. The issue seems to occur if the size starts as null/undefined then changes reactively to a numeric value. The error seems to indicate that the pane itself somehow becomes undefined. I have not yet found a fix for this.

orefalo commented 2 months ago

Good morning,

Regarding the version, I am sticking to 8.x.x. I made a mistake a few weeks ago, and it was too late to withdraw the package from npm. I plan to port to Svelte 5 as 9.x.x.

Regarding the exception, those are good tips. I will investigate. A REPL to reproduce the error would be helpful.

Best regards,

PS: size should not be null. set it to 0

orefalo commented 2 months ago

Seems to be going through this code.

  const reportGivenSizeChangeSafe = (size: number) => {
    // We put an extra check of `size != sz` here and not in the reactive statement, since we don't want a change
    //  of `sz` to trigger report.
    if (size != sz) {
      carefullClientCallbacks('reportGivenSizeChange')(size);
    }
  };
  $: {
    if (browser && size != null) {
      reportGivenSizeChangeSafe(size);
    }
  }

considering introducing a typeof value === 'number' but I need a REPL to test @hopperelec

orefalo commented 1 month ago

plz test the latest code changes in master - not release as a version yet. if the bug comes from the component, it should fix it. closing for now

hopperelec commented 1 month ago

Oh sorry, been busy lately because I've just started uni. I'll try and test it ASAP!

hopperelec commented 1 month ago

Ok, I tried creating a REPL for this using StackBlitz and now I'm even more confused about the issue.

I first tried with your changes in master (repl) where:

But then, to make sure that these differences were because of your changes, I also made a REPL based on the exact code I've been testing with from the svelte-mosaic PR (repl) where:

So somehow, I am now able to reproduce 5 different types of behaviour:

Leodog896 has the same issues as me when running it locally, so I'm confident that this is not just something weird with my specific setup. My first guess as to what could cause the difference between running it locally and on StackBlitz was node versions, since I use Node 20 locally and StackBlitz only supports Node 18, however the difference between viewing it in-editor and in a separate tab makes me think it's more likely to be related to StackBlitz's WebContainers. More local testing could be done to narrow down what is causing these differences (running GitHub master version locally, running both versions locally using Node 18, setting up WebContainers locally) but, with there being so many differences to test now, I'm not really sure where to start, and I'm hoping someone else will have a better idea from the information I've already found.

orefalo commented 1 month ago

First and foremost, thank you for setting up the test.s

I will need a bit more time to identify the root cause, and I plan to release the changes in master as version 8.0.7 for this purpose.

Will keep you posted.

orefalo commented 1 month ago

Released 8.0.7 - didn't make any significative changes other than releasing the code from master.

Tried it against your second REPL. seems to be working fine, I can't reproduce the 0 size issue on chrome or firefox.

However, there is an error on the console: TypeError: Failed to construct 'URL': Invalid URL, which might be coming from code.

hopperelec commented 1 month ago

it took a while to realize the code was duplicated

Ye, it was a bit of a weird setup, but I wasn't sure of a better way to do it in StackBlitz. Although I haven't used StackBlitz very much, so it is very possible I'm missing something here. I also did have to strip down the repo a bit since some of the linting packages required Node 20 which StackBlitz doesn't support and I didn't want to figure out which specific ones. Do let me know if you know a better way to go about testing unpublished changes!

Tried it against your second REPL. seems to be working fine, I can't reproduce the 0 size issue on chrome or firefox.

Just tried it myself and I'm still getting the 0 size issue in Chromium if I open it in a separate tab. Just to confirm, did you try by opening it in a new tab?

However, there is an error on the console: TypeError: Failed to construct 'URL': Invalid URL, which might be coming from code.

I'm not getting any errors in the server console, and in the browser console I only get the same errors I showed a couple weeks ago. image

orefalo commented 1 month ago

I think I understood what you meant by new tab... you mean maximizing starblitz internal browser, which opens a new tab CleanShot 2024-10-05 at 18 15 23

Now I do see a funny effect, looks like the rendering flickers and is overlayed by a blank area, to be honest, I doubt this is related to splitpane as I can see an IFRAME overlayed in the html rendering CleanShot 2024-10-05 at 18 17 24

I don't see any of the errors you are pointing, this is what i get... CleanShot 2024-10-05 at 18 20 49

hopperelec commented 1 month ago

I don't see any of the errors you are pointing, this is what i get...

Oh, those are more likely to be related to StackBlitz itself; check the browser console with it open in a new tab and you should see the console actually for the svelte-mosaic example page

hopperelec commented 1 month ago

to be honest, I doubt this is related to splitpane as I can see an IFRAME overlayed in the html rendering

Not sure where that IFRAME is coming from, but it is completely blank and deleting it does not make a difference, so I don't think this is the issue

orefalo commented 1 month ago

ok.. I see it now. weird.

Not sure where that IFRAME is coming from, but it is completely blank and deleting it does not make a difference, so I don't think this is the issue

No idea what Starblitz is doing, Do you know how to export the project? I want to test locally.. I have no trust in Starblitz

hopperelec commented 1 month ago

Do you know how to export the project? I want to test locally

StackBlitz has a "Download as ZIP" button at the top of the explorer pane, but the easier way to test locally would be to just clone the svelte-mosaic PR https://github.com/hopperelec/svelte-mosaic/tree/splitpanes

orefalo commented 1 month ago

the git repo above is the bundled version, wont' work

trying to download,

hopperelec commented 1 month ago

What do you mean by "the bundled version"?

orefalo commented 1 month ago

bundled - you are not using splipanes as a dependency. i don't want to debug code bundling.

I can't download this damn project, I hate starblitz

hopperelec commented 1 month ago

you are not using splipanes as a dependency

I am? It is using version 0.8.2 right now, though. The only place I used it bundled was in the first StackBlitz REPL; the git repo I linked to does not do that

orefalo commented 1 month ago

this is your package.json - there is no refs to splitpanes,

CleanShot 2024-10-05 at 18 48 25

How can I get the 2nd REPL from starblitz as a zip file?

hopperelec commented 1 month ago

this is your package.json - there is no refs to splitpanes,

It looks like you cloned the master branch, not the splitpanes branch I linked to. svelte-mosaic used to handle the panes on its own, but the PR I created is to migrate it to using splitpanes. The PR hasn't been merged yet because of the issues, so it still lives on a separate branch.

How can I get the 2nd REPL from starblitz as a zip file?

image image

orefalo commented 1 month ago

my bad, got it now. interesting... looking into it

orefalo commented 1 month ago

The issue has been identified, and version 8.0.8 is being released.

Thank you for highlighting this issue. The root cause remains unclear, but the problem has been resolved.

Please try, confirm and close this ticket.

hopperelec commented 1 month ago

Thanks so much for looking into the issue! Sadly, it still doesn't seem to be fully working for me- it is very similar to 8.0.6

However, in Firefox, the initial size now gets precise if the container is resized, which wasn't the case before.

To re-iterate for clarity:

I should also add for full clarity that, in both browsers, it shows as 50% width very briefly before going to the initial size I mentioned. This has been the case the whole time and I assume is just part of the rendering process. I mention this since you mentioned the rendering "flickering" earlier.

orefalo commented 1 month ago

Morning, Correct I can reproduce what you are describing above.... but I thought it was per design ;-/

I changed your code as such -

    $: { initial = containerSizePx && getInitial()

        console.log("initial="+initial)
    };

CleanShot 2024-10-06 at 10 05 19

my point is, there is no way the component can render proper if the size is undefined. am I missing something?

hopperelec commented 1 month ago

Undefined should be the default value, meaning it should invoke the default behaviour, no? I have used undefined for panes where a specific size is not provided.

orefalo commented 1 month ago

The only way to "hook" into the default behavior is to

  1. render the dom
  2. hook a reactive variable on the splitter position we do both, which explains the ugly flicker effect that you get..

There is no way to ask the browser the default spitter position upfront.

As a consequence, as of now, you need to explicitly set a default pane size.

orefalo commented 1 month ago

Turning this into a feature request,

hopperelec commented 1 month ago

There is no way to ask the browser the default spitter position upfront.

I'm confused- why would it be controlled by the browser? If I don't set the size at all (which should evaluate to undefined), it works fine. But if I explicitly set it to undefined, it doesn't work? Surely this is something being done by splitpanes itself?

orefalo commented 1 month ago

and I am puzzled... ;-)

That's a valid point. I didn't try it the other day, so I restarted your project to remove the size for testing purposes.

and... I can't reproduce the bug?! I am puzzled...