552982 - titlebars for panel

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Add support for titlebars on <panel> elements for floating tool windows.

An attribute titlebar added to panels will indicate that a titlebar should be displayed. The default value is none. Other values are 'normal' and 'thin.

See https://wiki.mozilla.org/XUL:Panel_Improvements.

Comment 1

[Neil Deakin]

Attachment: basic patch

This is a very basic patch showing this feature on Mac. The patch isn't correct though.

The issue is that the positions don't update properly, more so as the popup is moved around by dragging the titlebar.

I think the issue may be related to the widget and view positioning not expecting the titlebar to be there and calculating the popup size incorrectly. I'm not familiar enough with how the positions and sizes are determined to know what the solution here is.

Comment 2

[Rob Campbell [:rc] (:robcee)]

I'm going to take a look at this.

for better positioning, should I use an unanchored panel and set an absolute initial position rather than locking it to a particular xul widget?

Comment 3

[Rob Campbell [:rc] (:robcee)]

interesting. Using "normal" creates a titlebar and lets you drag the panel around. I see what you mean by the positions not getting updated properly. At least I think that's why my tree widget isn't functional with this enabled. I'll watch for updates to this bug and try them out as they come in.

thanks!

Comment 4

[Neil Deakin]

Attachment: updated work-in-progress patch

This work-in-progress patch adds titlebar to panel support on Windows and Mac and should fix the panel jumping around problems.

Still to do:
 - Linux support
 - decide which coordinate systems should really be used
 - remove the hard-coded computation in nsView

The normal/thin feature isn't currently implemented. Just use titlebar="normal" for now. On Windows, you'll get a thin titlebar, on Mac a normal one. This is just a matter of it being the default for popups in the native platform api.

Comment 5

[Neil Deakin]

Attachment: work in progress 2

This large patch combines several smaller patches which:
 - fix nsXULPopupManager::AdjustPopupsOnWindowChange to only move popup in the same window that just moved
 - fixes up coordinate handling when a popup or its containing window is moved or resized
 - adds a level="floating" attribute for panels to make them floating popups
 - add support for titlebar="normal" on panels. Thin/normal distinction is not yet implemented.
 - tests for the much of the above
 - fixes the setting of the widget's mBounds field on Mac

Comment 6

[Neil Deakin]

Attachment: work in progress 3

This patch:
 - fixes various Linux coordinate and positioning issues
 - changes level handling a bit; now only titlebar="normal" is needed, the level attribute doesn't need to be set
 - adds support for close buttons (<panel close="true">)
 - adds a backdrag="true" which allows dragging popups by their background. This doesn't yet integrate with popups with titlebars
 - fixes a number of tests

Comment 7

[Neil Deakin]

Also, panel titles are now supported using <panel label="Tools">

Comment 8

[Rob Campbell [:rc] (:robcee)]

Titlebars look good, though I'm craving the "thin" bars. :)

The "jumping" effect I noticed in earlier patches is gone on OS X. Positioning is stable.

The height of the menu bar appears to be "eating into" the total height of the panel. I have an hbox with a spacer and a resizer in it at the bottom of these panels and they're being obscured as well as part of the scrollbar's down arrow. (see attached screenshot).

Testing both floating and top positions and they work as advertised.

This is really coming together nicely!

Comment 9

[Rob Campbell [:rc] (:robcee)]

Attachment: Titlebar screenshot, Mac OS X

one other comment, I'm using some CSS to make these panels partially transparent when actively inspecting. They don't seem to become transparent anymore with this patch in place.

I'm using a class and a corresponding rule in CSS to set the transparency.

Comment 10

[Rob Campbell [:rc] (:robcee)]

Attachment: panel-titlebars-inspector

not a minimal patch, but if you want to see what this is doing, apply the attached patch to:

hg.mozilla.org/users/rcampbell_mozilla.com/mozilla-inspector-3

Comment 11

[Neil Deakin]

