268090 - [FIXr]corrupt painting of select

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Felix Miata]

OS/2 trunk 2004110508. Problem not found in OS/2 trunk 2004110412 or Linux trunk
2004110506.

To reproduce:
1-remove *.css from profile's chrome directory
2-start browser
3-open subject URL
4-try to select some combinations of product and component
5-enter, then try to edit summary or URL

Actual behavior:
1-select scrollbars mispositioned
2-select highlights misplaced
3-input cursor misplaced or missing
4-displayed characters in input don't follow keystrokes

Expected behavior:
1-scrollbars reflect actual position in select lists
2-highlights only where you put them in select lists
3-input cursor visible where it belongs
4-displayed characters in input properly reflect actual keystrokes

Painting is corrected when some other window is placed over browser and then
removed.

Comment 1

[Felix Miata]

Attachment: screenshot of corrupt selects

Comment 2

[Peter Weilbacher]

Yes, I definitely also see this on my build of Nov 6th, although it is more
general screen corruption not just related to input elements. Even the display
of the small spinners in each tab and the "Site Navigation Bar" are not always
updated. I also read about input problems on with 20041104 on Linux which
supposedly vanished today.

Looking through checkins nothing really rings a bell. Hmm, bug 267302 sounds
very general, but that's probably not it...

Comment 3

[Felix Miata]

My description of the INPUT problem is described in bug 268352

Comment 4

[Felix Miata]

Changing severity to blocker, because impossible to use
https://bugzilla.mozilla.org/query.cgi due to this. No improvement apparent in
2004110908.

Comment 5

[Peter Weilbacher]

OK, this is caused by the changes to mozilla/view/src/nsViewManager.{cpp,h} in
the patch called "remove childCovers" in bug 243726. Any idea why those changes
would cause problems on OS/2?

Comment 6

[Boris Zbarsky [:bzbarsky]]

One possibility is that the assumptions mentioned in bug 243726 comment 51
paragraph 2 (not counting the quoted stuff) are false here.  If you make changes
per bug 243726 comment 50 to only count the widgets that we actually call
UpdateWidgetArea on, does that help?

Comment 7

[Peter Weilbacher]

