594796 - Popups don't draw with accelerated layers on OS X

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Things like the awesomebar window have this problem.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Resize mPopupContentView when nsCocoaWindow changes size

We use the widget bounds to setup drawing for accelerated layers and without this patch they stay at 0x0

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Remove unrelated code from the patch

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 473578 [details] [diff] [review]
Remove unrelated code from the patch

Why are you doing this?  Is it to make the popup window's nsChildView
send a size event?  Then you should probably make the nsChildView
*just* send a size event, and not call nsChildView::Resize() on it.

Resizing a native Cocoa window (an NSWindow object) automatically
resizes its content view (which in a popup window is a native
ChildView object, corresponding to an nsChildView object).  So your
current patch resizes it twice.  Furthermore it resizes the ChildView
*before* the NSWindow gets resized, which might cause trouble
(e.g. artifacts).

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #3)
> Comment on attachment 473578 [details] [diff] [review]
> Remove unrelated code from the patch
> 
> Why are you doing this?  Is it to make the popup window's nsChildView
> send a size event?  Then you should probably make the nsChildView
> *just* send a size event, and not call nsChildView::Resize() on it.

Nope, I'm not doing this for the events. I'm doing this so that mBounds in mPopupContentView gets updated to match the size of the view. Is there a better way that I can do this?

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Attachment: Minimize possible resize conflicts

> I'm doing this so that mBounds in mPopupContentView gets updated to
> match the size of the view.

OK.  That makes sense, but makes things a little more complicated.

Does this patch do what you need?

It avoids resizing the ChildView before resizing the NSWindow of which
it's the content view.  And, though it doesn't eliminate the
redundancy, should end up "resizing" the ChildView to the same size.

Let me know your results.

Comment 6

[Neil Deakin]

Don't you also want to update the bounds on Move as well?

Comment 7

[Steven Michaud [:smichaud] (Retired)]

> Don't you also want to update the bounds on Move as well?

An NSView object that's a "content view" never gets moved inside its NSWindow.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #5)
> Created attachment 473685 [details] [diff] [review]
> Minimize possible resize conflicts
> 
> > I'm doing this so that mBounds in mPopupContentView gets updated to
> > match the size of the view.
> 
> OK.  That makes sense, but makes things a little more complicated.
> 
> Does this patch do what you need?

That patch does what I need, but like my patch it seems to break selection boxes. They contents draw offset from where they should be. Any ideas why?

Comment 9

[Steven Michaud [:smichaud] (Retired)]

> That patch does what I need, but like my patch it seems to break
> selection boxes. They contents draw offset from where they should
> be.

You mean selection boxes in popup windows?  Offset in which
direction(s)?

> Any ideas why?

Not really.

Comment 10

[Markus Stange [:mstange]]

Maybe we should just remove the mBounds field and just ask the NSView about its size every time we used to access mBounds.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

> Maybe we should just remove the mBounds field and just ask the
> NSView about its size every time we used to access mBounds.

Sounds reasonable to me.  But it's cross-platform (it's defined in
nsBaseWidget.h).  So it'll be some work to do this.

>> Any ideas why?
>
> Not really.

Actually, if they're offset down by 22 pixels, I *do* have a very good
idea why.  Revised patch coming up.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #9)
> > That patch does what I need, but like my patch it seems to break
> > selection boxes. They contents draw offset from where they should
> > be.
> 
> You mean selection boxes in popup windows?  Offset in which
> direction(s)?

The <select> form controls. (like the ones used in bugzilla)

They seem to be painted higher up than they should be. However, even the hit testing is shifted up.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix offset problem

Try this.  It should (I think) fix your offset problem -- whether it's
22 pixels down or 22 pixels up.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix dumb mistake

The previous patch had a dumb mistake, and wouldn't compile.

*This* one should work.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #14)
> Created attachment 473782 [details] [diff] [review]
> Fix dumb mistake
> 
> The previous patch had a dumb mistake, and wouldn't compile.
> 
> *This* one should work.

This crashes:

#0  0x91cf06a0 in objc_msgSend ()
#1  0x057789fd in nsCocoaWindow::Resize (this=0x223b5cb0, aX=162, aY=112, aWidth=922, aHeight=227, aRepaint=1) at ../../../../mozilla-central/widget/src/cocoa/nsCocoaWindow.mm:1164
#2  0x04d7872d in nsView::DoResetWidgetBounds (this=0x223c7af0, aMoveOnly=0, aInvalidateChangedSize=1) at ../../../mozilla-central/view/src/nsView.cpp:467
#3  0x04d7938c in nsView::NotifyEffectiveVisibilityChanged (this=0x223c7af0, aEffectivelyVisible=1) at ../../../mozilla-central/view/src/nsView.cpp:509
#4  0x04d7942f in nsView::SetVisibility (this=0x223c7af0, aVisibility=nsViewVisibility_kShow) at ../../../mozilla-central/view/src/nsView.cpp:529
#5  0x04d7d303 in nsViewManager::SetViewVisibility (this=0x2047a980, aView=0x223c7af0, aVisible=nsViewVisibility_kShow) at ../../../mozilla-central/view/src/nsViewManager.cpp:1406
#6  0x049fb2f8 in nsMenuPopupFrame::LayoutPopup (this=0x251ba410, aState=@0xbfffcec0, aParentMenu=0x0, aSizedToPopup=0) at ../../../../../mozilla-central/layout/xul/base/src/nsMenuPopupFrame.cpp:494
#7  0x04a0529a in nsPopupSetFrame::DoLayout (this=0xd43e10, aState=@0xbfffcec0) at ../../../../../mozilla-central/layout/xul/base/src/nsPopupSetFrame.cpp:158

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Calling the Resize method with x and y parameters set to 0 seems to solve the offset problem

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Attachment: Seemigly working version

