Opened 12 years ago

Closed 7 years ago

Last modified 6 years ago

#5777 closed enhancement (fixed)

popups: move/close when window resized / scrolled

Reported by: guest Owned by: bill
Priority: high Milestone: 1.9
Component: Dijit Version: 1.0
Keywords: popup moving place Cc: ben hockey
Blocked By: Blocking:

Description (last modified by bill)

I guess the problem is related to ticket #5776.

Some of the widgets use dijit.popup to show some content attached to the widget, e.g. TimeTextBox shows _TimePicker. When you have the content of the page centered (or right-aligned I guess) resizing the browser window make all the page elements move. But the popups do not move, since they are positioned with:

// In dijit/base/_place.js:155-156 you can read:
node.style.left = best.x + "px";
node.style.top = best.y + "px";

In my opinion they should be somehow sticked to the parent widget, so that they move when the parents move.

Maybe some trick with position:relative would do the job?

Regards, Grzegorz Olędzki

Attachments (1)

popup.diff (1.4 KB) - added by Paul Christopher 7 years ago.
Patch for dijit/popup.js against 1.8.0 [CLA on file]

Download all attachments as: .zip

Change History (44)

comment:1 Changed 12 years ago by bill

Milestone: 2.0
Owner: set to bill
Summary: Popups should be placed relatively to aroundNodespopups: close when window resized

I'd just like to close the popups when the window is resized. See also #4600.

comment:2 Changed 12 years ago by alex

Milestone: 2.01.3

Milestone 2.0 deleted

comment:3 Changed 11 years ago by bill

Description: modified (diff)
Milestone: 1.31.4

comment:4 Changed 10 years ago by bill

I've been experimenting and deliberating over this.

I'm starting to feel uneasy about just closing a popup on resize, as that may be bad for the user, especially if it's a dialog-esque popup and they've typed some data into it.

However, it's hard to reposition the popup for a number of reasons:

  1. Resizing the viewport may actually make the aroundNode go off screen. This can be for simple cases like a page with a long form, or when there are multiple scrollbars like the ComboBox inside of a BorderContainer pane in themeTester.html
  2. when you open a popup, the popup API tells the caller where the popup was positioned (top/bottom/left/right). There's no API to notify the caller that the popup was moved.
  3. sizing: ComboBox typically has a tall drop-down which it resizes (thus adding a scrollbar) when there isn't room to put the whole thing in the viewport. Resizing the browser window means that the drop down needs to be resized too, but there's no API to relay that to the ComboBox code.

comment:5 Changed 10 years ago by bill

Milestone: 1.41.5

comment:6 Changed 9 years ago by Adam Peller

Milestone: 1.51.6

comment:7 Changed 9 years ago by bill

Fixing this will also fix #10875.

comment:8 Changed 9 years ago by bill

Resolution: fixed
Status: newclosed

(In [22546]) Close popups when browser window is resized/scrolled, or when a nested div (ex: pane in a BorderContainer) is scrolled. Fixes #5777, #10547, #10875 !strict.

comment:9 Changed 9 years ago by bill

(In [22752]) Ignore spurious resize events that occur on IE. The spurious resize events were causing Menu_mouse.html etc. to prematurely close the menu, but only when run from the test suite (via runTests.html), not when run stand alone from Menu_mouse.html. Refs #5777 !strict.

comment:10 Changed 9 years ago by bill

(In [22761]) Temporary fix to ignore spurious onscroll events that occur on IE. The spurious onscroll events were causing Select.html plus ComboBox tests to prematurely close the drop down.

This check in does not completely fix the Select.html test problem since that test has a Select with a tall drop down (taller than the viewport) which causes the whole page to scroll when one of the elements gets focused. That will be fixed in #10631.

Refs #5777 !strict.

comment:11 Changed 9 years ago by bill

(In [23812]) Don't close popup when the scrollwheel is turned over the popup, in order to scroll the popup. Fixes #12297, refs #5777 !strict.

comment:12 Changed 9 years ago by bill