(In reply to comment #6)
> If you make changes
> per bug 243726 comment 50 to only count the widgets that we actually call
> UpdateWidgetArea on, does that help?

I fiddled around with it a while, e.g. placed the
    childWidget->GetBounds(r);
    r.ScaleRoundIn(mContext->DevUnitsToAppUnits());
into the if section that calls UpdateWidgetArea (both including and not
including the children.* lines), but that didn't help and I am not sure that you
really meant that. (I don't understand the code anyway...) Any more hints?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to try

I meant something like this... Does applying it help?

Comment 9

[Peter Weilbacher]

No, as far as I can see that has no effect.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Does it help if you remove the .Or() line?  That is, remove the child
optimization altogether?

Comment 11

[Peter Weilbacher]

Yes, if I remove the .Or line, both before and after your "Patch to try", the
bugzilla query page and everything else I tried displays, scroll, and marks fine. 

Hmm, easy solution is to #ifndef XP_OS2 that line. But display is already slower
than on other platforms so I guess we can use any optimization... Dainis, are
you the right person to ask? Why would .Or give different results on OS/2?

Comment 12

[Dainis Jonitis]

I wrote nsRegion, but I can not say that I understand fully what view manager is
doing with it :)

Most likely the nsRegion.Or should work exactly the same on all platforms,
because it does not contain platform specific code. I would rather suspect that
OS/2 widget code does not correctly report it's dimensions or something like that.

Can you add debug prints and look if view->GetDimensions() and view->GetOffsetTo
return reasonable values. Maybe because OS/2 coord space is upside-down it
forgets to flip y coords somewhere.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Another option is to back out the last patch in bug 243726 locally and compare
the invalidates/paints that happen then with the ones that happen with the patch
applied...  (does OS/2 have paint flashing?)

Comment 14

[Peter Weilbacher]

Is this "paint flashing" something within Mozilla or a feature of some external
tool?

Tried the debug output, like this:
    childWidget->GetBounds(r);
    printf("1st: x/y=%d/%d, w/h=%d/%d\t", r.x, r.y, r.width, r.height);
    r.ScaleRoundIn(mContext->DevUnitsToAppUnits());
    printf("2nd: x/y=%d/%d, w/h=%d/%d\n", r.x, r.y, r.width, r.height);
    children.Or(children, r);
When scrolling in the Component: field of Buzilla's query I get these values:
   1st: x/y=276/44, w/h=16/16	2nd: x/y=3312/528, w/h=192/192
   1st: x/y=2/107, w/h=823/681	2nd: x/y=24/1284, w/h=9876/8172
   1st: x/y=276/44, w/h=0/0	2nd: x/y=3312/528, w/h=0/0
   1st: x/y=0/1, w/h=0/0	2nd: x/y=0/12, w/h=0/0
   1st: x/y=0/1, w/h=0/0	2nd: x/y=0/12, w/h=0/0
   1st: x/y=0/1, w/h=0/0	2nd: x/y=0/12, w/h=0/0
   1st: x/y=0/1, w/h=0/0	2nd: x/y=0/12, w/h=0/0
   1st: x/y=0/1, w/h=0/0	2nd: x/y=0/12, w/h=0/0
   1st: x/y=0/1, w/h=0/0	2nd: x/y=0/12, w/h=0/0
   1st: x/y=0/1, w/h=0/0	2nd: x/y=0/12, w/h=0/0
   1st: x/y=0/1, w/h=0/0	2nd: x/y=0/12, w/h=0/0
repeated often. If the 1st ones are pixels (I hope the 2nd values are not on
screen pixels!), then those values cannot be too far off, although I don't
really know how to check which on screen widget they correspond to (I didn't
find an OS/2 tool to measure relative pixel distances on the screen).

Comment 15

[Dainis Jonitis]

I opened bug 269632, where I implemented paint flashing for OS/2. Peter, you may
be interested to use this while investigating invalidation problems.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yes, the first set is pixels (and the second is twips).

I'm really somewhat at a loss as to what's going on here...  the only way I can
explain it is that before this patch the "childCovers" boolean ended up false so
we invalidated the rect in the parent widget, and with this patch we either
invalidate a smaller rect or nothing at all (and then don't paint the child
widget for some reason, which is odd; with the patch I attached to this bug we
should be invalidating the child widget!).

Comment 17

[Peter Weilbacher]

Attachment: new screenshot

I don't know why it happens but I now have a more specific view which regions
are not repainted. This new screenshot shows part of the the bugzilla query
page again, after selecting the Browser product, scrolling a line down in the
Product box and a few lines downwards in the Components box without selecting
anything. The region that does not get repainted is exactly the one that would
be overlayed by the drop-down box in the Summary line.
If I then open and close that box once, everything in that part of the page
gets repainted correctly until I reload the page. Mozilla seems to think that
the drop-down box is open after loading the page. (The same is true for all
other drop-down boxes on that page, including missing repaints above the ones
for "Advanced Querying Using Boolean Charts:" because those open upwards.)

Because those regions do not get repainted, text selection does not happen,
either.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Another patch to try

> The region that does not get repainted is exactly the one that would
> be overlayed by the drop-down box in the Summary line.

Oh, interesting.  That could cause some issues, yes...

Note that this patch is not acceptable to land as-is, since GTK1 reports
nsIWidgets as not visible if they have no mWidget, and we hit that case for
scrollwidgets, looks like (that is, this patch regresses painting for GTK1, and
putting the whole body of the loop inside the IsVisible() check makes us not
paint things like selections altogether....)  So if this helps on OS/2, we may
need to consider either changes to the GTK1 widget impl, api changes for
nsIWidget to retrieve the "real" show/hide state, or both.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Shouldn't GTK1 be checking its mMozArea when it has one, rather than its
mWidget?  Certainly Show() works on mMozArea....

In any case, I'm not sure whether we want to check what nsIWidget::IsVisible
means in this code (or in the other place in the viewmanager where IsVisible is
called, for that matter).

Comment 20

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

why is updatewidgetarea traversing those dropdowns? since recently they
shouldn't be children of any nsIwidget

Comment 21

[Peter Weilbacher]

Comment on attachment 165859 [details] [diff] [review]
Another patch to try

Yes, the extra visibility check gets rid of the OS/2 problem.

Comment 22

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #20)
> why is updatewidgetarea traversing those dropdowns?

I'm not sure, but I think we want the visibility check there anyway...  The
problem is that IsVisible() doesn't say whether the widget is shown (in the
Show()/Hide() sense), but rather whether it's actually visible (eg on GTK2 a
widget obscured by some other window will report false, looks like).  What we
_really_ want here is to check whether the widget has had Hide() called on it, I
would think.  Note that the attached patch doesn't just fix the OS/2 code here,
but also the Linux and Windows issues in bug 269736...

