Rewrite Modals.setScrollbar() and Modals.resetScrollbar() to properly handle padding-right of body and fixed elements #18441
Conversation
|
Travis is failing because some of your indentation is incorrect. See the output. |
js/src/modal.js
Outdated
| // Adjust Fixed Content Padding | ||
| $(Selector.FIXED_CONTENT).map((index, element) => { | ||
| let fixedPadding = $(element).css('padding-right') | ||
| this._originalPadding[index] = fixedPadding |
There was a problem hiding this comment.
Store the original padding in a data-* attribute on each element instead of in _originalPadding
|
Comments should use sentence casing, not title casing. |
|
Unit tests should be added to prevent future regressions. See https://github.com/twbs/bootstrap/blob/v4-dev/js/tests/README.md for info. |
|
I still need to write some extra unit tests for the newly inserted data-* attributes. The pre-existing unit tests checked whether the body padding was properly changed and restored and they still work fine. Also, if a script changes the dom while a modal is open, it can optionally remove the data-* attribute to prevent it from resetting after the modal is closed. Should we reference that somewhere? |
Yes, but it was getting changed to the wrong value. The unit tests ought to have detected that, so they're deficient in some respect.
IMHO, no. I wouldn't call it a feature, and libraries which remove random data-* attributes do so at their own peril. |
|
The unit tests only check whether the dom changes as expected; they don't care how it's done internally, which is why they don't fail after my modifications. I will add tests for the data-* values, which will in practice also monitor the internal operations. Beyond that I think anything else would be overkill. |
|
Added unit tests for body padding. |
|
Would love a review of this if it's still valid, and possibly a rebase if it's good to go so we can merge. |
|
@mdo, rebase and squash, plus minor changes are at 9ea8274, however the following unit tests are failing. I'll need to review this further. @deilv, for future reference, |
|
@bardiharborow thanks, I totally missed that. 9ea8274 looks good. |
|
@bardiharborow I took a look at this PR again and I'm unsure how to proceed. I see your 9ea8274 commit, but I don't know how to fetch that. You also mentioned reviewing this further. Any chance you can revisit this? Happy to help however I can to see it merged. |
|
@mdo I can rebase and cleanup the PR, but I'm not sure how I can cleanly include @bardiharborow's commit for proper credit. Please advise. |
|
@deilv oh don't worry about credit, it's not an issue. The main issue here is that the unit tests fail when I rebase this. If you could look into that and fix the issues, then we'll get this merged. |
|
I will get this fixed asap |
|
@cvrebert @bardiharborow Hm, this is weird. It's been over a year and now I can no longer replicate this issue. Either another commit or the upgrade to jquery3 seem to have fixed the issue. Can anyone verify? |
|
Closing this PR as no longer relevant. Issue #18373 should probably be closed as well. |
|
IT seems the issue is reproduced here #21700. Maybe this PR is still relevant? |
|
Indeed, when the body is large enough the issue still occurs. I will get this sorted. |
|
I encountered the same issue with the unit test. Those tests do not work because |
|
@vanduynslagerp have you figured out why those two tests were needed in the first place? |
|
The test are used to:
It was added in PR #17536 even though, due to the absence of I kept those test in #21800 and I added the bootstrap css to the Qunit page so they now work properly. In addition I made the necessary change in the other unit test due to the presence of the bootstrap styles (ie. it fixed a test in tooltip as well). You created the PR first, and I created one because there was no response on this thread for a while and it didn't pass the test at that time. Now that you're back, feel free to reuse the code from mine regarding the unit tests if you're interested and I'll close it to avoid the dupe. |
|
IMHO each test should be individual of everything else and should only be linked to a specific part of code. That said adding a css file and modifying another test to keep things working is only looking for trouble next time someone makes a seemingly unrelated change. Thus I'd rather have those two tests removed and reviewed, so they can be written properly if needed. I'll wait to see what mdo thinks about this. Note: the changes in the tests appear messy in the online editor, but they're actually quite straight. Two tests are added and two are removed. |
|
Restored the removed tests and will spend some more time to rewrite them so they work properly. I also found that we need to include a fix for the collapse button which is also affected by the padding issue. |
|
Ok, here is what happened so far. Commit 3af4560 was meant to fix #17399, and then commit c01fa6b added tests for those changes. As it turns out, the issue still exists and the tests don't even come close to what they intended to do (I'm amazed they didn't fail in the first place). This PR completely rewrites the problematic modal functions, solving all padding issues and making both previous commits obsolete. |
|
You still don't have any test that check the modified code works because the class |
|
@vanduynslagerp this PR is about fixing a padding issue, not adding a test for a private function. If you want to work on that on a separate PR, you're welcome. Please keep in mind that unit tests are meant to test the logic in javascript, so adding css in the mix may not be desired. @mdo @bardiharborow Please review this and send me your feedback. There still one issue to solve, but it's not an easy one. When using a fixed navbar, the absolutely positioned navbar-toggler button does not respect the parent's padding, so when the modal opens it moves to the right. I think a css solution is the most clean one, but I haven't found it yet. The alternative would be to have modal.js manipulate margin-right instead of padding-right, which solves all problems but the user may notice an empty strip under the scrollbar. Any ideas? |
The fix of the padding issue is tested by the 2 unit tests that you removed.
The logic in javascript (in _getScrollbarWidth) depends on the css class
I did: #21800 |
Actually no, those 2 unit tests were related to a code change that I reversed and are no longer relevant. There are other tests in place already. Remember these are unit tests, they have a very specific scope.
If you read the title, this PR has nothing to do with _getScrollbarWidth(). Each commit should focus on a very specific change, so it's easier to track issues in the future.
No you did not actually. You copied the code contributed by me and bardiharborow, and then rigged the tests to make them always succeed; since the code failed to add padding, you added it yourself and then checked if it was there. Really? Please stop spamming this thread. Really, please. |
|
Breathe. @deilv, your prior comment seems to indicate that you take attribution fairly seriously. This is not in any way a problem. @vanduynslagerp, understandably, the PR system is a little clunky and sometimes it is easier to just re-create a PR than to work with the existing PR owner. It isn't as much of a problem for me, as often the PR creator will have allowed edits from maintainers, but I do understand. Additionally, your PRs often cross several issues, which while quite common given the casual atmosphere of Bootstrap, is probably technically incorrect. The combination of these things seems to have caused a bit of a dispute, and it is specifically why I'm quite paranoid about maintaining commit's author value when I do have to recreate PRs. Now I'm not in a position to comment on the technical content of either PR, as I still haven't had the time to understand the actual issue it's fixing. I'm sorry about this, I've just been in the middle of some family issues. Rest assured that the PR I end up merging will include correct attribution for whoever's code I end up using. Thank you for contributing to Bootstrap, and I hope that things will work smoothly from here. I'm reachable on Slack anytime, if you'd like to discuss any of this, or just Bootstrap in general. Peace out. ❤️ |
|
Hi @bardiharborow and thank you for your comment. I hope your family issues are resolved soon. Honestly, this isn't about credit. The comment was about your rights, for the work you did in commit 9ea8274. I couldn't care less who's name shows up in the merge. However, I really enjoy using bootstrap and I'm trying to do my part to keep code quality high. Anyway, I still need some feedback on how to best solve the navbar-toggler padding issue. I already have a working solution in my codepen, but this will require existing users to add a navbar-wrapper around the button (which may not be a big issue while in alpha stage). The alternative is to adjust the button's position through javascript, like all other fixed elements. The first way is more elegant, the second is more hacky. Please reply so I can push another commit with the complete solution. PS. Edits from maintainers have been enabled for quite some time, feel free to play along. @Johann-S can you please elaborate on whether the tests you added in c01fa6b are still relevant? The affected code has be completely replaced in this PR. |
|
Updated the initial post of this PR. You can see all changes live in my codepen including the fixed navbar-toggler. |
|
@deilv my tests aren't relevant anymore 👍 you did great job |
|
Updated commit to include the latest changes promised earlier. All tests pass. Everything looks good in all browsers I tested. I would like to do one more optimization pass and add some reference to navbar-wrapper in the docs. Some feedback would be great at this point. |
|
Yet another update to this PR. When opening/closing a modal with the navbar-toggler present, its right margin is adjusted (instead of padding) which means it does not break compatibility (ie users won't have to change their code to add a parent div). More tests were added, too. @bardiharborow Any chance you can have a look at this? It'd be shame to let it stall for another year. |
|
Hi @deilv, is it possible to you to update your branch ? And I'll review your PR asap thank you |
|
@Johann-S The branch is now up-to-date, no conflicts. All tests pass locally. |
…when opening or closing modal
|
When opening a modal popup on a page with navbar fixed top, the page below the navbar still get wrong horizontal adjustment (padding) compared to the navbar above (Bootstrap v4.0.0-beta, IE 11, Windows 10). Please see #22792 |
Fixes #18373, Fixes #20824, Fixes #21700, Fixes #21840 (duplicates)
The problem:
_setScrollbar() and _resetScrollbar() in file js/src/modal.js failed to properly adjust padding-right when opening and closing modals, while using fixed elements (ie fixed-top navbar). Both functions had to be rewritten.
Changes:
_setScrollbar() -- called when opening a modal
_resetScrollbar() -- called when closing a modal
Unit Tests
You can see the changes live in codepen.