(In reply to comment #9)
> Created an attachment (id=453754) [details]
> Titlebar screenshot, Mac OS X
> 
> one other comment, I'm using some CSS to make these panels partially
> transparent when actively inspecting. They don't seem to become transparent
> anymore with this patch in place.
> 
> I'm using a class and a corresponding rule in CSS to set the transparency.

I tested using style="opacity: 0.4;" and this works ok.

I've fixed the cropping at the bottom for the next patch.

Comment 12

[Neil Deakin]

Attachment: Implement panels with titlebars, complete patch

This patch implements this feature and includes all of the following patches

Comment 13

[Neil Deakin]

Attachment: Part 1: change nsXULPopupManager::AdjustPopupsOnWindowChange so that it only adjusts popups associated with the same window

Comment 14

[Neil Deakin]

Attachment: Part 2: update the popup when it is moved or resized natively

Comment 15

[Neil Deakin]

Attachment: Part 3: support panel levels

Supports three levels on noautohide panels: top, floating and parent

Comment 16

[Neil Deakin]

Attachment: Part 4: change nsXULPopupManager::GetVisiblePopups to also return noautohide panels

Comment 17

[Neil Deakin]

Attachment: Part 5: support for titlebars on panels

This is done via <panel noautohide="true" titlebar="true">

Comment 18

[Neil Deakin]

Attachment: Part 6: Mac widget bounds fixes

Comment 19

[Neil Deakin]

Attachment: Part 7: Hacky GTK widget changes

Comment 20

[Neil Deakin]

Attachment: Part 8: support titlebar labels

Comment 21

[Neil Deakin]

Attachment: Part 9: support for titlebar close buttons

Comment 22

[Neil Deakin]

Attachment: All Windows changes from the previous parts, for easy reviewing

Comment 23

[Neil Deakin]

Attachment: All Mac widget changes from the previous parts, for easy reviewing

Comment 24

[Neil Deakin]

Attachment: All GTK changes from the previous parts, for easy reviewing

Comment 25

[[e-mail]]

Comment on attachment 455729 [details] [diff] [review]
Part 8: support titlebar labels

>+  PRBool setTitle = PR_FALSE;
>   if (mContent && widgetData.mNoAutoHide) {
>     if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::titlebar,
>                               nsGkAtoms::normal, eCaseMatters)) {
>       widgetData.mBorderStyle = eBorderStyle_title;
>+      setTitle = PR_TRUE;
...
>+  if (setTitle) {
>+    nsAutoString title;
>+    mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::label, title);
>+    if (!title.IsEmpty()) {
>+      widget->SetTitle(title);
At first I thought why bother with a boolean when you can query the border style. But then I thought why not just fetch the title up front?

nsAutoString title;
if (mContent && widgetData.mNoAutoHide) {
  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::titlebar,
                            nsGkAtoms::normal, eCaseMatters)) {
    widgetData.mBorderStyle = eBorderStyle_title;
    mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::label, title);
...
if (!title.IsEmpty()) {
  widget->SetTitle(title);

>+        if (!title.IsEmpty()) {
>+          widget->SetTitle(title);
Although I'm not sure why you feel it necessary to special-case empty titles.

>+      <property name="label" onget="return this.getAttribute('label');"
>+                                onset="this.setAttribute('label', val); return val;"/>
Nit: onset doesn't align with onget.

Comment 26

[[e-mail]]

Comment on attachment 455730 [details] [diff] [review]
Part 9: support for titlebar close buttons

>+        widgetData.mBorderStyle =
>+          static_cast<enum nsBorderStyle>(widgetData.mBorderStyle | eBorderStyle_close);
Does
widgetData.mBorderStyle |= eBorderStyle_close;
not work?

>+      if (mBorderStyle & eBorderStyle_close) {
>+        style |= WS_SYSMENU;
>+      }
Or you could add the style in the eWindowType_popup case, since it will get automatically removed if the eBorderStyle_close style is not set.

Comment 28

[Josh Aas]

Comment on attachment 455736 [details] [diff] [review]
All Mac widget changes from the previous parts, for easy reviewing

>-      // if a parent was supplied, set its child window. This will cause the
>-      // child window to appear above the parent and move when the parent
>-      // does. Setting this needs to happen after the _setWindowNumber calls
>-      // above, otherwise the window doesn't focus properly.
>-      if (nativeParentWindow)
>+      // if this is a popup at the parent level was supplied, set its child
>+      // window. This will cause the child window to appear above the parent

This first sentence appears to be written incorrectly. Please capitalize it like a proper sentence too.

>+void nsCocoaWindow::SetPopupWindowLevel()
>+{
>+  // Floating popups are at the floating level and hide when the window is
>+  // deactivated.
>+  if (mPopupLevel == ePopupLevelFloating) {
>+    [mWindow setLevel: NSFloatingWindowLevel];
>+    [mWindow setHidesOnDeactivate: YES];
>+  }
>+  else {
>+    // Otherwise, this is a top-level or parent popup. Parent popups always
>+    // appear just above their parent and essentially ignore the level.
>+    [mWindow setLevel: NSPopUpMenuWindowLevel];
>+    [mWindow setHidesOnDeactivate: NO];

No spaces between ":" and the argument itself in Gecko Obj-C code.

Comment 29

[Mats Palmgren (inactive)]

Comment on attachment 455723 [details] [diff] [review]
Part 2: update the popup when it is moved or resized natively

In layout/xul/base/src/nsMenuPopupFrame.cpp
>  nsMenuPopupFrame::MoveTo(PRInt32 aLeft, PRInt32 aTop, PRBool aUpdateAttrs)

Assert !IsAnchored() at the start of this method?

In layout/xul/base/src/nsMenuPopupFrame.h
+  PRBool IsAnchored() { return mScreenXPos == -1 && mScreenYPos == -1; }

Make it a const method.

+  nsIntPoint ScreenPosition() { return nsIntPoint(mScreenXPos, mScreenYPos); }

Make it a const method.

In layout/xul/base/src/nsXULPopupManager.cpp
+void
+nsXULPopupManager::PopupPositionedOrResized(nsIView* aView, PRBool aResized,
+                                            nsIntRect aRect)

I would prefer if this was split into two methods, one for move and one
for resize.  The first part that finds a visible frame can be shared by a
static function.

+  nsPresContext* presContext = menuPopupFrame->PresContext();
+  if (!presContext);
+    return;

unnecessary null check

In view/src/nsViewManager.cpp
+static PRBool
+PopupPositionedOrResized(nsIView* aView, nsGUIEvent* aEvent, PRBool aResized,
+                         PRInt32 aWidth, PRInt32 aHeight)

I would prefer a function that just get the nsXULPopupManager* ...

+  else if (PopupPositionedOrResized(aView, aEvent, PR_TRUE, width, height))

and this to use that (if non-null) and then call pm->PopupResized

+  if (aView && PopupPositionedOrResized(aView, aEvent, PR_FALSE, 0, 0))

same, then call pm->PopupMoved

Comment 30

[Mats Palmgren (inactive)]

Comment on attachment 455724 [details] [diff] [review]
Part 3: support panel levels

In layout/xul/base/src/nsMenuPopupFrame.cpp:
> +nsPopupLevel
> +nsMenuPopupFrame::Level(PRBool aIsNoAutoHide)

Name it nsMenuPopupFrame::PopupLevel instead?
(Level is a too generic term)

Can it be a const method?

In layout/xul/base/src/nsMenuPopupFrame.h:
> +  nsPopupLevel Level(PRBool aIsNoAutoHide);

Maybe it would be better to have two methods, a public method taking no args
and the above one being protected.
(the param is just a perf optimization, used internally, right?)

Comment 31

[Mats Palmgren (inactive)]

Comment on attachment 455726 [details] [diff] [review]
Part 5: support for titlebars on panels

In widget/public/nsIWidget.h:

+    virtual nsIntPoint WindowToClientOffset(const nsIntRect& aWindowRect) = 0;
+    virtual nsIntSize ClientToWindowSize(const nsIntSize& aClientSize) = 0;

const methods?

widget/src/gtk2/nsWindow.cpp:
+                // popups should be resizable via their borders
+//                gtk_window_set_resizable(GTK_WINDOW(mShell), PR_FALSE);

I don't understand why you add this.

Comment 32

[Rob Campbell [:rc] (:robcee)]

This is awesome. Thanks for the reviews, mats!

I would love to see this landed for b2 on all platforms.

Comment 33

[Jim Mathies [:jimm]]

Comment on attachment 455734 [details] [diff] [review]
All Windows changes from the previous parts, for easy reviewing

WindowToClientOffset could probably be replaced by GetClientOffset. r+ with that sorted out.

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

We're going to need to get this into a beta in order to get developer feedback - I suggest beta3 (so, next two weeks) but am willing to get input on whether that's reasonable or not before making the call.

Comment 35

[Rob Campbell [:rc] (:robcee)]

I'd love to get this in ASAP so I can make use of it in the Inspector. See, bug 574408. Not sure if it's feasible to get these reviews this week, but I'd love to see it happen.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+     * Given the specified window rectangle including borders and titlebar,
+     * return the corresponding client size, which excludes the size of the
+     * borders and titlebar. This method should work even when the window
+     * is not yet visible.
+     */
+    virtual nsIntPoint WindowToClientOffset(const nsIntRect& aWindowRect) = 0;

If this returns a size, why isn't it returning an nsIntSize? Should we return the entire client rect, perhaps?

Comment 37

[Neil Deakin]

(In reply to comment #36)
> If this returns a size, why isn't it returning an nsIntSize? Should we return
> the entire client rect, perhaps?

The value is an absolute screen position. The callers only need the topleft corner but we could return the entire size anyway. I'll fix the comment so as not to be as misleading.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, I'd like to see the revised patch.

Comment 39

[Karl Tomlinson (:karlt)]

Comment on attachment 455726 [details] [diff] [review]
Part 5: support for titlebars on panels

>+     * Given the specified window rectangle including borders and titlebar,
>+     * return the corresponding client size, which excludes the size of the
>+     * borders and titlebar. This method should work even when the window
>+     * is not yet visible.
>+     */
>+    virtual nsIntPoint WindowToClientOffset(const nsIntRect& aWindowRect) = 0;
>+
>+    /**
>+     * Given the specified client size, return the corresponding window size,
>+     * which includes the area for the borders and titlebar. This method
>+     * should work even when the window is not yet visible.
>+     */
>+    virtual nsIntSize ClientToWindowSize(const nsIntSize& aClientSize) = 0;

Are both these and GetNonClientMargin() required?

Do you know what the "window origin" for GetClientOffset() is?
Is that meant to be the top-left of the frame (screen) or client bounds?

Comment 40

[Karl Tomlinson (:karlt)]

Comment on attachment 455737 [details] [diff] [review]
All GTK changes from the previous parts, for easy reviewing

The decorations for these noautohide titlebar popups will not look like they
are active, but the browser window will become active when the user attempts
to activate the popup.  Just checking that's the intention here.

Where is IsPopupWithTitleBar defined?

> NS_IMETHODIMP
> nsWindow::GetScreenBounds(nsIntRect &aRect)
> {
>-    aRect = nsIntRect(WidgetToScreenOffset(), mBounds.Size());
>+    if (IsPopupWithTitleBar()) {
>+        // use the point including window decorations
>+        gint x, y;
>+        gdk_window_get_root_origin(GTK_WIDGET(mContainer)->window, &x, &y);
>+        aRect.MoveTo(x, y);
>+    }
>+    else {
>+        aRect.MoveTo(WidgetToScreenOffset());
>+    }
>+    aRect.SizeTo(mBounds.Size());

This still has the client size, so the bottom-right is actually inside the
client area, but that's no change from the existing behaviour.

If you feel like fixing this, using gdk_window_get_frame_extents for toplevel
widgets would give the frame bounds.

> nsIntPoint
> nsWindow::WidgetToScreenOffset()

>-        gdk_window_get_root_origin(GTK_WIDGET(mContainer)->window,
>-                                   &x, &y);
>+        if (IsPopupWithTitleBar()) {
>+            gdk_window_get_origin(GTK_WIDGET(mContainer)->window, &x, &y);
>+        }
>+        else {
>+            gdk_window_get_root_origin(GTK_WIDGET(mContainer)->window, &x, &y);
>+        }
>         LOG(("WidgetToScreenOffset (container) %d %d\n", x, y));
>     }

Do you know why this returns (and has returned) the top-left of the frame
(instead of the client top-left) for leading top-level windows?

Why should popups be different?

nsIWidget::WidgetToScreenOffset doc's say "widget's origin".
Again, I don't know which origin that is.

>     // Toplevel windows need to have their bounds set so that we can
>     // keep track of our location.  It's not often that the x,y is set
>     // by the layout engine.  Width and height are set elsewhere.
>+    nsIntPoint pnt(aEvent->x, aEvent->y);
>     if (mIsTopLevel) {
>         mPlaced = PR_TRUE;
>         // Need to translate this into the right coordinates
>         mBounds.MoveTo(WidgetToScreenOffset());
>+        pnt = mBounds.TopLeft();
>     }

This code has me even more confused, the code without your changes.  I thought
mBounds was the client rectangle top-left.  It's size set in OnSizeAllocate is
the client size.

You are changing this because you want the frame top-left?
That seems OK, but mBounds.MoveTo(WidgetToScreenOffset()) disturbs me.

>-                gtk_window_set_decorated(gtkWin, FALSE);
>+                if (mBorderStyle == eBorderStyle_default || !(mBorderStyle & eBorderStyle_title)) {
>+                  gtk_window_set_decorated(GTK_WINDOW(mShell), FALSE);
>+                  gtk_window_set_deletable(GTK_WINDOW(mShell), mBorderStyle & eBorderStyle_close);
>+                }

I would have expected the close button to not normally be shown if the window
is not decorated.  i.e. I would have thought it would make more sense to call
set_deletable on windows for which set_decorated is not called with false.
It's probably safe to call it for windows with and without decoration though,
as this seems to be more a function hint than a decoration hint.
This was the best documentation that I found:
http://www.vaxination.ca/motif/VendorShell_3X.html

It also seems a bit strange to force deletable for eBorderStyle_default.

>+            if (mBorderStyle != eBorderStyle_default && (mBorderStyle & eBorderStyle_title)) {
>+                gdk_window_set_decorations(mShell->window, GDK_DECOR_TITLE);

Will the window manager actually decorate GTK_WINDOW_POPUP windows or is this
only intended for mNoAutoHide windows?

This will turn off all other decorations: resize handles, border, menu,
minimize, maximize.  Is that the intention?

>+                // popups should be resizable via their borders
>+//                gtk_window_set_resizable(GTK_WINDOW(mShell), PR_FALSE);

Assuming you mean, *not* resizable, that looks like the right thing to do.
It looks like that should continue to let gtk_window_resize have effect.
i.e. I don't know why that's not working.

Comment 41

[Neil Deakin]

> Are both these and GetNonClientMargin() required?

GetNonClientMargin() is only implemented on Windows. It also appears to return
the height of a standard window, not of the specific window called on, and would
be inaccurate for popups with thin titlebars. I don't know if that is intended
or not.


> Do you know what the "window origin" for GetClientOffset() is?
> Is that meant to be the top-left of the frame (screen) or client bounds?

Don't know. On GTK and Mac, GetClientOffset() always returns (0, 0). Perhaps a followup bug could implement these and we could combine the methods.


> The decorations for these noautohide titlebar popups will not look like they
> are active, but the browser window will become active when the user attempts
> to activate the popup.  Just checking that's the intention here.

It would be preferred if the titlebar on the child was active and the parent was active I think, or having the child active and the parent inactive but I'd rather investigate further in a followup bug.


> Where is IsPopupWithTitleBar defined?

In nsBaseWidget.h in patch part 5


> >-        gdk_window_get_root_origin(GTK_WIDGET(mContainer)->window,
> >-                                   &x, &y);
> >+        if (IsPopupWithTitleBar()) {
> >+            gdk_window_get_origin(GTK_WIDGET(mContainer)->window, &x, &y);
> >+        }
> >+        else {
> >+            gdk_window_get_root_origin(GTK_WIDGET(mContainer)->window, &x, &y);
> >+        }
> >         LOG(("WidgetToScreenOffset (container) %d %d\n", x, y));
> >     }

> Do you know why this returns (and has returned) the top-left of the frame
> (instead of the client top-left) for leading top-level windows?
> Why should popups be different?
> nsIWidget::WidgetToScreenOffset doc's say "widget's origin".
> Again, I don't know which origin that is.

Windows and Mac return the absolute screen coordinate of the client top-left (inside the titlebar).

I don't know why GTK is different but I din't want to break anything by changing it.


> >     // Toplevel windows need to have their bounds set so that we can
> >     // keep track of our location.  It's not often that the x,y is set
> >     // by the layout engine.  Width and height are set elsewhere.
> >+    nsIntPoint pnt(aEvent->x, aEvent->y);
> >     if (mIsTopLevel) {
> >         mPlaced = PR_TRUE;
> >         // Need to translate this into the right coordinates
> >         mBounds.MoveTo(WidgetToScreenOffset());
> >+        pnt = mBounds.TopLeft();
> >     }

> This code has me even more confused, the code without your changes.  I thought
> mBounds was the client rectangle top-left.  It's size set in OnSizeAllocate is
> the client size.
> You are changing this because you want the frame top-left?
> That seems OK, but mBounds.MoveTo(WidgetToScreenOffset()) disturbs me.

It disturbs me too.

I changed WidgetToScreenOffset() to return the client offset for popups with titlebars. So, mBounds gets set to the client rectangle which is what I expect to be correct. As above, I didn't want to change behaviour for non-popups. I can change the line above to not change 'pnt' for non-popups.


> Will the window manager actually decorate GTK_WINDOW_POPUP windows or is this
> only intended for mNoAutoHide windows?

I'll make sure this is only called for mNoAutoHide popups.


> This will turn off all other decorations: resize handles, border, menu,
> minimize, maximize.  Is that the intention?

Seems correct, although having the border visible would be acceptable.

Comment 42

[Neil Deakin]

(In reply to comment #26)
> Comment on attachment 455730 [details] [diff] [review]
> Part 9: support for titlebar close buttons
> 
> >+        widgetData.mBorderStyle =
> >+          static_cast<enum nsBorderStyle>(widgetData.mBorderStyle | eBorderStyle_close);
> Does
> widgetData.mBorderStyle |= eBorderStyle_close;
> not work?

It doesn't compile on Windows I think.


(In reply to comment #29)
> Comment on attachment 455723 [details] [diff] [review]
> Part 2: update the popup when it is moved or resized natively
> 
> In layout/xul/base/src/nsMenuPopupFrame.cpp
> >  nsMenuPopupFrame::MoveTo(PRInt32 aLeft, PRInt32 aTop, PRBool aUpdateAttrs)
> 
> Assert !IsAnchored() at the start of this method?

MoveTo can be called when the popup is anchored as well (popup.moveTo() via the popupBoxObject).

Comment 43

[Neil Deakin]

(In reply to comment #26)
> Comment on attachment 455730 [details] [diff] [review]
> Part 9: support for titlebar close buttons

> >+      if (mBorderStyle & eBorderStyle_close) {
> >+        style |= WS_SYSMENU;
> >+      }
> Or you could add the style in the eWindowType_popup case, since it will get
> automatically removed if the eBorderStyle_close style is not set.

It doesn't feel as readable, especially if someone wants to change this code and doesn't notice this case.

Comment 44

[Neil Deakin]

> If this returns a size, why isn't it returning an nsIntSize? Should we return
> the entire client rect, perhaps?

I filed bug 580301 on combining the two methods and improving them.

Comment 45

[Karl Tomlinson (:karlt)]

(In reply to comment #41)
> GetNonClientMargin() is only implemented on Windows. It also appears to return
> the height of a standard window, not of the specific window called on, and
> would
> be inaccurate for popups with thin titlebars. I don't know if that is intended
> or not.

It doesn't seem to ideal to me to have multiple APIs for the same info, where
different ones work in different situations, but I'll defer to the sr on this.

BTW, I suspect we may never want to try to get a synchronous answer when the
window is not yet visible on Linux.  (It requires waiting for the window
manager to reply, and even then we don't know how good that is.)

> > Do you know what the "window origin" for GetClientOffset() is?
> > Is that meant to be the top-left of the frame (screen) or client bounds?
> 
> Don't know. On GTK and Mac, GetClientOffset() always returns (0, 0). Perhaps a
> followup bug could implement these and we could combine the methods.

Thanks for filing that.

Comment 46

[Karl Tomlinson (:karlt)]

Comment on attachment 455737 [details] [diff] [review]
All GTK changes from the previous parts, for easy reviewing

(In reply to comment #41)
> > >-        gdk_window_get_root_origin(GTK_WIDGET(mContainer)->window,
> > >-                                   &x, &y);
> > >+        if (IsPopupWithTitleBar()) {
> > >+            gdk_window_get_origin(GTK_WIDGET(mContainer)->window, &x, &y);
> > >+        }
> > >+        else {
> > >+            gdk_window_get_root_origin(GTK_WIDGET(mContainer)->window, &x, &y);
> > >+        }
> > >         LOG(("WidgetToScreenOffset (container) %d %d\n", x, y));
> > >     }
> 
> > Do you know why this returns (and has returned) the top-left of the frame
> > (instead of the client top-left) for leading top-level windows?
> > Why should popups be different?
> > nsIWidget::WidgetToScreenOffset doc's say "widget's origin".
> > Again, I don't know which origin that is.
> 
> Windows and Mac return the absolute screen coordinate of the client top-left
> (inside the titlebar).
> 
> I don't know why GTK is different but I din't want to break anything by
> changing it.

I suspect that probably this was only there for GetScreenBounds, and we just
didn't notice it was wrong for other toplevel window callers because popups did
not have frames and for other toplevel windows most events are received on a
child window.

It looks like you've found the bug, and I'd prefer to have it fixed on all
toplevel windows rather than adding a special case.
(Blame me if it causes problems ;-))

> > >     // Toplevel windows need to have their bounds set so that we can
> > >     // keep track of our location.  It's not often that the x,y is set
> > >     // by the layout engine.  Width and height are set elsewhere.
> > >+    nsIntPoint pnt(aEvent->x, aEvent->y);
> > >     if (mIsTopLevel) {
> > >         mPlaced = PR_TRUE;
> > >         // Need to translate this into the right coordinates
> > >         mBounds.MoveTo(WidgetToScreenOffset());
> > >+        pnt = mBounds.TopLeft();
> > >     }

> I changed WidgetToScreenOffset() to return the client offset for popups with
> titlebars. So, mBounds gets set to the client rectangle which is what I expect
> to be correct.

OK, I see now that what this is doing is fetching root window coordinates for toplevel windows.
That looks good.

> I can change the line above to not change 'pnt' for non-popups.

NS_MOVE coordinates are not yet used (before patches in this bug), so no need
to worry about old behavior for that event.

>-    event.refPoint.x = aEvent->x;
>-    event.refPoint.y = aEvent->y;
>+    event.refPoint.x = pnt.x;
>+    event.refPoint.y = pnt.y;

event.refPoint = pnt;

I'd like to see the new patch.

Comment 47

[Neil Deakin]

Attachment: Part 5: support for titlebars on panels, version 2

I've removed WindowToClientOffset and merged it with GetClientOffset.

Comment 48

[Neil Deakin]

Attachment: Part 7: GTK widget changes, version 2

The change to always use gdk_window_get_origin instead of gdk_window_get_root_origin from within WidgetToScreenOffset causes some tests to fail:

layout/generic/test/test_plugin*
toolkit/content/tests/chrome/test_screenPersistence.xul
toolkit/content/tests/chrome/test_titlebar.xul

so I left it as is for now.

Comment 49

[Neil Deakin]

Attachment: Part 7: GTK widget changes, version 2, correct patch

See previous comment for description.

Comment 50

[Neil Deakin]

Attachment: Part 8: support titlebar labels, version 2

Comment 51

[Neil Deakin]

Attachment: Part 9: support for titlebar close buttons, version 2

Comment 52

[Neil Deakin]

Comment on attachment 459393 [details] [diff] [review]
Part 7: GTK widget changes, version 2, correct patch

bah, wrong patch again

Comment 53

[Neil Deakin]

Attachment: Part 7: GTK widget changes, version 2, this is really the right patch!

Comment 54

[Neil Deakin]

Attachment: Part 5: support for titlebars on panels, version 2.1

Found a minor issue that caused resizers in panels to not work. I will investigate a test for this.

Comment 55

[Karl Tomlinson (:karlt)]

Comment on attachment 459405 [details] [diff] [review]
Part 7: GTK widget changes, version 2, this is really the right patch!

>+            if (aInitData->mNoAutoHide &&
>+                mBorderStyle != eBorderStyle_default && (mBorderStyle & eBorderStyle_title)) {
>+                gdk_window_set_decorations(mShell->window, GDK_DECOR_TITLE);
>+            }

Would it make sense to use ConvertBorderStyles(mBorderStyle) here?

(In reply to comment #48)
> The change to always use gdk_window_get_origin instead of
> gdk_window_get_root_origin from within WidgetToScreenOffset causes some tests
> to fail:
> 
> layout/generic/test/test_plugin*
> toolkit/content/tests/chrome/test_screenPersistence.xul
> toolkit/content/tests/chrome/test_titlebar.xul

I tested this on the try server and didn't get failures with
http://hg.mozilla.org/try/rev/05a90220d1bd

(In a previous version, when I was returning the size correctly [from extents
not mBounds.width/height], test_screenPersistence.xul [correctly] observed
that the windows were the wrong size and started reporting the failure.  We
seem to have a bug on setting outer dimensions, but I'm happier leaving that
part to another bug, because its not the part you are fixing for popups here.)

Comment 56

[Neil Deakin]

(In reply to comment #55)

> Would it make sense to use ConvertBorderStyles(mBorderStyle) here?

It likely could, but ConvertBorderStyles prints an error when eBorderStyle_close is used, and also returns -1 when eBorderStyle_default is used which doesn't look like a valid value for gdk_window_set_decorations.


> > layout/generic/test/test_plugin*
> > toolkit/content/tests/chrome/test_screenPersistence.xul
> > toolkit/content/tests/chrome/test_titlebar.xul
> 
> I tested this on the try server and didn't get failures with
> http://hg.mozilla.org/try/rev/05a90220d1bd

I was actually referring to changing the WidgetToScreenOffset method as you describe in comment 46. If WidgetToScreenOffset is changed, those tests fail.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 459474 [details] [diff] [review]
Part 5: support for titlebars on panels, version 2.1

+     * @return the width and height of the offset.

/width and height/x and y/

Comment 58

[Karl Tomlinson (:karlt)]

Comment on attachment 459405 [details] [diff] [review]
Part 7: GTK widget changes, version 2, this is really the right patch!

> NS_IMETHODIMP
> nsWindow::GetScreenBounds(nsIntRect &aRect)
> {
>-    aRect = nsIntRect(WidgetToScreenOffset(), mBounds.Size());
>+    if (IsPopupWithTitleBar()) {
>+        // use the point including window decorations

"if (mIsTopLevel && mContainer)".
mContainer is null on destroyed windows and IsPopupWithTitleBar() makes its
look like non-popup-with-titlebar windows are different.

(In reply to comment #56)
> (In reply to comment #55)
> 
> > Would it make sense to use ConvertBorderStyles(mBorderStyle) here?
> 
> It likely could, but ConvertBorderStyles prints an error when
> eBorderStyle_close is used,

I'm OK with removing that debug message.

> and also returns -1 when eBorderStyle_default is
> used which doesn't look like a valid value for gdk_window_set_decorations.

Yes, -1 doesn't look like it will achieve the desired results, but if that bug
should be fixed for this situation it should be fixed for other situations
also.

However, I'm assuming that you only want to call gdk_window_set_decorations()
here when mBorderStyle != eBorderStyle_default.  I assume this is because
eBorderStyle_default means different things on different kinds of windows.
gtk_window_set_decorated(GTK_WINDOW(mShell), FALSE) is called above for
eBorderStyle_default.
 
> > I tested this on the try server and didn't get failures with
> > http://hg.mozilla.org/try/rev/05a90220d1bd
> 
> I was actually referring to changing the WidgetToScreenOffset method as you
> describe in comment 46. If WidgetToScreenOffset is changed, those tests fail.

That try changeset changes WidgetToScreenOffset as described in comment 46
(or at least as I was trying to describe).

Your patch correctly fixes WidgetToScreenOffset and GetScreenBounds for
IsPopupWithTitleBar().  I want that fixed for all toplevel windows, which is
simpler than special-casing IsPopupWithTitleBar().

Comment 59

[Karl Tomlinson (:karlt)]

> This still has the client size, so the bottom-right is actually inside the
> client area, but that's no change from the existing behaviour.

Filed bug 581863.

Comment 60

[Neil Deakin]

Attachment: Part 7: GTK widget changes, version 3

Comment 61

[Karl Tomlinson (:karlt)]

Comment on attachment 460268 [details] [diff] [review]
Part 7: GTK widget changes, version 3

> nsWindow::WidgetToScreenOffset()
> {
>     gint x = 0, y = 0;
> 
>     if (mContainer) {
>-        gdk_window_get_root_origin(GTK_WIDGET(mContainer)->window,
>-                                   &x, &y);
>+        gdk_window_get_origin(GTK_WIDGET(mContainer)->window, &x, &y);
>         LOG(("WidgetToScreenOffset (container) %d %d\n", x, y));
>     }
>     else if (mGdkWindow) {
>         gdk_window_get_origin(mGdkWindow, &x, &y);
>         LOG(("WidgetToScreenOffset (drawing) %d %d\n", x, y));
>     }

Thanks.  Just collapse this into one block with "if (mGdkWindow)".
mContainer->window is mGdkWindow.
(The logging is very frequent/verbose, so feel free to remove that too.)

Comment 62

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/ae8cc6e94bc1
http://hg.mozilla.org/mozilla-central/rev/ae1c9314a6a2
http://hg.mozilla.org/mozilla-central/rev/60fa04517a6f
http://hg.mozilla.org/mozilla-central/rev/7954db74158d
http://hg.mozilla.org/mozilla-central/rev/9df623bbccbc
http://hg.mozilla.org/mozilla-central/rev/057b802ec269
http://hg.mozilla.org/mozilla-central/rev/de7562be22b6
http://hg.mozilla.org/mozilla-central/rev/d8057f320d56
http://hg.mozilla.org/mozilla-central/rev/8524b753b454
http://hg.mozilla.org/mozilla-central/rev/6f69f62ec21e

Comment 63

[alexander :surkov (:asurkov)]

Marco, is everything fine on a11y side?

Comment 64

[Marco Zehe (:MarcoZ)]

Where can I see these in action?

Comment 65

[David Bolter [:davidb] (NeedInfo me for attention)]

Marco, control + shift + i brings it up. Right now accprobe shows it to be an alert... Catch me tomorrow on IRC and we can walk through it together with AT.

Comment 66

[Marco Zehe (:MarcoZ)]

Yeah that would be good. NVDA announces "alert, list", and nothing else. No keyboard focus goes anywhere and nothing else is spoken.

Comment 68

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

forked bug 591186

Comment 69

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336