work in progress about viewer resize behavior #2256
Conversation
There was a problem hiding this comment.
Cool! I had no idea that ResizeObserver existed. Looks like it's pretty well covered by our target browsers, but I support continuing to have the fallback. But yeah, using ResizeObserver will be a nice efficiency improvement!
Thank you for the demo page... It's a lot easier to understand what's going on that way. My observations:
With the 3.1 technique and preserve image size off, it seems the width and height resizes both resize the image down, but then don't resize it back up. That seems like a bug in the existing code.
With the new technique and preserve image size off, the width one resizes the image down and back up properly. The height one, however, doesn't resize the image down. That seems like a bug in the new code.
I can't get the bottom demo to produce the problem you describe. I have "use pulling" on and "preserve" off, and I hit the next/previous page buttons, and everything looks fine. Sounds like it's an intermittent error that only happens sometimes?
Is your proposed fix that the developer needs to add a forceResize call in their own code, or are you suggesting that we add that to the Viewer so it happens automatically? Ideally, it shouldn't require extra work for the developer.
|
@iangilman Thanks for reviewing!
The only potential sticking point I could see in terms of a change in behavior would be that if a resize happens during
The existing behavior of the viewer defines the scale/zoom of the image based on width, which is why in the "new code" resizing only the height doesn't rescale the image. So, I don't think it's a "bug," but whether this is actually desirable or not, I'm not sure. I share your feeling that it isn't necessarily intuitive... but it seems like this has been a longstanding "feature" of the system, so I worry that changing this might break things some how.
To clarify what the problematic behavior looks like, when I page right/forward, which causes a narrower viewer element because of a wider sidebar than the previous image, the images start out less than full height of the viewer. (Actually, they really do start at full height because of If you aren't seeing this behavior that is interesting. I can provide a video if that would be useful. In my testing, using the
Calling |
I think I like
How about we always do the resize in
True. In fact, I suppose the reason the image stays the same height in your issue demo when you hit the left arrow is because of this. I do think it would be an improvement though, to make it consistent horizontally and vertically, so I think it's worth making the change if we can.
I see! I suppose that was happening before and I just didn't know what to look for. I am definitely seeing it now. This seems like a specific enough scenario that I don't think I would want OSD to try to automatically account for it itself. I think if I was the developer of an app that needed this, I would add the sidebar in and put |
I can get on board with
This seems like a good idea to me. I had pulled it out of
I'm happy to work on implementing something, but it's not obvious what the right answer is to me just yet. I think it would be useful to put the specific behavior you have in mind into words. How would you propose handling width-only, height-only, and both-but-changed-aspect-ratio cases, while shrinking and growing?
That makes sense. In the demo it's less obvious than in my actual use-case, but I wanted both the viewer and controls to have enough space to be usable.
Having gone through the trouble of trying to automate it to some degree, I agree that we probably can just get by with |
I'm thinking in all of those cases when the viewer shrinks, so does the image, and when the viewer grows again, so does the image. Currently, when the width changes, it already does that, but when only the height changes, it does not. Furthermore, and this is the tricky part: If, after it shrinks, you grow the viewer back to its original size/shape, the image should be exactly the same size it was before the shrink/grow. I guess the real question is "by how much should it shrink/grow?" Right now, it's scaling down the image proportional to how much the viewport width is scaling down. Maybe it should look at the proportion the viewport height is scaling, and if it's scaling down more, use that to scale down the image? The challenge, of course, is that the image maintains its aspect ratio, whereas the viewport can change its aspect ratio. Actually, maybe it would be better to scale it down by the lesser of the two scales? That way it's not as disruptive. You might end up with something chopped off, but it's a zooming system, so maybe that's okay? Then, in terms of scaling back up, I guess it's whatever reverses the process properly. What do you think? BTW, we don't have to go down this rabbit hole if you don't want to... This is just where we've gotten to, starting from your new technique :) |
|
@iangilman I updated the PR with a proposed modification to the scaling behavior. I ended up going with a scale factor based on the diagonal of the viewer, which I think looks and feels nice and intuitive, and ends up with the same view when returned to the same viewer dimensions (barring any actual navigation of course). I added a demo mode with a manually resizable viewer (the new bottom button). It's a little hard to see the I also implemented the suggestion to just set a flag for Thoughts on next steps? (I'm not sure why the build is failing) |
There was a problem hiding this comment.
Beautiful! Of course, the diagonal is the right choice! It just hadn't occurred to me.
The new resizing feels really nice. Thank you for providing the free resizing demo.
I'd say this is all good, but it still needs some cleanup. We absolutely should keep this new demo page, but I think we should remove the old versus new toggle and all the code that supports that.
The failing build is:
https://app.travis-ci.com/github/openseadragon/openseadragon/builds/258833878
Can you see that, or do you not have access?
At any rate, it looks like it's the navigator tests that are failing. Can you merge in the latest from master, and see if that fixes it?
|
@iangilman The tests were actually useful! It turns out the new resize behavior had changed how the navigator was working. The simple fix is just to disable |
| var oldBounds = viewport.getBounds(); | ||
| viewport.resize(containerSize, true); | ||
| viewport.fitBoundsWithConstraints(oldBounds, true); | ||
| if (viewer.autoResize || THIS[viewer.hash].forceResize){ |
There was a problem hiding this comment.
It doesn't seem like we need forceResize... This can just be a check for needsResize (along with the autoResize check, of course).
There was a problem hiding this comment.
I thought the point of forceResize (as it currently stands, at least) is to allow a developer a way to trigger (force) the resize behavior even if autoResize==false. That's why I added a second flag, because otherwise if autoResize==false then needsResize is ignored.
There was a problem hiding this comment.
Yes, that's the behavior we want. Now that you mention it, if you take my suggestion and get rid of the forceResize flag, you would want to check for autoResize in the ResizeObserver callback.
I guess the key difference would be what would happen if you had autoResize off, resized the viewer, then turned it back on. With the separate forceResize flag, it could remember that it got resized and therefore immediately update when autoResize got turned back on. By using needsResize for the forceResize functionality, in that scenario it wouldn't remember that it needed to resize, and therefore it wouldn't resize when you turned autoResize on again.
I mentioned it because it seemed like a little bit of a pain to have to manage both of those flags, but now I think I've talked myself into wanting them both.
What do you think? If you agree, then I think we're good here and I can merge it. :)
There was a problem hiding this comment.
I agree. I think keeping the two flags for managing two different behaviors/usages makes sense. Since they're internal to Viewer and used for pretty straightforward purposes, I don't think it should be too painful to keep up with the logic of what's going on for future development. Also, I think your description of the behavior with the two flags - that when autoResize is turned on, it should resize immediately if needed - makes good sense for how it should be expected to work.
So, I'm good with moving forward with merging as long as you're satisfied with everything!
* master: (276 commits) Changelog for #2280 and #2238 remove trailing space fix problem with click precision on ReferenceStrip Changelog for #2273 Also add documentation for tileRetryDelay try fix with check for null and undefined fix build error Add tileRetryMax documentation. Revert async support and event breaking support in EventSource. Changelog for #2276 add box-sizing property to the navigator display region Implement support for async function and promise type recognition with $.type. Add $.Promise proxy. Implement support for promises in EventSource. Implement ability to abort events as well as prioritize events. Changelog for #2270 issues/2192 fix. Starting 4.0.1 Version 4.0.0 JSDoc fixes Changelog for #2256 Changelog for #2249 removed polling vs resizeviewer option from demo ...
In one of the apps I'm working on, I noticed some strange and sometimes inconsistent behavior of a viewer when it's size was changed at the same time that it switched to a new tilesource. Tracking down the root of this has sent me down a bit of a rabbit hole about how viewer resizing is handled.
This PR is currently in draft mode, and the code includes some changes that are intended just for comparative testing of before/after behavior, which would need to be removed if any of this moves forward towards merging.
Three distinct issues/proposals are presented here for consideration.
forceResize().ResizeObserver(where supported).I have created a new demo which is available at http://localhost:8000/test/demo/resizeviewer.html when running the development environment, which allows testing viewer behavior with various combinations of parameters/code versions. I hope the demo page is largely self-explanatory, but I expect there will be plenty of questions and discussion around this.