Closed Bug 552982 Opened 10 years ago Closed 10 years ago

titlebars for panel

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta3+

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Blocks 4 open bugs, )

Details

Attachments

(14 files, 13 obsolete files)

483.78 KB, image/png
Details
67.12 KB, patch
Details | Diff | Splinter Review
96.98 KB, patch
Details | Diff | Splinter Review
5.75 KB, patch
mats
: review+
Details | Diff | Splinter Review
13.29 KB, patch
mats
: review+
Details | Diff | Splinter Review
26.42 KB, patch
mats
: review+
Details | Diff | Splinter Review
6.40 KB, patch
mats
: review+
Details | Diff | Splinter Review
5.82 KB, patch
Details | Diff | Splinter Review
5.84 KB, patch
jimm
: review+
Details | Diff | Splinter Review
15.64 KB, patch
jaas
: review+
Details | Diff | Splinter Review
3.87 KB, patch
neil
: review+
Details | Diff | Splinter Review
7.53 KB, patch
neil
: review+
Details | Diff | Splinter Review
28.62 KB, patch
Details | Diff | Splinter Review
8.77 KB, patch
karlt
: review+
Details | Diff | Splinter Review
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.
Attached patch basic patch (obsolete) — Splinter Review
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Blocks: 554926
Blocks: 554928
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?
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!
Attached patch updated work-in-progress patch (obsolete) — Splinter Review
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.
Attachment #433141 - Attachment is obsolete: true
Attached patch work in progress 2 (obsolete) — Splinter Review
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
Attached patch work in progress 3 (obsolete) — Splinter Review
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
Attachment #447761 - Attachment is obsolete: true
Also, panel titles are now supported using <panel label="Tools">
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!
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.
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
Blocks: 554925
Blocks: 554919
(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.
Attachment #436063 - Attachment is obsolete: true
This patch implements this feature and includes all of the following patches
Attachment #453460 - Attachment is obsolete: true
Supports three levels on noautohide panels: top, floating and parent
This is done via <panel noautohide="true" titlebar="true">
Attached patch Part 7: Hacky GTK widget changes (obsolete) — Splinter Review
Attached patch Part 8: support titlebar labels (obsolete) — Splinter Review
Attachment #455737 - Flags: review? → review?(karlt)
Attachment #455729 - Flags: review?(neil)
Attachment #455730 - Flags: review?(neil)
Attachment #455722 - Flags: review?(matspal)
Attachment #455723 - Flags: review?(matspal)
Attachment #455724 - Flags: review?(matspal)
Attachment #455725 - Flags: review?(matspal)
Attachment #455726 - Flags: superreview?(roc)
Attachment #455726 - Flags: review?(matspal)
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 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.
Duplicate of this bug: 554925
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.
Attachment #455736 - Flags: review?(joshmoz) → review+
Blocks: 577377
No longer blocks: 554919
Attachment #455722 - Flags: review?(matspal) → review+
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
Attachment #455723 - Flags: review?(matspal) → review+
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?)
Attachment #455724 - Attachment description: Part 3: support panel levels → Part 3: support panel levels
Attachment #455724 - Flags: review?(matspal) → review+
Attachment #455725 - Flags: review?(matspal) → review+
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.
Attachment #455726 - Flags: review?(matspal) → review+
This is awesome. Thanks for the reviews, mats!

I would love to see this landed for b2 on all platforms.
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.
Attachment #455734 - Flags: review?(jmathies) → review+
Blocks: 473918
Blocks: 509662
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.
blocking2.0: --- → ?
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.
blocking2.0: ? → beta3+
+     * 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?
(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.
OK, I'd like to see the revised patch.
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 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.
> 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.
(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).
(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.
Blocks: 580301
> 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.
(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 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.
Attachment #455737 - Flags: review?(karlt) → review-
I've removed WindowToClientOffset and merged it with GetClientOffset.
Attachment #455726 - Attachment is obsolete: true
Attachment #459389 - Flags: superreview?(roc)
Attachment #455726 - Flags: superreview?(roc)
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.
Attachment #455728 - Attachment is obsolete: true
Attachment #459390 - Flags: review?
See previous comment for description.
Attachment #459390 - Attachment is obsolete: true
Attachment #459393 - Flags: review?(karlt)
Attachment #459390 - Flags: review?
Attachment #455729 - Attachment is obsolete: true
Attachment #459394 - Flags: review?(neil)
Attachment #455729 - Flags: review?(neil)
Attachment #455730 - Attachment is obsolete: true
Attachment #455737 - Attachment is obsolete: true
Attachment #459395 - Flags: review?(neil)
Attachment #455730 - Flags: review?(neil)
Comment on attachment 459393 [details] [diff] [review]
Part 7: GTK widget changes, version 2, correct patch

bah, wrong patch again
Attachment #459393 - Attachment is obsolete: true
Attachment #459393 - Flags: review?(karlt)
Attachment #459395 - Flags: review?(neil) → review+
Found a minor issue that caused resizers in panels to not work. I will investigate a test for this.
Attachment #459389 - Attachment is obsolete: true
Attachment #459474 - Flags: superreview?(roc)
Attachment #459389 - Flags: superreview?(roc)
Attachment #459405 - Attachment is patch: true
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.)
(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 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/
Attachment #459474 - Flags: superreview?(roc) → superreview+
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().
Attachment #459405 - Flags: review?(karlt) → review-
> 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.
Attachment #459394 - Flags: review?(neil) → review+
Attachment #459405 - Attachment is obsolete: true
Attachment #460268 - Flags: review?(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.)
Attachment #460268 - Flags: review?(karlt) → review+
Marco, is everything fine on a11y side?
Where can I see these in action?
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.
Yeah that would be good. NVDA announces "alert, list", and nothing else. No keyboard focus goes anywhere and nothing else is spoken.
Depends on: 584344
Duplicate of this bug: 93181
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.