In any case, in the short term we can use IsVisible().  I'll attach a GTK1 patch
to make it lie a little less, perhaps.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 165859 [details] [diff] [review]
Another patch to try

This is needed in any case

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix GTK1 IsVisible() a tad

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 165918 [details] [diff] [review]
Fix GTK1 IsVisible() a tad

Actually, this is no good.  :(	This sometimes causes combobox widgets to
appear when they should not... I can't figure out why that happens, and it's
not particularly reproducible.	:(  The only thing I can think of is that
mInternalShown gets into a bogus state...

Comment 26

[Boris Zbarsky [:bzbarsky]]

Actually, I _can_ reproduce it.  Just start mozilla with this bug's URL.  Note
that loading this page later works, so this is just a startup thing, in some way...

Comment 27

[Peter Weilbacher]

Comment on attachment 165859 [details] [diff] [review]
Another patch to try

I didn't notice before, but while this patch fixes problems in the content
area, there are are a few problems left in the chrome (e.g. unpainted site nav
bar and on tab activity indicator). If I comment the line with .Or() those
problems disappear.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 165859 [details] [diff] [review]
Another patch to try

What if you combine the two patches?   That is, only Or() when we call
UpdateWidgetArea(), and only do all of that if the widget is visible?

Comment 29

[Peter Weilbacher]

No, it doesn't matter where the Or() is, in both cases I get some corruption,
either in the content area or in the chrome or both.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: Combination approach

So this doesn't work on OS/2?

Comment 31

[Dainis Jonitis]

Boris, was it your intention in children.Or to use +=? It just modifies the
temporary nsRect that is returned by GetDimensions and later it is discarded
anyway. Isn't operator + sufficient?
BTW You may want to remove the leading apostroph in comment "region into 'view's
coordinate space"

Comment 32

[Dainis Jonitis]

With 'Combination approach' patch 166056 I do not see any problems with
listboxes, site navigation bar or tab activity repaints.

But I see the repaint problem on vertical scrollbar when scrolling page from
down to up. Use mouse wheel or press upper arrow to see this. This seems to be
some roundup problem, because there is always six black stripes and they repaeat
after equivalent distance. When page is scrolled down the vertical scrollbar
does not show any black lines. Not sure this is related to this bug.

Comment 33

[Dainis Jonitis]

Attachment: black stripes on vertical scrollbar

Comment 34

[Boris Zbarsky [:bzbarsky]]

> Isn't operator + sufficient?

You're right.  It is.

Does the vertical scrollbar issue go away if you back out the patches (either
just the last or both) from bug 243726?

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 166056 [details] [diff] [review]
Combination approach

roc, this fixes at least most of this bug, the invalidation issues in bug
269736, and bug 268252.  I think there are still widget issues this is
wallpapering, but given that nsIWidget::IsVisible is so flaky, this is the best
we can do for now, I think... I can file a followup bug on having a better way
to detect hidden widgets and we may want to keep the platform bugs open for
fixing the platform widget issues.

Comment 36

[Peter Weilbacher]

Yes, that patch does it for me, too. (My own try of a "combined patch" that did
not work was, ahem, more primitive.)
I see the black stripes on the scrollbar, too, but only in a debug build. They
are not caused by the child covers patch of bug 243726 but I somehow don't
manage to reverse the v3 patch from the same bug.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Taking

Comment 38

[Asa Dotzler [:asa]]

Comment on attachment 166056 [details] [diff] [review]
Combination approach

a=asa for 1.8a5 checkin.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Fix checked in.  It may be worth filing a followup bug on the OS/2 widgets --
Robert is right that the popup widgets should not have been nsIWidget kids of
anything.

Comment 40

[Michiel van Leeuwen (email: mvl+moz@)]

*** Bug 270641 has been marked as a duplicate of this bug. ***

Comment 41

[Felix Miata]

Verified in 2004111908 trunk.