34466 - invisible views are painted

Status: RESOLVED FIXED

(Core :: XUL, defect, P4)

Description

[Tomi Leppikangas]

When selecting text with paintflash on, area where Edit->Copy menuitem
should be, flashes. This happend even that menuitem is not shown
on screen (menu is not drop down).

Things to reproduce:

First open Edit menu and close it. Then put paint flashing on and select some
text from mozilla. You see that selection flashes, and then flashes area below
edit menu, just where 'Copy' menuitem should be. 

This same happens for 'Go' menu items 'Forward' and 'Back' when clickin on
back/forward buttons, but that is bit harder to see.

I have small patch that fixes this, but i am not sure is this checking
for visibility at right place?

Comment 1

[Tomi Leppikangas]

Attachment: small fix to nsViewManager2::ProcessPendingUpdates

Comment 2

[Simon Fraser [no longer active]]

Adding perf keyword

Comment 3

[Kevin McCluskey (gone)]

Great job tracking that down, tomi :-), but I think the fix needs to be placed 
in the IsRectVisible. This will prevent the invisible views from being put in 
the queue to be processed altogether. Can you try adding the following lines to 
the top of IsRectVisible?

 aView->GetVisibility(visibility);
 if(nsViewVisibility_kHide == visibility) {
   return PR_FALSE;
 }

If not, I'll give it a try after I've integrated some other view manager changes 
and see if it clears up the problem.
Thanks.

Comment 4

[Tomi Leppikangas]

Yes it works ok. Actually there is that test in nsViewManager2::UpdateView
but it is after we add region to dirty region. Maybe moving that test
to begining of function does the job?

Comment 5

[Tomi Leppikangas]

I found other similar situation:

Start mozilla, put paint flash on, mouse over menubar. First
time you mouse over menu, it gets loaded and build. And when building
menu, menuview gets resized and that causes repaint -> you see
flash at 0.0. 
Actuly you see two flashed, one from menu-view, other from parent-view.

This flash is seen just at first mouseover,  others mouseovers
doest generae menus.

I attach patch that fixed this and first bug.

Comment 6

[Tomi Leppikangas]

Attachment: patch #2, fixed both bugs

Comment 7

[Kevin McCluskey (gone)]

I checked in a fix for the first problem since I already had other view changes 
in my tree and had tested it on multiple platforms. I'll look at the second case 
next.

Comment 8

[Kevin McCluskey (gone)]

Fixed in 4/12/2000 4:29 PM build.
Thanks for tracking this down and providing the patch Tomi!  :-)

-- Kevin

Comment 9

[Tomi Leppikangas]

Looks good, marking as verified.

Comment 10

[Tomi Leppikangas]

Looks like this has come back with scrolling menus? Didnt
see any changes in view code, i assign this to menus/[e-mail]

Comment 11

[Tomi Leppikangas]

evaughan i reassing this to you, you changed menus to use scrolling
views?

Comment 12

[Peter Trudelle]

Moving all bugs that are not dogfood+, nsbeta2+,features, or nsbeta2- to M21

Comment 13

[Eric Vaughan]

fixed

Comment 14

[tor]

I'm seeing this on a 6/14 cvs pull built on solaris/native.  Steps to reproduce:

  * start mozilla
  * turn on paint flashing (prefs->debug, flashing, debug window, capslock, etc)
  * pop down edit menu - notice while doing this that the first time a menu
      title is moused over a region the size of the exposed menu will flash
      at the display's 0,0
  * get rid of edit menu
  * select some text on the webpage - select region will flash, along with an
      area right where the edit->copy menu entry would be

Comment 15

[Eric Vaughan]

Pav this only happens on linux. Can you take a look

Comment 16

[Stuart Parmenter]

Is this due to the paste menu item being changed constantly whenever the edit
menu is used or the clipboard stuff is used?  reassigning to editor... this
might also be xpapps, or layout, or menus.. sigh.  i'm not sure who should own
this.  pink?

Comment 17

[Tomi Leppikangas]

I bet this is views thing. I have patch for this now. Patch walks trought
parent views and see are they all visible, if not, dont paint.

Comment 18

[Tomi Leppikangas]

Attachment: patch #3 for views

Comment 19

[rubydoo123]

assigning to Kin to review patch

Comment 20

[rubydoo123]

adding patch keyword

Comment 21

[Tomi Leppikangas]

