If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[OS X] [OpenGL] Window resizer not drawn

VERIFIED FIXED in mozilla2.0b9

Status

()

Core
Graphics
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

({regression})

Trunk
mozilla2.0b9
All
Mac OS X
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta9+)

Details

(Whiteboard: [tb33needed][hardblocker])

Attachments

(5 attachments, 8 obsolete attachments)

34.16 KB, image/png
faaborg
: ui-review-
Details
22.16 KB, image/png
faaborg
: ui-review+
Details
80.61 KB, image/png
faaborg
: ui-review+
Details
2.25 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
10.61 KB, patch
Joe Drew (not getting mail)
: review+
Josh Aas
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
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.
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...
(Assignee)

Comment 2

7 years ago
Right you are.
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.
Duplicate of this bug: 602740
confirmed in xulrunner-based apps
This is different from bug 489303, right?
blocking2.0: --- → beta8+
Markus, ideas?
Assignee: nobody → mstange
(Assignee)

Comment 8

7 years ago
(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.
Assignee: mstange → nobody

Comment 9

7 years ago
Scott, can you look into this?
Assignee: nobody → sgreenlay

Updated

7 years ago
Duplicate of this bug: 606916

Updated

7 years ago
Duplicate of this bug: 602007
Whiteboard: [tbtrunkneeds]

Updated

7 years ago
Assignee: sgreenlay → joe
Created attachment 485833 [details] [diff] [review]
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.
Yeah, we'll be calling DrawOver on the nsChildView child of nsCocoaWindow. So, override DrawOver in nsChildView instead, or have it forward to nsCocoaWindow.
Duplicate of this bug: 608260
Keywords: regression

Updated

7 years ago
blocking2.0: beta8+ → betaN+

Comment 15

7 years ago
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.
Whiteboard: [tbtrunkneeds] → [tb33needs]
Created attachment 492439 [details] [diff] [review]
WIP Mac Resize Handle Patch
Attachment #485833 - Attachment is obsolete: true
The above patch tries to draw a red square for the resize handle (but fails).
Created attachment 496052 [details] [diff] [review]
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.
Attachment #496052 - Flags: review?(joe)
Created attachment 496053 [details] [diff] [review]
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.
(Assignee)

Comment 20

7 years ago
Created attachment 496128 [details] [diff] [review]
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.
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.
Depends on: 604101
Created attachment 497831 [details] [diff] [review]
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.
Attachment #492439 - Attachment is obsolete: true
Attachment #496052 - Attachment is obsolete: true
Attachment #496053 - Attachment is obsolete: true
Attachment #496128 - Attachment is obsolete: true
Attachment #497831 - Flags: review?(joe)
Attachment #496052 - Flags: review?(joe)
(Assignee)

Comment 23

7 years ago
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.
Created attachment 498067 [details]
Resizer, no scroll bars

Let's get UI feedback for the resizer-with-box.
Attachment #498067 - Flags: ui-review?(faaborg)
Created attachment 498068 [details]
Resizer, vertical scroll bar
Attachment #498068 - Flags: ui-review?(faaborg)
Created attachment 498069 [details]
Resizer, both scroll bars
Attachment #498069 - Flags: ui-review?(faaborg)
(Assignee)

Comment 27

7 years ago
Created attachment 498341 [details] [diff] [review]
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.
Attachment #498341 - Flags: review?(joe)
(Assignee)

Comment 28

7 years ago
Created attachment 498342 [details] [diff] [review]
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.
Attachment #498342 - Flags: review?(joe)
>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.
Attachment #498069 - Flags: ui-review?(faaborg) → ui-review+
Attachment #498068 - Flags: ui-review?(faaborg) → ui-review+
Attachment #498067 - Flags: ui-review?(faaborg) → ui-review-
(Assignee)

Updated

7 years ago
Attachment #497831 - Attachment is obsolete: true
Attachment #497831 - Flags: review?(joe)
(Assignee)

Comment 30

7 years ago
Created attachment 500837 [details] [diff] [review]
without box, v2

Attachment 498341 [details] [diff] was missing a static_cast<nsIWidget_MOZILLA_2_0_BRANCH*>. This one actually compiles.
Attachment #498341 - Attachment is obsolete: true
Attachment #500837 - Flags: review?(joe)
Attachment #498341 - Flags: review?(joe)
Whiteboard: [tb33needs] → [tb33needs][hard blocker]
Whiteboard: [tb33needs][hard blocker] → [tb33needs][hardblocker]
Attachment #498342 - Flags: review?(joe) → review+
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.
Attachment #500837 - Flags: review?(joe) → review-

Comment 32

7 years ago
Created attachment 501977 [details] [diff] [review]
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?
Attachment #500837 - Attachment is obsolete: true
Attachment #501977 - Flags: review?(joe)
(Assignee)

Comment 33

7 years ago
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 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.)
Attachment #501977 - Flags: review?(joe) → review+
blocking2.0: betaN+ → beta9+

Updated

7 years ago
Assignee: joe → mstange
blocking2.0: beta9+ → betaN+

Updated

7 years ago
Attachment #501977 - Flags: review+
blocking2.0: betaN+ → beta9+

Comment 35

7 years ago
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/8d005c3a6685
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre ID:20110109030350
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla2.0b9
Confirming all windows and dialogs of BlueGriffon who have a resizer now draw
it correctly again. Thanks a lot guys.
Depends on: 625827
(Assignee)

Updated

7 years ago
Depends on: 627656
Whiteboard: [tb33needs][hardblocker] → [tb33needed][hardblocker]

Updated

5 years ago
Depends on: 641357
(Assignee)

Updated

5 years ago
No longer depends on: 641357
You need to log in before you can comment on or make changes to this bug.