fix dropdown items going off screen

[garethnic]

This was an issue involving the drop down menus containing too many items that leaves "excess" items going off screen and not being able to see/select them.

My laptop screen resolution is 1366x768.

The code makes the drop down menus scrollable when it reaches the max height.

[garethnic]

The max-height value would be better served as a percentage instead of px. Can't believe that slipped passed me.

[garethnic]

I've played with % and so far that doesn't seem to work for some reason. Not sure if it only works when given a fixed value.

[peterflynn]

@garethnic Fyi, someone else has started working on this recently too: https://groups.google.com/forum/#!topic/brackets-dev/BjcG9bF98l4

Hardcoded heights are problematic because not everyone's window is going to be the same height as yours. Ideally you could use percentages, but if not you'll need to calculate the max-height dynamically when opening the menu (or after every window resize, but on open seems preferable performance-wise...).

[garethnic]

Thanks for the link. I see Tuur went the JS route first and I'm trying to do it in CSS. Different approaches I guess. If Tuur can solve this then I'd be happy. I fully agree on using percentages, it breaks the menu bar though, so I'm going to have to figure it out somehow. I'm gonna see what I come up with.

[redmunds]

@garethnic Brackets uses native menus on Mac and Windows, and HTML menus on Linux (because native are not yet supported). Since this should only affect HTML menus, then I'm guessing that you are on Linux, right? FYI, as soon as someone implements native menus for Linux, we'll switch to those.

Even for HTML, the max height should be set to the Brackets app height. My monitor has a height of 1200px, so I wouldn't want menus cut off at 700px. If you can figure out how to do that with CSS that's great, otherwise it'll have to be done with JS. Have you tried max-height: 100vh?

Also, I don't think you want overflow: hidden as that will truncate the menu. Try overflow: auto which will add scroll bars if (and only if) necessary.

UPDATE: overflow: hidden does work, but I'm not sure why :)

[redmunds]

Good News -- max-height: 100vh works! Bad News is that it sets height to be 100% of app height, but top of menu is offset, so it's truncated (by that same offset amount) on the bottom. So, you'll need to use something like max-height: 90vh or max-height: 100vh - 50px.

UPDATE: math using different units is not supported in LESS

[TomMalbran]

We can use max-height: calc(100vh - 50px);. The code is actually using overflow-x: hidden, I would still change that to make it clear.

[garethnic]

@redmunds Yeah, I'm on Ubuntu. Any idea on when the native menus will happen in Linux? I'll be trying out your ( @redmunds & @TomMalbran ) suggestions. I was hoping someone could explain why overflow-x: hidden works because I wasn't expecting that behaviour.

[redmunds]

@TomMalbran Ah, I forgot the calc() part. But, I did not forget that it doesn't seem to work in LESS. I think LESS overrides that and it assumes that all units are the same as the first length. It may have something to do with the fact that Brackets is still on LESS v1.4. Let me know if you can get it to work.

Note: to test HTML Menus on Windows or Mac, make this edit to utils/Global.js

    // Are we in a desktop shell with a native menu bar?
//    var hasNativeMenus = params.get("hasNativeMenus");
//    if (hasNativeMenus) {
//        global.brackets.nativeMenus = (hasNativeMenus === "true");
//    } else {
//        global.brackets.nativeMenus = (!global.brackets.inBrowser && (global.brackets.platform !== "linux"));
//    }
    global.brackets.nativeMenus = false;

@garethnic Maybe you should just change it to overflow-x: auto.

[redmunds]

This is for #7496 which is tagged for Sprint 39 so marked as Medium Priority.

[TomMalbran]

@redmunds Right I forgot that LESS will always try to do the calculations, so we need to use: max-height: calc(~"100vh - 50px");. calc is a CSS function that will calculate the length at runtime, so it will work. The issue was that LESS was trying to perform the operation at compile time. Using ~"..." will make LESS not touch the operation, so now it should work.

We kind of want the overflow-y to scroll.

[redmunds]

@TomMalbran

We kind of want the overflow-y to scroll.

Ooops. I had x/y mixed up -- that explains it. overflow-x should not be a part of this pull request.

@garethnic FYI, 50px was just a guess, so you'll need to take a closer look at the actual value to use.

[TomMalbran]

@redmunds Exactly. I am thinking that Chrome might have changed the value of overflow-y to auto when it changed the value of overflow-x. But I am not sure.

We should have a LESS variable for the height of the menu, and it will hopefully work in the code.

[redmunds]

We should have a LESS variable for the height of the menu, and it will hopefully work in the code.

Also need a variable for height of titlebar, so that can also be subtracted from viewport height.

Note that these heights may be different on different OS'es. Maybe just use 90vh for now :)

[TomMalbran]

I thought that the viewport sizes didn't added the size of the chrome window, and it was just the size of the actual tab?

[garethnic]

I'm having issues getting calc to work. Oddly enough - max-height: 3000% works. Though that just looks wrong. overflow: auto also works fine.

[TomMalbran]

There is a bug with vw units inside calc in blink, which is fixed in Chrome 34: https://code.google.com/p/chromium/issues/detail?id=168840. But Brackets uses an older version, even after the CEF update, which will bump Brackets to use chromium v33.

The solution is to use 90vh, although it fails in really small windows and could work better in larger ones, but is good enough, or use JavaScript to calculate the height.

[marcelgerber]

@garethnic Try calc(~"100vh - 50px")
http://stackoverflow.com/a/11973298

[TomMalbran]

@SAplayer Read my comment, is not an issue with LESS but with Chrome.

[peterflynn]

We may want to put this in a more specific selector -- in addition to the top menu bar, .dropdown-menu is used for everything from code hints to the recent projects dropdown and other "dropdown buttons" in the UI. It's possible we'd break something in one of those other cases by making this change apply to all of them.

[peterflynn]

The other option is to actually do something in JS at the time the popup is opened -- calculating a max-size dynamically based on its vertical position. That would make the fix apply to all of these cases instead of just the menu bar, and it would eliminate the fuzziness with 90% only approximating the y offset.

PopUpManager.addPopUp() might be a viable bottleneck if we wanted to take that approach.

[marcelgerber]

@peterflynn But we'd need to recalculate on resize.

[TomMalbran]

From my testings with using max-height, the menu didn't changed size after resizing the window. So Recalculating on resize might still be required and a good option.

Also, the 90vh, or even using calc (if it was possible) will not work for context menus, or menus like the recent projects one which pop from a different position.

[redmunds]

I played around with this and discovered exact window offset to be 34px, but I couldn't get calc(~"100vh - 34px") to work.

Since I haven't heard back from @garethnic, I went ahead and submitted pull request #7731 with a more isolated fix that I'd like to get in for this Sprint.

[TomMalbran]

No one read my Blink comment (#7693 (comment)) and still tried to play with calc? hehe

[redmunds]

@garethnic Thanks for the pull request, but we trying to close out Sprint 39, so I went ahead and made the changes (for html main menus) discussed above in a separate pull request. I am going to keep the original issue open so this also gets fixed for native main menus.

@TomMalbran Good find! Yeah, I was too busy yesterday to read it :)

Closing.