Skip to content

fixes ticket #5916, the incredible shrinking ie8 dialog #269

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

Closed
wants to merge 1 commit into from

Conversation

fracmak
Copy link
Contributor

@fracmak fracmak commented May 13, 2011

fixes ticket #5916, when the dialog uses a padding with an em unit that rounds up, the dialog slowly shrinks because we're resetting the height() whenever a drag event happens. This checkin adds a unit test showing this behavior and also removes the height() tinkering which stops the dialog from shrinking

…at rounds up, the dialog slowly shrinks because we're resetting the height() whenever a drag event happens. This checkin removes the height() tinkering which stops the dialog from shrinking
@scottgonzalez
Copy link
Member

The height setting is a feature so you can hide all the contents of the dialog during drag.

@fracmak
Copy link
Contributor Author

fracmak commented May 13, 2011

Wouldn't that be better done using the "ui-dialog-dragging" css class? or having the user track/restore the height themselves?

@scottgonzalez
Copy link
Member

Yes, the styling should be done via the class, but if you hide the content, you need the dialog to stay the same size. As for having users track the height themselves, if we can't get it right, why would they?

@fracmak
Copy link
Contributor Author

fracmak commented May 13, 2011

hmm, good point. An alternate idea is to measure the height of the content area and set that back to it's original value instead of wrapping dialog (which uses em's quite a bit). That way there's much less chance of rounding errors. Thoughts?

@scottgonzalez
Copy link
Member

I think what people do is .ui-dialog-dragging .ui-dialog-content { display: none; } so setting the height on the content wouldn't help there. I suppose they could change the rule to .ui-dialog-dragging .ui-dialog-content * but we'd need to test the performance of that. Does grabbing the value of .style.height help instead of using .height()?

@fracmak
Copy link
Contributor Author

fracmak commented May 13, 2011

Yup, that works just fine, reverted my original fix, and put in the newer code change. It now just restored the original value of this.style.height so the value is unmodified from beginning to end of drag. I left the same height() calculation to make certain "height: auto" didn't screw up the display

@scottgonzalez
Copy link
Member

Just tested in IE8 and I'm still seeing the dialog shrink.

@fracmak
Copy link
Contributor Author

fracmak commented May 13, 2011

is the unit test failing?

@scottgonzalez
Copy link
Member

No, the unit test is passing. I just tested again and here's what happening: When you start dragging the dialog grows by 1px, then when you stop it shrinks by 1px. So the final height is correct, but the dialog does change size during drag.

If you can figure out how to get it to stay the same height to whole time, that'd be ideal, but if not we can still land this since the overall effect is better than what we have now.

@fracmak
Copy link
Contributor Author

fracmak commented May 16, 2011

Ah, well I doubt we can/should fix any of that in the dialog code, this is more of an issue with the jquery .height() function. When you calculate the height of 0.22em, modern browsers give you a fractional height like 3.52px, but IE8 is rounding that up. The height() function would need to add both the padding-top and padding-bottom together if they're not in pixels then convert them to pixels. I'll open a ticket in the core and see what comes of it

@scottgonzalez
Copy link
Member

Thanks, after some debate we decided that we're ok with your original solution of just ripping this code out. Now that we have a mostly working solution that we can point people to, there's no need for us to build this in. Can you go ahead and just remove the code that saves and set the height?

@fracmak
Copy link
Contributor Author

fracmak commented May 27, 2011

Done, I've rollback the code to the original fix

@scottgonzalez
Copy link
Member

Thanks, landed in e34dbfe. I dropped the test since we no longer attempt to even do anything anymore.

@calexander3
Copy link

This is still an issue with IE 8 using jqueryUI 1.8.21.

@scottgonzalez
Copy link
Member

@calexander3 Yes, that's correct. The ticket has a milestone of 1.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants