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

Menu clipping #749

Merged
merged 5 commits into from
Feb 14, 2018
Merged

Menu clipping #749

merged 5 commits into from
Feb 14, 2018

Conversation

SteveTheTechie
Copy link
Contributor

@SteveTheTechie SteveTheTechie commented Feb 11, 2018

Resolves #739

  1. Add zIndex option to allow overriding menu z-index for edge use cases.
  2. Add logic to assign a reasonable z-index if not appending to a jQuery UI Dialog. (e.g., appending to document.body instead)
  3. Go back to setting the menu position on every open in case the button has been moved (e.g. container dialog moved) from the last open.
  4. Close the menu if the mouse cursor is outside the menu and the mouse wheel is turned. This fixes a scrolling issue that made the menu look detached from the button when the mouse wheel was turned.
  5. Add _allowInteraction support for jQuery modal dialogs to prevent interaction issues when appending to document body to prevent menu truncation.

Note these changes do not directly fix the menu truncation issue. However, they do make it much easier to address menu truncation by just explicitly setting the appendTo option to document.body, since the z-index would automatically get set to a reasonable value in that case. The _allowInteraction support is needed for this use case since typically jQuery UI modal dialogs only allow interactions with elements that are contained in the dialog. (When appending the menu to document.body, typically this would be a case where normal interaction with the menu would be expected to be blocked by the jQuery UI modal dialog code.)

1.  Add zIndex option to allow overriding menu z-index for edge use cases.
2.  Add logic to assign a reasonable z-index if not appending to a jQuery UI Dialog.  (e.g., appending to document.body instead)
3.  Go back to setting the menu position on every open in case the button has been moved (e.g. container dialog moved) from the last open.
4.  Close the menu if the mouse cursor is outside the menu and the mouse wheel is turned.   This fixes a scrolling issue that made the menu look detached from the button when the mouse wheel was turned.
5.  Add _allowInteraction support for jQuery modal dialogs to prevent interaction issues when appending to document body to prevent menu truncation.

Note these changes do not explicitly fix the menu truncation issue.  However, they do make it much easier to address menu truncation by just explicitly setting the appendTo option to document.body.
@mlh758
Copy link
Collaborator

mlh758 commented Feb 12, 2018

It looks like these changes are bringing the z-index calculation in, but not changing the append behavior yet?

@SteveTheTechie
Copy link
Contributor Author

SteveTheTechie commented Feb 12, 2018

@mlh758 This is a compromise that lets you "have it both ways". Essentially, I am not explicitly changing the append behavior as it is fine for the situation where you append to a "tall" dialog where clipping would not occur. I have looked at the jQuery UI code, so I have an idea of what they are doing w/ .ui-front Re: stacking order.

However, while the original code did allow the appendTo element to be explicitly specified for some use cases (e.g. append to document body for short dialogs), it really did not provide sufficient support for setting the z-index properly for that use case and it did not address modal dialog interaction for that use case.

In essence, this PR does not really change anything for the appendTo ".ui-front use case", as I think that is probably fine for many situations if you also set the position within property for the inner scrolling div of the dialog. What this PR addresses is the appendTo's that are not '.ui-front'... a reasonable z-index is set for those or you can override it. Also, the _allowInteraction extension is set for jQuery UI dialogs to prevent interaction problems.

I came to the conclusion in my thinking that there really was no good reliable way to have the code automatically determine whether to appendTo .ui-front or document body, so we should leave that decision to a developer. We should provide some guidance in a wiki to assist that decision.

@SteveTheTechie
Copy link
Contributor Author

@mlh758 Do you have any specific requests for changes?

@mlh758
Copy link
Collaborator

mlh758 commented Feb 13, 2018

No, I've just been really busy the last couple of days and haven't had a chance to finish testing it.

@mlh758 mlh758 merged commit 0a340f3 into ehynds:version3 Feb 14, 2018
@mlh758
Copy link
Collaborator

mlh758 commented Feb 14, 2018

Looks good, merged it. I updated the wiki for the appendTo option to describe the behavior and truncation issue.

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

Successfully merging this pull request may close these issues.

2 participants