95401 - Implement resizable popups

Status: RESOLVED DUPLICATE of bug 511180

(Core :: XUL, defect, P2)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

For floating windows, palettes, resizable autocomplete dropdown, etc. 

Patch in a moment.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: resurrected patch

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

This is a patch from around December 2000, brought up to date with the current
build. I've ifdef'ed out some code in nsPopupBoxObject.cpp for metric gathering
because this functionality has since been added to nsBoxObject (where it should
be), however I've left the code here for now to remind myself to make the
nsBoxObject code stash info in attributes. 

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: better patch

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

hyatt, sr=?

Comment 5

[David Hyatt]

I'm working on popups right now (bringing them up to spec), and my patch
conflicts heavily with this patch.  I think we're largely doing the same thing
(I'm also defining an nsIPopupBoxObject), but I worry that the re-architecture
will affect how this has to be implemented.  I'd rather finish that (which
should only take 1-2 more days), land, and then land this shortly after that.

Comment 6

[David Hyatt]

Specifically I'm allowing <popup>, <tooltip> and <menupopup> to be definable
anywhere without needing a popupset.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: progress... still not quite done. merged with dave's changes.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

The one item awaiting resolution is the use of dimension/position attributes on
the popup node to specify positioning and size. 

Here's my proposal:

- if width, height, left or top are specified in attributes, these values are
used as the position and dimensions at which to launch and size the popup to.
This is to support persistence of these values which will be of use to UI
elements such as palette windows and floating toolbars. 

- if these attributes are not set (in all other cases, e.g. tooltips, menus,
context menus), the values supplied to ShowPopup etc are used. 

The patch supplied makes nsPopupBoxObject::ShowPopup obey |left| and |top|
during creation, however it does not yet make the popup size to the values of
|width| and |height|. Nor are these values dynamic - the popup does not move or
size when you set these attributes to new values. 

To be consistent with other XUL, this could be supported although it's not
likely to be the best way to resize a popup smoothly. Experience with this has
shown the need to support sizing in two dimensions at once, hence |sizeTo| and
|moveTo|. These methods also minimize FE notification during attribute set. As a
result of these methods, I don't know that we want to support dynamic attribute
listening... 
I'm marking this as a blocker as it is blocking progress in creation of a
generic inline edit facility, which is required in the Bookmarks Upgrade
currently under way. Bug for this feature coming soon...

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

After discussion with hyatt, a todo list before completion:

- provide pass-thrus for the new nsIPopupBoxObject methods on the popup binding
in XML, in preparation for a finalized nsIDOMXULPopupElement interface.
- implement nsIDocumentObserver on nsMenuPopupFrame to listen for left/top
attribute changes so as to shift the popup around when the attributes are changed. 
- make 'width' and 'height' fields on BoxObject return attribute value first, if
present.
- make 'screenX' and 'screenY' fields on BoxObject return attribute value (left
and top) first, if present. (*)
- implement 'left' and 'top' mappings for nsIDOMXULElement

(*) a potential problem here. .left and .top traditionally refer to client
coordinates. Thus far I've been using them as screen coordinates which is
probably wrong. Better solutions are:

Screen Coords       Client Coords
.screenX            .left (or .x, to be consistent with the interface)
.screenY            .top  (or .y, similarly)

Comment 10

[David Hyatt]

Ben, boxes already look for left and top and dirty themselves accordingly. 
width/height/left/top should all be taken care of automatically.   They dirty
for a reflow, which will then call RepositionPopup, and you presumably look for
left/top there.

So this should just work.

Comment 11

[David Hyatt]

use left and top.  for a popup it makes sense that they mean screen.

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

But the getters for .x, .y, .screenX and .screenY are on boxobject, not on
popupboxobject. Do you propose I override these methods in the popupboxobject
case and use a different attribute? 

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

Don't mind me, I'm confusing myself. New patch shortly.

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: dom/content/layout changes.

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

OK Dave, ready for your take on this stuff. I'd like to get the back end changes
in, then work on the details of the actual XBL binding. 

Comment 16

[David Hyatt]

I'll look at this later today.

Comment 17

[David Hyatt]

sr=hyatt, but hold off until 0.9.5.

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

summarizing suggestions:

- code in ::Init routine to position popup intially based on attributes
- add err2 in cases where ToInteger return code is checked for validity
- add persist, ref, datasources as well to XULElement
- refer to inconsistency in left/top for popups/other elts in idl.

new patch tomorrow. 

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

jag also requests if (NS_FAILED(err)) return err; in preference to if 
(NS_SUCCEEDED(err)) { stuff }

Comment 20

[jag (Peter Annema)]

More specifically, I requested this be rewritten:

+  return NS_SUCCEEDED(err) ? popupSet->ShowPopup(srcContent, 
+                                                 popupContent, 
+                                                 aXPos, aYPos, 
+                                                 popupType, 
+                                                 anchorAlign,
+                                                 popupAlign) : err;

to:

if (NS_FAILED(err))
  return err;

return popupSet->ShowPopup(srcContent, popupContent,
                           aXPos, aYPos, popupType, 
                           anchorAlign, popupAlign);

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: latest patch

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: test case for MoveTo function and left/top attributes

Comment 23

[Ben Goodger (use ben at mozilla dot org for email)]

jag sez 'r=jag'

Comment 24

[Ben Goodger (use ben at mozilla dot org for email)]

back end changes checked in. 

As I don't plan on using a popup for inline edit anymore, at this stage the
0.9.5 section of this bug is complete. Shifting the remainder of the work to
.9.7 for now. 

Comment 25

[Ben Goodger (use ben at mozilla dot org for email)]

No longer a blocker. 

Comment 26

[Alfred Kayser]

two years further, still a lot of code commented, so in there
but not used. Will this be ever used? 
If not, then please remove it from the tree(s)...

Comment 27

[Ben Goodger (use ben at mozilla dot org for email)]

Mass reassign of my non-Firefox bugs to [e-mail]

Comment 28

[Ben Goodger (use ben at mozilla dot org for email)]

Mass reassign of my non-Firefox bugs to [e-mail]

Comment 29

[Ben Goodger (use ben at mozilla dot org for email)]

Let's not remove code that there may be a use for in XUL2.0. If we don't
identify any use for this in XUL2.0, then it can be removed. But for now, let it
stay, at least in toolkit/content. 

Comment 30

[Neil Deakin]

This was fixed by bug 511180.