588657 - Console and Inspector panel chrome flashes on windows 7 and linux

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[David Dahl :ddahl]

The panel's chrome "flashes" when dragging and when working with the Tree inside.

This appears to be a problem on Linux and Windows only.

Comment 1

[Mathieu Pellerin]

I can also see this on my windows 7 machine. The panels' border go back and forth between aero to plain style.

Comment 2

[Benjamin Smedberg]

Yeah, I see the normal window borders flashing on and off.

Comment 4

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

I did a bit of testing with this last week.

I confirmed that this isn't a result of d2d fallout by disabling mozilla.widget.render-mode to 0 and restarted. Flickering still occurred.

Also tested a build with the patch from bug 554982 backed out. Flickering was still apparent.

Anybody have any other ideas?

Comment 5

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

I

Comment 7

[Ben Tremblay]

Mousing around in white-space induces a frenzy.
After having selected an element? No frenzy.

Comment 8

[Mike Shaver (:shaver emeritus)]

If I have selected an element, I still get the frenzy if I mouse over a link (tested on this very bug page).

Comment 9

[Mike Shaver (:shaver emeritus)]

We can't miss b6 with this and expect to get meaningful feedback from Windows users: all we'll get is "inspector looks ridiculous, I stopped using it".

Comment 10

[Steve Glardon (Glards)]

Attachment: Correct the panel chrome flash bug

I saw this bug while trying to test the inspector. So I gave it a try and after a debugging session I came up with the two following problems:

1) The flash of the glass border on the panel is triggered by the following code snippets (from widget/src/windows/nsWindow.cpp):
 ClearThemeRegion();
 VERIFY(::SetWindowPos(mWnd, NULL, aX, aY, 0, 0, SWP_NOZORDER | SWP_NOACTIVATE | SWP_NOSIZE));
 SetThemeRegion();

The code is used by the methods nsWindow::Move and nsWindow::Resize. The ClearThemeRegion method calls SetWindowRgn and this has the effect to show the glass border on the panel. The glass border goes away with the call to the method SetThemeRegion. This has been introduced by bug 376408. The change I made is that we don't clear the region for panel.

2) While having a breakpoint in the nsWindow::Move method, I saw an abnormal number of call to this method on the panel window. I could track that down to the nsView::DoResetWidgetBounds (in view\src\nsView.cpp). There is a check between the current bounds and the widget bounds to see if it is needed to resize or move the window. The current bounds is retrieved by the nsWindow::GetBounds methods. This method returns an offset of the parent's screen origin for window with parent (which is the case of the panel). And this is never the same as the widget bounds. It causes the code to call move/resize every time there is a pending update of the window. That was really slowing down the browser for me every time there was something to repaint while the inspector window was open. So for popup window, we should use the GetScreenBounds instead of the GetBounds method.

This patch fixes the issue for me. Maybe there are better ways to fix the issues and I'm up to any suggestion.

Comment 11

[David Dahl :ddahl]

Cool. If I want to test this patch, where do I need to build? make -C view && ?

Comment 12

[Jim Mathies [:jimm]]

(In reply to comment #10)
> Created attachment 471317 [details] [diff] [review]
> Correct the panel chrome flash bug
> 
> I saw this bug while trying to test the inspector. So I gave it a try and after
> a debugging session I came up with the two following problems:
> 
> 1) The flash of the glass border on the panel is triggered by the following
> code snippets (from widget/src/windows/nsWindow.cpp):
>  ClearThemeRegion();
>  VERIFY(::SetWindowPos(mWnd, NULL, aX, aY, 0, 0, SWP_NOZORDER | SWP_NOACTIVATE
> | SWP_NOSIZE));
>  SetThemeRegion();
> 
> The code is used by the methods nsWindow::Move and nsWindow::Resize. The
> ClearThemeRegion method calls SetWindowRgn and this has the effect to show the
> glass border on the panel. The glass border goes away with the call to the
> method SetThemeRegion. This has been introduced by bug 376408. The change I
> made is that we don't clear the region for panel.
> 
> 2) While having a breakpoint in the nsWindow::Move method, I saw an abnormal
> number of call to this method on the panel window. I could track that down to
> the nsView::DoResetWidgetBounds (in view\src\nsView.cpp). There is a check
> between the current bounds and the widget bounds to see if it is needed to
> resize or move the window. The current bounds is retrieved by the
> nsWindow::GetBounds methods. This method returns an offset of the parent's
> screen origin for window with parent (which is the case of the panel). And this
> is never the same as the widget bounds. It causes the code to call move/resize
> every time there is a pending update of the window. That was really slowing
> down the browser for me every time there was something to repaint while the
> inspector window was open. So for popup window, we should use the
> GetScreenBounds instead of the GetBounds method.
> 
> This patch fixes the issue for me. Maybe there are better ways to fix the
> issues and I'm up to any suggestion.

Do the corner of popup panels still look correct with this applied? For example, the "Edit this bookmarks" popup on the gold star in the address bar?

Comment 13

[Steve Glardon (Glards)]

