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

Resizable: Modified the default z-index value of resizable handles. Fix... #840

Closed

Conversation

tjvantoll
Copy link
Member

...ed #7960 - Dialog: Modal dialogs do not disable resizables on the page.

See http://bugs.jqueryui.com/ticket/7960.

I was a little torn about adding the inline comment. 90 is kind of a random number so I thought having a reference might help.

…ixed #7960 - Dialog: Modal dialogs do not disable resizables on the page.
@mikesherov
Copy link
Member

Unit tests?

@@ -42,7 +42,7 @@ $.widget("ui.resizable", $.ui.mouse, {
maxWidth: null,
minHeight: 10,
minWidth: 10,
zIndex: 1000
zIndex: 90 // See #7960
Copy link
Member

Choose a reason for hiding this comment

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

stylistically, top down comments are preferred to EOL comments:

// See #7960
zIndex: 90

@tjvantoll
Copy link
Member Author

@mikesherov Good call. I'll add a test that makes sure the z-index of a resizable is less than the modal dialog's overlay.

@tjvantoll
Copy link
Member Author

@mikesherov I changed the formatting and added a test.

@mikesherov
Copy link
Member

Much better! In general, I'll only land pull requests without tests if its not practical to do so. Thanks for updating.

@gnarf
Copy link
Member

gnarf commented Nov 25, 2012

:sigh: - zIndex cc @scottgonzalez

Do we really need a zIndex at all here? I know we've been evaluating our (ab)use of zIndex in other places.

@mikesherov
Copy link
Member

zIndex wars are like !important wars. Programatic zIndex assignment would be better IMO, but what I really personally care about is the test. As long as these are getting covered, we can try any approach you like :)

@tjvantoll
Copy link
Member Author

@gnarf37 Yeah I guess I've never really considered why the resizable plugin blindly assignes a z-index in the first place.

z-index is only used in two places in the entire file.

  • Line 120 simply applies the zIndex option value to all handles.
  • Line 579 adds the zIndex plus 1 to the proxy wrapper element if it's necessary.

I did some testing and if I simply remove those lines the plugin still works just fine. That being said I only tried some basic use cases on Chrome.

If no one knows of why this option is needed in the first place I can test this more thoroughly (hit all the options, browsers, etc).

@mikesherov
Copy link
Member

I'm, as usual, against a complete removal of zIndex until we do a full git blame to see where and why it was introduced.

@tjvantoll
Copy link
Member Author

@mikesherov The option has been in there since the beginning of git time. The initial version in git has pretty the exact same 2 lines I reference above. That being said I'm sure there must be a reason, and I'm guessing it's related to the proxy element.

I'm mostly curious at this point.

@scottgonzalez
Copy link
Member

I think it's just because we want the handles to be on top of all content, even if the content has a z-index applied. This should really just be moved to the stylesheet and the option should be dropped, but that won't happen until 2.0.

@gnarf
Copy link
Member

gnarf commented Nov 25, 2012

@scottgonzalez thanks for the direction - Do we have a ticket to track these zIndex uses for removal in 2.0?

@scottgonzalez
Copy link
Member

@gnarf37 The tickets will be created when we announce the API redesign for resizable. The change in this PR will just go against the dialog ticket.

@mikesherov
Copy link
Member

Ok, then I'm going to land as is.

@jzaefferer
Copy link
Member

Landed in 0cd470b - squashed three commits into one, fixed whitespace in test, included button widget along with dialog dependency.

@jzaefferer jzaefferer closed this Nov 26, 2012
@scottgonzalez
Copy link
Member

Not sure why I missed this before, the test should be in dialog, not resizable.

@jzaefferer
Copy link
Member

Fixed in ee8d20e

@tjvantoll
Copy link
Member Author

Thanks @jzaefferer.

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.

None yet

5 participants