(In [23866]) Changes related to closing/repositioning popups on scroll / resize:

  • Rollback (erase) code to close popups on window resize (refs #5777, fixes #12317) and mouse wheel (refs #12297).
  • When possible, don't close popups when the browser window is scrolled using the mouse on the browser scroll bar. Popups still close by clicking anywhere else on the page, including inner scrollbars. Unfortunately on IE6 & 7, clicking the browser scrollbar also still causes popups to close.
  • In RTL mode, make place.js set popup.style.right rather than popup.style.left, so that on screen resize (at least in the common case) the popup and the aroundNode stay aligned. (refs #10875)

Background:

  • It's often harmful that resizing a window closes the popup; the user may be expanding the window to see all of the popup. Although resizing can cause a reflow, separating the aroundNode from the popup, but often it doesn't. The case of centered/right-aligned elements mentioned in #5777 is an exception.
  • Closing popups on scroll is also questionable. Scrolling the main window will scroll the aroundNode and the popup together, so it's counterproductive to close the popup. OTOH, scrolling a <div> will separate the aroundNode from the popup. And for a mousewheel event it's hard to tell whether it's affecting the main window or a nested <div>.

In the future, I'll add code to actually reposition popups when a browser window resize or a scroll has caused the aroundNode to move.

!strict.

comment:13 Changed 9 years ago by bill

Description: modified (diff)
Milestone: 1.6future
Resolution: fixed
Status: closedreopened
Summary: popups: close when window resizedpopups: move/close when window resized

comment:14 Changed 8 years ago by bill

Summary: popups: move/close when window resizedpopups: move/close when window resized / scrolled

comment:15 Changed 8 years ago by bill

Note that the problem can also happen when scrolling an inner div, see #15023.

comment:16 Changed 8 years ago by bill

#15023 is a duplicate of this ticket.

comment:17 Changed 8 years ago by bill

#15326 is a duplicate of this ticket.

comment:18 Changed 8 years ago by Paul Christopher

A similar issue appeared in dojox.mobile and has been fixed in

http://bugs.dojotoolkit.org/ticket/14096

http://bugs.dojotoolkit.org/ticket/14488

http://bugs.dojotoolkit.org/ticket/14489

Is this code/approach reusable for this issue somehow?

comment:19 Changed 7 years ago by bill

Well, it's the same basic approach as #14096, except that the code goes into dijit/popup rather than client classes like ComboBox. Also, I was thinking of monitoring for scroll / resize events rather than using a timer, but either approach is probably fine.

comment:20 Changed 7 years ago by bill

#15388 is a duplicate of this ticket.

comment:21 Changed 7 years ago by Paul Christopher

Not sure about this: Besides resizing/scrolling there are other "events" which make a DOM node move: inserting new DOM nodes, toggling (showing/hiding) a DIV etc. Maybe in most cases, the popup might be closed in these cases. But if not? Popup is a generic class widget authors can reuse as they like...

comment:22 Changed 7 years ago by bill

Yeah, I agree, probably a timer is better.

comment:23 Changed 7 years ago by Paul Christopher

Dragging a dialog does have the same effects like resize or scroll, see http://bugs.dojotoolkit.org/ticket/15602: It calls for repositioning of the drop down.

comment:24 Changed 7 years ago by bill

#15667 is a duplicate of this ticket.

comment:25 Changed 7 years ago by ben hockey

Cc: ben hockey added

comment:26 Changed 7 years ago by Paul Christopher

Since this is such a long standing issue (5 years now) and several persons have reported it in the meanwhile as a "new bug" again and again (see list of duplicates),I now have added a proposal for a patch. Repositioning "works", but as a JS and Dojo newbie, I cannot judge whether the approach is really okay (efficient, fast, scalable etc.).

IMHO repositioning should work for tooltips, too, e.g. for invalid form fields when the window gets resized. However the above patch does not work with tooltips for some reason...

comment:27 Changed 7 years ago by Paul Christopher

Maybe it is not enough just to reposition the popup but calculate also the remaining space and orientation (below, above, left, right)?

comment:28 Changed 7 years ago by bill

Well, yes, to have a 100% solution the popup might need to be reoriented, basically all the code for positioning needs to be rerun including callbacks to the widget itself like orient().

BTW Tooltip doesn't use dijit/popup; it calls dijit/place directly, which is why your code isn't affecting it.

comment:29 Changed 7 years ago by bill

PS: I suppose it can get even more complicated, for example for a dropdown (ex: for ComboBox) where the number of entries displayed was tailored to the amount of space available on the screen. I thought there was some API allowing this but I need to go back and check.

comment:30 Changed 7 years ago by bill

#16213 is a duplicate of this ticket.

Changed 7 years ago by Paul Christopher

Attachment: popup.diff added

Patch for dijit/popup.js against 1.8.0 [CLA on file]

comment:31 Changed 7 years ago by Paul Christopher

As mentioned by Bill on http://bugs.dojotoolkit.org/ticket/15370#comment:14 : Updated patch and fixed paths and tabbing. Hopefully, the patch is more usable now... Still learning ;-)

comment:32 Changed 7 years ago by Paul Christopher

As Bill said, http://bugs.dojotoolkit.org/ticket/16335 maybe shows, that just repositioning the drop down is not enough but also needs orientation check and resizing of the drop down?

comment:33 Changed 7 years ago by cmketchu

Hi, I am running into the issues mentioned in this bug. I was hoping for an update on any progress made towards a fix or new patches and workarounds.

comment:34 Changed 7 years ago by bill

Milestone: future1.9
Status: reopenedassigned

Thanks Paul, that patch looks good in principle. I'm going to try a modified version of it that only sets up one timer, and uses it to process the whole popup stack. Plus, probably add checking to see if stack[0].aroundNode has moved or not.

comment:35 Changed 7 years ago by bill

Resolution: fixed
Status: assignedclosed

In [30911]:

Move popup (or stack of popups) when the aroundNode is moved. It doesn't try to reposition the popups (ex: moving a ComboBox's dropdown from below the aroundNode to above the aroundNode, but just simply moves them up/down (and left/right) the same amount that the aroundNode moved.

Fixes #5777 !strict.

comment:36 Changed 7 years ago by bill

In [31013]:

guard code from when clearTimeout() doesn't work on IE8, refs #5777 !strict.

comment:37 Changed 7 years ago by bill

In [31024]:

remove stray console.log() from [31013], refs #5777 !strict.

comment:38 Changed 7 years ago by cmketchu

#15388 was marked as a duplicate of this bug. It does not look like it was addressed with the fix checked in here. Will a fix be provided with this bug or could we reopen the original request?

Last edited 7 years ago by bill (previous) (diff)

comment:39 Changed 7 years ago by bill

That's true, OK I'll reopen that ticket.

comment:40 Changed 6 years ago by Bill Keese <bill@…>

In eecfebfd88c6c56b38b8015f62e2b3ea4ebb9287/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:41 Changed 6 years ago by Bill Keese <bill@…>

In 56edb1d133f6f8912198a046911718d5c3d1e292/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:42 Changed 6 years ago by Bill Keese <bill@…>

In f27741d04f4843e9adb8dcacea61749c9938f618/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:43 Changed 6 years ago by Bill Keese <bill@…>

In a8daca52ecd362aa119a9aab4409a4b43e95fabf/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.