(In reply to comment #11)
> Cool. If I want to test this patch, where do I need to build? make -C view && ?

I always do the full build thing (make -f client.mk).

Comment 14

[Steve Glardon (Glards)]

Attachment: Screenshot of the "Edit this bookmarks" popup

(In reply to comment #12)
> Do the corner of popup panels still look correct with this applied? For
> example, the "Edit this bookmarks" popup on the gold star in the address bar?

Here is a screenshot of the popup with the patch applied. It looks correct to me.

There is a small artifact on the top right side but I checked the nightly build and it is also there.

Comment 15

[Jim Mathies [:jimm]]

(In reply to comment #14)
> Created attachment 471333 [details]
> Screenshot of the "Edit this bookmarks" popup
> 
> (In reply to comment #12)
> > Do the corner of popup panels still look correct with this applied? For
> > example, the "Edit this bookmarks" popup on the gold star in the address bar?
> 
> Here is a screenshot of the popup with the patch applied. It looks correct to
> me.
> 
> There is a small artifact on the top right side but I checked the nightly build
> and it is also there.

This patch is a little odd. You only apply the changes to ClearThemeRegion. So the theme region is still being set on SetThemeRegion, or the call there is failing. Is there any way we can detect these windows down in nsWindow and clear and set theme regions on windows that want them? At the very least, your changes should be matched up between the two calls, but I suspect that will reintroduce bug 485484.

Comment 16

[Jim Mathies [:jimm]]

(In reply to comment #15)
> (In reply to comment #14)
> > Created attachment 471333 [details] [details]
> > Screenshot of the "Edit this bookmarks" popup
> > 
> > (In reply to comment #12)
> > > Do the corner of popup panels still look correct with this applied? For
> > > example, the "Edit this bookmarks" popup on the gold star in the address bar?
> > 
> > Here is a screenshot of the popup with the patch applied. It looks correct to
> > me.
> > 
> > There is a small artifact on the top right side but I checked the nightly build
> > and it is also there.
> 
> This patch is a little odd. You only apply the changes to ClearThemeRegion. So
> the theme region is still being set on SetThemeRegion, or the call there is
> failing. Is there any way we can detect these windows down in nsWindow and
> clear and set theme regions on windows that want them? At the very least, your
> changes should be matched up between the two calls, but I suspect that will
> reintroduce bug 485484.

bug 485484 -> bug 376408

Comment 17

[Steve Glardon (Glards)]

(In reply to comment #15)
> This patch is a little odd. You only apply the changes to ClearThemeRegion. So
> the theme region is still being set on SetThemeRegion, or the call there is
> failing. Is there any way we can detect these windows down in nsWindow and
> clear and set theme regions on windows that want them? At the very least, your
> changes should be matched up between the two calls, but I suspect that will
> reintroduce bug 485484.

Well the call that poses problems is the SetWindowRgn(NULL) as it redraws the window with the glass border (even with the bRedraw parameter set to FALSE).

So I changed the code to not call that for popup.

Do we really have to call SetWindowRgn(NULL) before calling SetWindowPos ? The call to SetWindowRgn(...) after SetWindowPos doesn't suffice ?

Comment 18

[Neil Deakin]

> 2) While having a breakpoint in the nsWindow::Move method, I saw an abnormal
> number of call to this method on the panel window. I could track that down to
> the nsView::DoResetWidgetBounds (in view\src\nsView.cpp). There is a check
> between the current bounds and the widget bounds to see if it is needed to
> resize or move the window. The current bounds is retrieved by the
> nsWindow::GetBounds methods. This method returns an offset of the parent's
> screen origin for window with parent (which is the case of the panel). And this
> is never the same as the widget bounds. It causes the code to call move/resize
> every time there is a pending update of the window. That was really slowing
> down the browser for me every time there was something to repaint while the
> inspector window was open. So for popup window, we should use the
> GetScreenBounds instead of the GetBounds method.

I tested this part of the patch and it seems to improve calls on Windows, and I didn't notice any issues with it, but I didn't test extensively.

Steve, did you test this on other platforms besides Windows?

The bounds handling is quite confusing and is a bit different on each platform. The titlebar bug 552982 improved things, but I still think there's some inconsistencies, especially on Linux. It might be better to do this second part of the patch in a different bug.

Comment 19

[Jim Mathies [:jimm]]

Attachment: widget patch

Comment 20

[Jim Mathies [:jimm]]

Attachment: screenshots w/patch

The widget patch I posted keeps theme regions applied to other popups, while excluding popups that use the caption style (via Neil's IsPopupWithTitleBar()).

Attached are screen shots of the windows that are created, is this the desired effect?

Note this patch alone addresses the flashing problem. Steve, feel free to take this and integrate it in with your other changes.

Comment 21

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

this tested fine in OS X. Still waiting on a windows build to finish.

Comment 22

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

In Window 7, the inspector panels now have glass titlebars and borders, which is an improvement. They also don't appear to flicker when mousing over other widgets in the browser.

The one strange thing that I noticed is some odd tearing when scrolling the webpage with the inspector highlighter locked over a node. This is possibly unrelated to this bug. I'll have to test without this patch installed.

As it is, this is an improvement.

Comment 23

[Steve Glardon (Glards)]

Well I guess we can submit Jim's patch for review to fix the flashing problem.

I created bug 593625 for the second part of the modifications I made since there is some problems with Linux with that change.

Comment 24

[Jim Mathies [:jimm]]

(In reply to comment #22)
> The one strange thing that I noticed is some odd tearing when scrolling the
> webpage with the inspector highlighter locked over a node. This is possibly
> unrelated to this bug. I'll have to test without this patch installed.

I filed something similar to this in bug 593075.

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 471551 [details] [diff] [review]
widget patch

sure, more conditions never hurt anything!

Comment 26

[Jim Mathies [:jimm]]

(In reply to comment #25)
> Comment on attachment 471551 [details] [diff] [review]
> widget patch
> 
> sure, more conditions never hurt anything!

We should probably file a follow up to diagnose why Resize or Move are being triggered when the mouse is dragged. This doesn't happen except when the inspector is up. I imagine it has something to do with the window highlighting. (If that's not already covered by bug 593625.)

Comment 27

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/6726c330ce8e