Something between builds 07/28/2000 08:00 and 07/28/2000 21:00
changes so hidden menus flicker every time when mousing over
menubar. This make significant slowdown on linux.

This is show when turning paintflash on and mousing over menubar,
menu area flash even menu is hidden. Also seen in editor, toolbar buttons shift
slightly when mousing over menubar.

Comment 22

[Tomi Leppikangas]

Slowdown comes from bryner's patch around 07/28/2000 15:09

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=07%2F28%2F2000+15%3A00%3A00&maxdate=07%2F28%2F2000+15%3A10%3A00&cvsroot=%2Fcvsroot

Comment 23

[kinmoz]

Reassigning to [e-mail] (the owner of the view module) so he can
evaluate the nsViewManager2 submitted patch.

Comment 24

[Kevin McCluskey (gone)]

I can not apply Tomi's patch. It is legal for child views to be visible and have 
hidden ancestors. This is required to support CSS's specification of the 
visibility property.

Comment 25

[Christopher Blizzard (:blizzard)]

Well, there's a difference between being visible from a progromatic standpoint
and being physcially mapped on screen, right?

Comment 26

[Kevin McCluskey (gone)]

In this case they are the same. It is perfectly legal for a child view to render  
 even though it has an ancestor that is hidden. Setting the visibility to hidden 
on an ancestor does not affect whether a child view is rendered. It was designed 
this way to support CSS's visibility property.  If you need a view and all of 
it's children to be hidden you need to walk the view sub-tree and mark all of 
the view's hidden. 

Reassigning back to kin. Adding myself to the CC list.

Comment 27

[kinmoz]

This isn't really an editor issue. Changing Component to "XP Toolkit/Widgets: 

Menus"

Comment 28

[Mike Pinkerton (not reading bugmail)]

what's the actual bug  here?

Comment 29

[Tomi Leppikangas]

Actual bug is, or how it shows, from tor's comment:

  * start mozilla
  * turn on paint flashing (prefs->debug, flashing, debug window, capslock, etc)
  * pop down edit menu - notice while doing this that the first time a menu
      title is moused over a region the size of the exposed menu will flash
      at the display's 0,0
  * get rid of edit menu
  * select some text on the webpage - select region will flash, along with an
      area right where the edit->copy menu entry would be

Actual performance bug here is that views that are visible, but are child
of invisible view, are painted on screen. Nothing is show thougt native parent
window is invisible, but trip to x-server is done anyway, and this is slow on
linux. My patch adds loop that goest trought all parent and make sure they
are all visible, and if not, don't repaint.

This isn't so big issue with edit menu, but there might be situations
where this helps more.

Comment 30

[Mike Pinkerton (not reading bugmail)]

why is this a toolkit bug and not a view bug?

Comment 31

[Kevin McCluskey (gone)]

Well there was an assumption that this was a view bug, but this really isn't a 
bug in the view system. It behaves this way by design (i.e child views 
visibility is independent of their ancestor's visibility) So if the desired 
behavior is pop a menu up and down without causing the painting of the child 
views, the code which manages the menus will need to walk the view hierarchy and 
set all of the child view's visibility. 

In most cases, when CSS is used child elements inherit there visibility from 
their parent and this issue does not come up. I believe the menu code does not 
use CSS properties to control the visibility of the popup. Instead, the menu 
popup code directly manipulate the visibility of a parent view. If this is the 
case, then it's up to the menu code to set the visibility on the child views.

Comment 32

[Peter Trudelle]

reassigned to pinkerton, ccpavlov, ->M20, but could still take patch if it is
worth the risk.

Comment 33

[Peter Trudelle]

->future

Comment 34

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

I don't see this problem with my view manager in Win32, but I don't understand 
why you would see it with nsViewManager2 either.

When the hidden view is invalidated, we cause an invalidation to be sent to a 
hidden widget. Surely this should just be dropped on the floor either by the X 
server or (better) by our X widget code?

Comment 35

[Mike Pinkerton (not reading bugmail)]

new owner

Comment 36

[Markus Hübner]

Any news on that one?

Comment 37

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

Markus, can you verify that this is still a bug? I think it may have been fixed
by my fix for bug 130263.

Comment 38

[Markus Hübner]

seems okay

Comment 39

[Markus Hübner]

Can we marke this fixed or is there still something that needs to be done?

Comment 40

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

It's been fixed, at least the problem that Tomi originally reported.