Opened 9 years ago
Closed 5 years ago
#15606 closed defect (patchwelcome)
[ccla] dijit/_HasDropDown - with maxHeight -1, properly calculate height post-display
Reported by: | Brian Arnold | Owned by: | bill |
---|---|---|---|
Priority: | high | Milestone: | 1.13 |
Component: | Dijit | Version: | 1.7.3 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
This work is covered under both SitePen?'s CCLA and my personal CLA.
We have a simple example where some style is being added to the dijitMenuPopup
class that adds additional height to the node, but the maximum height is being calculated based on the size of the node before those classes are in play.
There is also a note that says the following in _HasDropDown.js:
// TODO: isn't maxHeight dependent on the return value from dijit.popup.open(), // ie, dependent on how much space is available (BK)
This patch looks to resolve that TODO. It checks if maxHeight is in play, and if a resize of height was performed before (as there are scenarios where the dropdown isn't overheight and so the height resize isn't necessary) and then it recalculates the size.
It's a little frustrating that it does have potential to cause two resizes in a row, but the classes in play aren't actually on the wrapping nodes until the menu is opened, so it seems that there's not much choice.
I'm attaching a reduced test case that demonstrates the issue (use a 1024x768 browser size or smaller to see it) as well as a patch. This patch is written against the 1.7 branch, but looking at trunk, it should be able to easily be applied to both the 1.7 branch as well as trunk.
Attachments (2)
Change History (9)
Changed 9 years ago by
Attachment: | maxHeight-miscalculation.html added |
---|
Changed 9 years ago by
Attachment: | _HasDropDown-maxHeight.patch added |
---|
A patch to add recalculation to height
comment:1 Changed 9 years ago by
Summary: | dijit/_HasDropDown - with maxHeight -1, properly calculate height post-display → [ccla] dijit/_HasDropDown - with maxHeight -1, properly calculate height post-display |
---|
comment:2 Changed 9 years ago by
Cc: | bill removed |
---|---|
Milestone: | 1.7.4 → 1.8 |
I see, you're adding a border etc. on dijitMenuPopup. I wasn't expecting anyone to do that, although admittedly I added some other styling in [21266].
I think what you are doing is technically possible by creating a wrapper widget, but admittedly that's heavy-handed.
comment:3 Changed 9 years ago by
Brian, if you can update your patch to change dijit/tests/popup.html to test a tall popup with a border, I'll check this in. (You shouldn't make popup.html depend on high level widgets like dijit/form/Select or dijit/Menu since dijit/popup is infrastructure code.)
comment:4 Changed 9 years ago by
Milestone: | 1.8 → 2.0 |
---|
comment:5 Changed 8 years ago by
Priority: | undecided → high |
---|
comment:7 Changed 5 years ago by
Milestone: | 2.0 → 1.12 |
---|---|
Resolution: | → patchwelcome |
Status: | new → closed |
Given that no one has shown interest in creating a patch in the past 3+ years, I'm closing this as patchwelcome.
A reduced test case that demonstrates the problem against the 1.7 branch