-
Notifications
You must be signed in to change notification settings - Fork 187
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
fix for #18839: place.around() now works correct in fullscreen mode #117
Conversation
Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA). 📝 Please visit http://contribute.jquery.org/CLA/ to sign. After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know. If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check. |
It also requires similar css-rule for popups to be displayed correctly over fullscreen divs. It is not in the commit, because I am not sure, whether applying this globally will break something in dojo or not. It looks like it shouldn't. .dijitPopup {
z-index: 2147483647 !important;
} |
OK. Thanks for the patch. As I wrote in the ticket, if you can include a test case for when the current code is failing I'll look at it. One other issue with your patch though is that https://developer.mozilla.org/en-US/docs/Web/API/Document/mozFullScreen says:
You seem to be violating that rule. (But again, I don't even understand what problem you are trying to fix, since the code seems to work in fullscreen mode for me already.) |
I don't see how I am violating the rule. I am using
I will be able to send you an example on Tuesday. Basically, this code will fail, if there is an aroud div inside a div in fullscreen mode with geometry like {1800, 732, 32, 32}, but one of fullscreen div's parents has {width: 800px, height: 600px} and located somewhere left on the page. This results in negative width and height anchor values after computing positions of visible parts of the anchor. |
OK, I look forward to the test case. About the MDN page, I was just worried because the MDN page says to "check whether document.fullscreenElement is non-null", but you are actually accessing the value. But I suppose that's OK. |
It looks for me that you took this wrong. It says, that there is no So, the paragraph just recommends us to not use |
There is a bug demo source: https://pastebin.mozilla.org/8870456 Note, that it is reproducible in Chrome and in Opera, but not in FF. |
That https://pastebin.mozilla.org/8870456 example link doesn't work [anymore]. It would actually be ideal if you could add an automated test case to the pull request. Otherwise someone else will have to do it before accepting the PR. You mentioned creating a CSS rule to set zIndex on |
@39dotyt would love to finally land this, but still waiting on an automated test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs automated test per comments.
Closing this for extended inactivity and lack of a test case. Running Dijit's existing test file ( |
https://bugs.dojotoolkit.org/ticket/18839