595180 - [OS X] [OpenGL] Window resizer not drawn

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Markus Stange [:mstange]]

The window resizer in the bottom right corner is missing when using the OpenGL layers backend. NSWindow used to draw it on top of everything after our NSViews were finished rendering, but apparently you can't draw anything on top of OpenGL-enabled NSViews.

The only simple fix I can think of at the moment is adding this to xul.css:
:root::after {
  content: '';
  position: fixed;
  bottom: 0;
  right: 0;
  -moz-appearance: resizer;
  z-index: some large value;
}

... but I don't like it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

That wouldn't do the right thing anyway, would it?  It'd put that in for non-toplevel XUL documents, for example.  And nonresizable XUL documents, etc...

Comment 2

[Markus Stange [:mstange]]

Right you are.

Comment 3

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

Can't we just draw the resizer in nsChildView.mm (perhaps via a callback from LayerManagerOGL when after compositing the layers)?

Maybe that's the way to fix bug 595156 as well ... add nsIWidget::DrawUnder and nsIWidget::DrawOver callbacks that the layer managers use when drawing toplevel windows? I guess for non-BasicLayers we also need to get the size and position of the area drawn so we can create temporary buffers.

We could also hook something up from layout, having FrameLayerBuilder create a couple of extra layers.

Comment 5

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

confirmed in xulrunner-based apps

Comment 6

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

This is different from bug 489303, right?

Comment 7

[Joe Drew (not getting mail)]

Markus, ideas?

Comment 8

[Markus Stange [:mstange]]

(In reply to comment #6)
> This is different from bug 489303, right?

Right. Bug 489303 will only restore the resizer on the main browser window, but on Mac we need it on all resizable windows. And bug 489303 won't have any effect on Mac, otherwise its main window resizer would conflict with the all-window one we're trying to restore in this bug. :)