Like this.

Comment 18

[Neil Deakin]

Comment on attachment 474965 [details] [diff] [review]
Seemigly working version

I tested this and it causes panels with titlebars (like those in the temporarily disabled Inspector) to not work. In a simple test, the titlebar simply wasn't visible, but the popup's height was large enough to include it.

<button label="Open Me" type="panel">
  <panel noautohide="true" titlebar="normal" label="Panel" close="true">
    <textbox/>
    <button label="Hello"/>
  </panel>
</button>

In another case, the popup kept resizing to take up the full height of the screen.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Attachment: Return the bounds of the mView in nsChildView::GetBounds()

Try a different approach. This seems to fix all the problems that I've seen.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 475133 [details] [diff] [review]
Return the bounds of the mView in nsChildView::GetBounds()

This looks fine to me.  I did some tests and didn't find any problems.

It doesn't fix the underlying problem of a popup window's ChildView's
mBounds not tracking it actual size.  But dealing with that would take
a while, and I understand that a fix for this bug is urgently needed.

(Ultimately we should probably get rid of mBounds altogether, as
Markus suggested in comment #10.)

Comment 21

[Jeff Muizelaar [:jrmuizel]]

The new patch seems to cause test failures:

3183 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_plugin_focus.html | Test timed out.
3255 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div1 aren't selected (div1-div2, all boxes are overflow: visible;): Selected String: ""
3256 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div2 aren't selected (div1-div2, all boxes are overflow: visible;): Selected String: ""
3267 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div1 aren't selected (div1-div3, all boxes are overflow: visible;): Selected String: ""
3268 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div2 aren't selected (div1-div3, all boxes are overflow: visible;): Selected String: ""
3269 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div3 aren't selected (div1-div3, all boxes are overflow: visible;): Selected String: ""
3279 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div1 aren't selected (div1-fixedDiv1, all boxes are overflow: visible;): Selected String: ""
3280 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div2 aren't selected (div1-fixedDiv1, all boxes are overflow: visible;): Selected String: ""
3281 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div3 aren't selected (div1-fixedDiv1, all boxes are overflow: visible;): Selected String: ""
3283 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of fixedDiv1 aren't selected (div1-fixedDiv1, all boxes are overflow: visible;): Selected String: ""
3291 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div1 aren't selected (div1-xbl_child, all boxes are overflow: visible;): Selected String: ""
3292 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div2 aren't selected (div1-xbl_child, all boxes are overflow: visible;): Selected String: ""
3293 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div3 aren't selected (div1-xbl_child, all boxes are overflow: visible;): Selected String: ""
3294 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of xbl_child aren't selected (div1-xbl_child, all boxes are overflow: visible;): Selected String: ""
3295 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of fixedDiv1 aren't selected (div1-xbl_child, all boxes are overflow: visible;): Selected String: ""
3303 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div1 aren't selected (div1-fixedDiv2, all boxes are overflow: visible;): Selected String: ""
3304 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div2 aren't selected (div1-fixedDiv2, all boxes are overflow: visible;): Selected String: ""
3305 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of div3 aren't selected (div1-fixedDiv2, all boxes are overflow: visible;): Selected String: ""
3306 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of xbl_child aren't selected (div1-fixedDiv2, all boxes are overflow: visible;): Selected String: ""
3307 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of fixedDiv1 aren't selected (div1-fixedDiv2, all boxes are overflow: visible;): Selected String: ""
3308 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of fixedDiv2 aren't selected (div1-fixedDiv2, all boxes are overflow: visible;): Selected String: ""

Comment 22

[Steven Michaud [:smichaud] (Retired)]

Attachment: Use 'frame' instead of 'bounds'

I shouldn't have r+ed the previous patch -- we should be using [NSView
frame] instead of [NSView bounds], since we use [NSView setFrame:]
instead of [NSView setBounds:].  (An NSView's "frame" is its
dimensions in the coordinate system of its superview.  It's "bounds"
are its dimensions in its own coordinate system.)

This patch fixes the problem.  It works fine in my (brief) tests.  It
also (locally) passes the tests the previous patch failed, and all the
layout mochitests.  Jeff, please test this yourself, and run it
through all the automated tests on your own group's testbed.

Comment 23

[Joe Drew (not getting mail)]

Comment on attachment 475237 [details] [diff] [review]
Use 'frame' instead of 'bounds'

Check it in :)

Comment 24

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/8745a2a64e23