(In reply to comment #7)
> Markus, ideas?

Roc's plan sounds good to me, but I don't have time to implement it at the moment. Somebody else will have to do it.
I don't have any opinion on whether the additional layer should be managed by the layer manager or by FrameLayerBuilder.

Comment 9

[Josh Aas]

Scott, can you look into this?

Comment 12

[Scott Greenlay [:sgreenlay]]

Attachment: WIP Mac Resize Handle Patch

This patch adds nsIWidget::DrawOver and DrawUnder, and has an override in nsCocoaWindow that can be used to draw the resize handle (the drawing code in there right now is temporary and probably wrong).

The problem is that nsCocoaWindow::DrawOver is not called because mWidget in LayerManagerOGL::Render is an nsIWidget found through GetNearestWidget() and not the nsCocoaWindow.

Comment 13

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

Yeah, we'll be calling DrawOver on the nsChildView child of nsCocoaWindow. So, override DrawOver in nsChildView instead, or have it forward to nsCocoaWindow.

Comment 15

[Benjamin Smedberg]

Joe asked me to state that I don't think this blocks a time-based beta 8. We'd certainly like to get it in, but it isn't a regression since beta7 and we're trying to stick to schedules for these betas, not features.

Comment 16

[Scott Greenlay [:sgreenlay]]

Attachment: WIP Mac Resize Handle Patch

Comment 17

[Scott Greenlay [:sgreenlay]]

The above patch tries to draw a red square for the resize handle (but fails).

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Add UploadSurfaceToTexture

Add an interface to easily upload a gfxASurface to a texture. 

I've tried to keep this as generic as possible so it can be used in CairoImageOGL and CanvasLayerOGL (might need a few more changes) to take advantage of the GetAsImageSurface optimization. I'll file a separate bug for that if this scrapes it's way through review.

Comment 19

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP drawing of the resize handle

This uses UploadSurfaceToTexture and our normal rendering code to draw the resize handle.

Currently only draws a green rectangle, the cairo drawing code needs to be actually written.

We definitely don't need to be repeating this every frame, we can hang on to the texture and just re-use it.

Comment 20

[Markus Stange [:mstange]]

Attachment: WIP native resizer drawing

Here's a patch on top of your patches that uses a private method to draw the native resizer. Instead of using private stuff it would probably be better to draw the resizer manually, though.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Marking 604101 as blocking this bug as it should resolve the issue of uploading surfaces to textures efficiently. We may not need the UploadSurfaceToTexture patch depending on how this is resolved.

Comment 22

[Matt Woodrow (:mattwoodrow)]

Attachment: Implement DrawOver for cocoa window

This uses the native cocoa theme drawing code to draw a window resizer.

It does however draw an outlined square as well as the diagonal lines, not sure if this is an issue.

Comment 23

[Markus Stange [:mstange]]

The square was the reason why I didn't go for that approach, but I guess we can still change that later.

Should we really add DrawUnder? I can't foresee a use for it.

The new nsIWidget methods need to be added to the nsIWidget_MOZILLA_2_0_BRANCH interface instead.

Comment 24

[Joe Drew (not getting mail)]

Attachment: Resizer, no scroll bars

Let's get UI feedback for the resizer-with-box.

Comment 25

[Joe Drew (not getting mail)]

Attachment: Resizer, vertical scroll bar

Comment 26

[Joe Drew (not getting mail)]

Attachment: Resizer, both scroll bars

Comment 27

[Markus Stange [:mstange]]

Attachment: without box

Here's a patch that draws the resizer using CGContextStrokeLineSegments. I've also gotten rid of DrawUnder, moved DrawOver to the nsIWidget_MOZILLA_2_0_BRANCH interface and made it compile in non-libxul builds.

Comment 28

[Markus Stange [:mstange]]

Attachment: only assert current program in debug libxul builds

There's another change that's necessary to make it compile in non-libxul builds. Widget doesn't pick up the definition of LayerManagerOGLProgram::sCurrentProgramKey in gfx/layers/opengl/LayerManagerOGL.cpp, so it can't call some methods of LayerManagerOGLProgram which use ASSERT_THIS_PROGRAM.

This patch just disables the assert in non-libxul builds.

Comment 29

[Alex Faaborg [:faaborg] (Firefox UX)]

>It does however draw an outlined square as well as the diagonal lines, not sure
>if this is an issue.

Yeah, the outline on the square draws too much attention to the re sizer (and isn't native in the sense that Safari doesn't do this).  Let's just use a pixel perfect image so that we can have direct control.

Comment 30

[Markus Stange [:mstange]]

Attachment: without box, v2

Attachment 498341 [details] [diff] was missing a static_cast<nsIWidget_MOZILLA_2_0_BRANCH*>. This one actually compiles.

Comment 31

[Joe Drew (not getting mail)]

Comment on attachment 500837 [details] [diff] [review]
without box, v2

Won't this draw on all windows, not just resizable ones?

Also, I think we should create one texture per window, and reuse it.

Comment 32

[Josh Aas]

Attachment: draw resizer, v3

This fixes drawing the resizer for non-resizable windows. Nice catch Joe. Hope I'm not stepping on your toes jumping in here Markus.

I'm not sure what the best way to save off one texture per window is, I'll leave that for Markus. Why wouldn't we just save off one texture for all windows, btw?

Can we take this patch for now (so it'll make beta 9) and implement saving the texture in another bug later?

Comment 33

[Markus Stange [:mstange]]

Thanks, Josh!

Joe, would the nsCocoaWindow destructor be the only place where we'd have to delete the retained texture, or is it possible that the layer manager goes away before the widget is destroyed?

Comment 34

[Joe Drew (not getting mail)]

Comment on attachment 501977 [details] [diff] [review]
draw resizer, v3


>diff --git a/widget/src/cocoa/nsChildView.mm b/widget/src/cocoa/nsChildView.mm

>+void
>+nsChildView::DrawOver(LayerManager* aManager, nsIntRect aRect) {

Put that { on a new line.

Looks good; please file a blocker followup to only allocate one texture per window.

(We need one texture per window because we have GL context per window; we might be able to share, but it's a bit tricksy.)

Comment 35

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/8d005c3a6685

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre ID:20110109030350

Comment 37

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

Confirming all windows and dialogs of BlueGriffon who have a resizer now draw
it correctly again. Thanks a lot guys.