126263 - Scrollbars overpaint divs

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[Mike Kaply [:mkaply]]

Go to the above testcase.

Click on Action, then Print.

Notice the scrollbar for the listbox overpaints the menu.

Comment 1

[rods (gone)]

When I print, I don't see the menu and I don't see the scrollbars printing over top.

Comment 2

[Mike Kaply [:mkaply]]

Attachment: Picture of problem

Man was I unclear :)

I meant Action->Print in the DHTML application.

I've attached a picture of the problem. I'm assuming this is going to be fixed
with XBL forms. Do we know when those are going in?

Comment 3

[Kevin McCluskey (gone)]

Moving to future. P3. This is because we are still using native scrollbars for
the dropdown menu. This should be fixed when XBL form elements land. They are
currently scheduled to land for mozilla1.0

Comment 4

[Mike Kaply [:mkaply]]

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

Comment 5

[Mike Kaply [:mkaply]]

OK so XBL forms aren't going to make it for 1.0. 

HELP!

I have to have a fix for this.

I took some advice from bryner and looked at hooking up XBL scrollbars in 
nsScrollframe but did not get anywhere.

Any help is appreciated.

Comment 6

[jhp (no longer active)]

Attachment: patch to use nsGfxListControlFrame

Comment 7

[jhp (no longer active)]

Attachment: layout/html/forms/src/nsGfxListControlFrame.cpp

Comment 8

[jhp (no longer active)]

Attachment: layout/html/forms/src/nsGfxListControlFrame.h

Comment 9

[jhp (no longer active)]

In order to solve this problem, I resurrected nsGfxListControlFrame.  I tried to
keep it as close as possible to nsListControlFrame, and for the most part, it
works (it even works with the testcase here!).  However, there are some issues:

1) Scrollbar always paints in the listboxes, even when it is not needed
2) PaintFocus() doesn't work -> 'mFocused' is never equal to 'this' in the code.
3) Dropdowns draw in the wrong location when page is scrolled.
4) Dropdowns with scrollbars -> scrollbar draw incorrectly (not in the right
position; the arrows point to the left and right, rather than up and down). 
Sometimes, the scrollbar doesn't even paint.
5) Dropdown does not work correctly when it draws below the window edge.
6) Do we need to StatefulFrame?

I think many of these could stem from the Reflow code.  I tried to make it work
as best as possible, but I am not too familiar with this code.  I'd appreciate
any help here.

Comment 10

[Mike Kaply [:mkaply]]

bryner, rod,

I know you guys don't care much about this, but this is going to be a stop ship 
issue on the IBM browser, and is probably going to cause one of our customers to 
give up on Mozilla browsers.

Essentially, this bug renders cool things like DHTML menus, etc. USELESS on 
Mozilla if the web page contains listboxes.

Check out:

http://www.kaply.com/work/demo.html

How useless is that?

Any help you could provide would be much appreciated.

Comment 11

[jhp (no longer active)]

Attachment: patch to use nsGfxListControlFrame for Listboxes only

Comment 12

[jhp (no longer active)]

Attachment: layout/html/forms/src/nsGfxListControlFrame.cpp

Comment 13

[jhp (no longer active)]

Attachment: layout/html/forms/src/nsGfxListControlFrame.h

Comment 14

[jhp (no longer active)]

Due to the many bugs in using nsGfxListControlFrame with comboboxes, I have
changed the combobox code to use the old code.  So now only listboxes use
nsGfxListControlFrame.  Here are the remaining issues:

1) If you click on the first item in a listbox, and then press the down arrow in
order to scroll the list, it scrolls down one item and then immediately jumps
back up to the top.  Afterwards, the scroll arrows will work correctly.  The
jump back to the top happens because Reflow() is called when you first click on
the down arrow, causing the currently selected item to come into view.
2) Scrolling using the thumb does not work.  It starts to scroll and then
immediately stops.  I think this is also related to the Reflow() code.  If I
change line 938 in nsGfxListControlFrame.cpp to call containerFrame->Reflow(),
rather than firstChildFrame->Reflow(), then I can scroll using the thumb until I
select an element in the list box.
3) Scrollbars do not disable when they are not needed.
4) PaintFocus() doesn't work -> 'mFocused' is never equal to 'this' in the code.

Comment 15

[rods (gone)]

Attachment: full patch

Here is the full patch using "cvs diff -uN" (meaning it includes the new files)

Status:
1) thumb/slider scrolling - works
2) Print Preview/Printing sizing - works
3) focus rect - works
4) arrow key issue - couldn't reproduce
5) disable slider when not needed - didn't look at

Note: the changes to the css files for PP/Printing cannot be ifdef'ed

Comment 16

[Jessica Blanco]

Rod, I can still reproduce the 4)arrow key issue- listed as (1) in 
Javier's comment #14. This isn't a problem using the 'down arrow key'. 
To recreate this problem-

-Go to bugzilla.mozilla.org/query.cgi
-Click on 'All' in the Platform list box (first option displayed in 
listbox)
-Then click on the down arrow on the scrollbar for that listbox. (just 
one click, don't hold the mouse down)
-Notice the listbox scrolls down one, but then it pops back up one to 
where the selected option is displayed.

Expected result- The listbox should scroll down one to where the 
selected option 'All' is no longer displayed. It should not scroll 
back up.

Comment 17

[jhp (no longer active)]

Attachment: full patch updated

An updated patch against the MOZILLA_1_0_BRANCH.

This patch fixes two problems:
1) Added code in nsGfxListControlFrame::DidReflow() to not call ScrollToIndex()
during an incremental reflow.  This prevents the listbox from jumping back to
the first item when you click on the down scroll arrow the first time.
2) Made some of the nsGfxListControlFrame code in nsCSSFrameConstructor.cpp to
look more like the code that instantiates the nsListControlFrame.  This results
in the scroll bar actually disappearing when it is disabled.

The two remaining issues are purely cosmetic:
1) Once you scroll the listbox, a selection will not highlight the full width
of the listbox, only the width of the characters.
2) Sometimes when scrolling, the focus rect will draw artifacts outside of the
actual listbox.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Mike Kaply [:mkaply]]

So are XBL form controls completely dead at this point?

Comment 22

[rods (gone)]

It's a good question for Bryner

Comment 23

[Brian Ryner (not reading)]

I've stopped working on XBL form controls, after we found that they measurably
increased memory usage and increased page load time by nearly 20% on pages such
as the Bugzilla query page.

If we can fix those problems, I don't see any reason not to enable XBL form
controls, but I think someone else will need to pick this up if that's going to
happen.

Comment 24

[Mike Kaply [:mkaply]]

OK, what about the possibility of taking this patch Javier created that switches 
Mozilla to use non native scrollbars?

Comment 25

[jhp (no longer active)]

Since XBL form controls aren't going in, could we have some one look at this
patch?  We have several customers complaining about this one instance of
platform scrollbars, and we would like it fixed.  I took a stab at it, but I
don't know enough about this code to do it properly.  It would be great to get
this in before 1.3 final.

Comment 26

[Chris Nalls]

Marking topembed+ as part of topembed triage

Comment 27

[Kevin McCluskey (gone)]

-> jkeiser

Comment 28

[Carine Held]

Attachment: Test Case

Comment 29

[Mike Kaply [:mkaply]]

This bug is VERY VERY bad.

We have put a fix together in this bug.

Is anyone interested in this?

Nominating for 1.3b

Comment 30

[Mike Kaply [:mkaply]]

There is a patch. In the bug.

Why is this moved out two releases?

This bug makes DHTML SUCK hard on Mozilla.

Comment 31

[Boris Zbarsky [:bzbarsky]]

I have to be frank; I can probably review this but I don't know when that will
be.  It won't be within the next two weeks, no matter what -- I simply do not
have the time due to non-Mozilla commitments.

Some things that would help any potential reviewers, however:

1)  What are the differences between nsGfxListControlFrame and
    nsListControlFrame, if these are small enough to summarize succinctly?  Is
    it at all feasible to have them derive from a single base class so that the
    extra code in nsGfxListControlFrame is as small as possible?
2)  Under what conditions is USE_NON_NATIVE_SCROLLBARS defined?

Comment 32

[John Keiser (jkeiser)]

The code duplication issue is serious since the patch has bitrotted
(nsListControlFrame has changed a lot).  Also, "full patch updated" is probably
not what we're looking for.  That says it's against branch.

The reason this moved out 2 milestones is it needs a lot of work.  I am on it
now, but I suspect that it's going to take us past 1.3beta (a week or two from
now), which means it misses 1.3 as well (I doubt anyone would let us check in a
change like that to a "stable" release).

Comment 33

[John Keiser (jkeiser)]

Comment on attachment 90823 [details] [diff] [review]
full patch updated

Minusing, we need to do this right and not have two implementations of list
box.  We're just asking for big trouble here, especially with those static
casts.

Comment 34

[John Keiser (jkeiser)]

Kaply, Pedemonte: if you want this on branch, actually, I'm not worried about
that patch (since it affects only OS/2) as long as the impact on other code is
more minimal.  An easy way to accomplish this is to either rename
nsGfxListControlFrame to nsListControlFrame in nsGfxListControlFrame.h or to
typedef nsGfxListControlFrame nsListControlFrame.  I don't want to put this
particular patch on trunk, though.

Comment 35

[jhp (no longer active)]

The patch was meant to be a stop-gap effort, enough to remove native scrollbars
from the releases to our customers.  I don't know enough about this code to
write it up properly, and many issues remain with it.  I was hoping that by
posting this that someone who knows more about this area of the code would write
it the correct way or at the very least point out to me what I need to be doing.

I'll try to update the code to the trunk, and see if I can make then derive from
the same class.

Comment 36

[Mike Kaply [:mkaply]]

We just found a very strange thing.

We thought we could work around this by using IFRAMEs for the menus, since
IFRAMEs are child windows and should overlay the scrollbars.

What we found is that this problem exists for IFRAMEs as well, but ONLY on
Windows and Linux (GTK)

See:

http://www.kaply.com/work/zorder.html

The scrollbar should NOT paint through the IFRAME.

Note this is not the same bug, because in the iframe case we are dealing with
window zorder.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 90823 [details] [diff] [review]
full patch updated

taking out of my queue pending Javier and jkeiser working out what the plan is.

Javier, for your internal use do you need this on trunk or branch?

Comment 38

[John Keiser (jkeiser)]

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

Comment 39

[jhp (no longer active)]

Boris, we would need this for 1.3.

Comment 40

[John Keiser (jkeiser)]

Pedemont: it should probably be enough for you to do a typedef or alternate name
for nsGfxListControlFrame--the only thing I care is that the code that
references nsListControlFrame doesn't have to *syntactically* change so that
there is no impact on the normal codepath.  If you need this for 1.3 I would be
happy to have it in the tree (I'll give it r=) as long as you guys are comfy
with your level of testing (this patch will only affect OS/2).  I suspect there
are features missing that were landed on trunk between the time this patch was
made and today; things like the ability to search in the select by typing the
first few letters of an option, for example.  I am not yet familiar enough with
scrollbar code to do a full review or to write this into the existing code.

The fuller fix, which I am investigating now, is to use XUL outliner internally
or a similar class (basically a full rewrite of the <select> layout).  This will
*not* happen in 1.3 timeframe.  I suspect we will want to pull this code back
out at that time and have OS/2 work like everybody else.

Comment 41

[jhp (no longer active)]

I am currently trying to implement this by ifdefing nsListControlFrame. 
However, due to the changes that have gone into the trunk since the patch was
last updated, some of the things that used to work do not work now.  Still
investigating...

Comment 42

[jhp (no longer active)]

Attachment: trunk patch

With this patch, I just ifdefed nsListControlFrame.  It looks kinda ugly, but
then it doesn't create 2 files.  Listboxes work ok (there are some paint
issues), but comboboxes crash whenever I move the mouse over them.

Keiser, I know you are busy with other things, but I could use some help with
some of the more glaring issues.  Unfortunately, I don't know enough about this
code.

Comment 43

[John Keiser (jkeiser)]

This patch is a lot better and more manageable.  If *this* one can be made to
work I don't mind if we turn it on for all platforms but I suspect that will not
occur until 1.4alpha; I think we can convince drivers to do this for OS/2 only
during the 1.3 freeze though.  I'll apply and see if I can deal with the problems.

Comment 44

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

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

Comment 45

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

We can't hold 1.3 for this. However, I'd consider landing this kind of patch up
to 1.3final given that it's all #ifdefed out for everyone except OS/2. And I
would really love to see a real fix for this problem.

Comment 46

[jhp (no longer active)]

Attachment: trunk patch v2

With this patch on the trunk, listboxes work very well (no more painting issues
when scrolling).  Also, comboboxes are working much better (don't crash
anymore);  however, the drop down does not draw in the correct place for all
comboboxes.  Maybe a reflow problem.

Comment 47

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

We definitely want this for 1.4a for all platforms, no #ifdefs. Of course we'll
need to test it a fair bit and eliminate as many regressions as we can. Great work!

I have one question though ... how hard would it be to reorganize so that the
nsListBoxFrame is *inside* the scroll frame? It would be very nice if we got the
scrolling just by making <select> be "overflow:auto", so it got wrapped in its
own scrollframe by regular frame construction. Also, it would mean that other
values of 'overflow' would work correctly on <select>.

Comment 48

[jhp (no longer active)]

Attachment: trunk patch v3

Cleaned up nsCSSFrameConstructor.cpp.  Roc, this patch is not ready for prime
time.  While listboxes work with only very minor regressions, comboboxes are
pretty much useless.

Comment 49

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

I was thinking about this some more and I think that we can't simply make
nsListControlFrame use nsGfxScrollFrame, because that will break builds that do
not include XUL.

I think we should find a way to make nsListControlFrame not actually be a scroll
frame itself, but wrap it in a scroll frame by declaring it 'overflow:auto'.
Then it would automatically either use nsGfxScrollFrame or nsScrollFrame
depending on whether XUL was enabled. Also, other values of 'overflow' could be
applied to <select> and they would probably work. It might even simplify the code.

Comment 50

[jhp (no longer active)]

Any chance you could give me some hints on how to get started with this?  Is
there any code in the tree that does a similar thing?

Comment 51

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

I'm working on this.

The approach I'm trying is to make nsListControlFrame inherit from
nsHTMLContainerFrame. It will contain the scroll frame (either nsGfxScrollFrame
or nsScrollFrame) rather than being one itself. This is a little bit more
overhead but it may turn out to simplify the code and it completely insulates
nsListControlFrame from changes in the way we do scrolling.

Comment 52

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

OK ... in bug 194240, there is a branch which will land soon which means we'll
always be using Gfx scrollbars, so nsListControlFrame can use nsGfxScrollFrame
unconditionally. So I'm going to forget about my approach and go back to what
Javier was doing, which will have no footprint impact and be closer to the
current code.

Comment 53

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

Attachment: work in progress

OK, this is turning out to be reasonably simple so far. With this, listboxes
mostly work. Focus rings appear to be broken. Dropdown lists also seem to work
OK but the sizing is a little off (doesn't take account of the space for the
arrow?) and no scrollbar appears when the dropdown needs to be scrolled...
scrolling does work though if you use the keyboard.

Comment 54

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

I've found and fixed the focus ring problem in my tree. Listboxes now seem to
work perfectly. There are still a couple of layout issues with combobox
dropdowns, which I'll try to track down ASAP. I would like to land this soon...

Comment 55

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

I'm taking this over

Comment 56

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

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

Comment 57

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

I made good progress over the weekend. I'm down to just a couple of bugs and I
have a handle on both of them. The trickiest thing seems to be that with Gfx
scrollbars in comboboxes, you have two different sources of anonymous content
for the same SELECT element, and you also have two sets of event listeners for
the same element. There's some work required to make them work well together.

Comment 58

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

Attachment: THE FIX

This is it. It really works for me. Try it out.

It's not as big a patch as it looks, since it's diff -u6. 350 lines of code
removed, 410 added. Much of the added stuff is just comments.

Comment 59

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

Some good things about this patch:
-- Fixes this bug and duplicates.
-- Listboxes and comboboxes get the Mozilla theme now (e.g., if you're using
Modern, listboxes get Modern scrollbars).
-- Fixes other obscure non-filed bugs, e.g., you can now have a listbox with
visibility:hidden containing some OPTIONs with visibility:visible.
-- I believe this removes the last usage of nsScrollFrame and nsScrollingView.
Once this is checked in and the Mini-Mo branch lands, we can remove those files
from the build.
-- This also means that various hacks in layout to deal with nsScrollFrame are
no longer needed (e.g., see the assertion and comment in the patch in
nsContainerFrame::PositionFrameView).
-- In general this cleans up layout considerably because comboboxes now obey the
same rules for views and so on as the rest of layout. E.g., see how the patch
replaces almost all of SyncViewWithFrame with a call to
nsContainerFrame::PositionFrameView. This will enable further cleanup in layout
and views.

Some bad things about this patch:
-- Footprint will probably increase a little for pages with lots of
listboxes/comboboxes, because Gfx scrollbars create more frames than native
scrollbars.
-- There are some hacks in comboboxes that I haven't fixed that can probably be
fixed.
-- It creates an unconditional dependency on XUL (via nsGfxScrollFrame). But
this is already the approach taken by the Mini-Mo branch so I think it's the
right thing (especially since it lets us get rid of nsScrollFrame and friends).

Comment 60

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

Attachment: testcase

Here's a testcase I was using.

Comment 61

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

Attachment: FIX v2

Fixes a couple of issues I found:
-- change mHTMLFormControls->IndexOf aFlush parameter to PR_TRUE to ensure new
content is added to the form control map before looking up the current element.
Otherwise we get index=-1 and we fire the assertion. I don't see how this ever
worked before.
-- restore form control state for the listbox after the scrollable view has
been created, so it can be manipulated during restore

Comment 62

[John Keiser (jkeiser)]

roc: it IndexOf() never worked before, and there's a bug on it (bug 139568) ...
the problem is, adding that flush increases pageload by 5-10%.  You can mitigate
it some by first trying it without the flush, and if that fails, trying with the
flush, but that still significantly increases pageload.  Until save/restore is
triggered *after* document notifications have gone through (bug 166636 comment
3) this penalty might just have to be paid.

http://www.johnkeiser.com/mozilla/select should give a few more tests.

Comment 63

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

OK. I'll adjust the patch so that instead of changing to PR_TRUE, there's a
comment explaining why it's PR_FALSE and that the assertion is expected.

I have some ideas about fixing the core problems and I'll join you in bug 166636.

Comment 64

[Mike Kaply [:mkaply]]

Should the scrollbar go away when it isn't needed?

Comment 65

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

Perhaps, but our current listboxes always have a scrollbar, and I just want to
preserve current behavior as much as possible for now.

That's just one preexisting bug that I found that should be fixed eventually.
Another cool preexisting bug I found is that in listboxes whose size is set by
CSS (like most of the listboxes in my testcase), pageup and pagedown keys
actually move the selection DOWN and UP by one respectively. Weeeird.

Comment 66

[Brian Ryner (not reading)]

Comment on attachment 117200 [details] [diff] [review]
FIX v2

A few additional comments in addition to jkeiser's comment about IndexOf():

>Index: layout/html/base/src/nsContainerFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsContainerFrame.cpp,v
>retrieving revision 1.163
>diff -u -6 -r1.163 nsContainerFrame.cpp
>--- layout/html/base/src/nsContainerFrame.cpp	5 Mar 2003 14:26:31 -0000	1.163
>+++ layout/html/base/src/nsContainerFrame.cpp	14 Mar 2003 04:42:05 -0000
>@@ -440,12 +440,15 @@
> 
>     // it's possible for the parentView to be nonnull but containingView to be
>     // null, when the parent view doesn't belong to this frame tree but to
>     // the frame tree of some enclosing document. We do nothing in that case,
>     // but we have to check that containingView is nonnull or we will crash.
>     if (nsnull != containingView && containingView != parentView) {
>+      NS_ASSERTION(PR_FALSE, "This hack should not be needed now!!!");

I'd prefer NS_WARNING() to NS_ASSERTION(PR_FALSE, ...)

>Index: layout/html/base/src/nsGfxScrollFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsGfxScrollFrame.cpp,v
>retrieving revision 3.115
>diff -u -6 -r3.115 nsGfxScrollFrame.cpp
>--- layout/html/base/src/nsGfxScrollFrame.cpp	22 Feb 2003 00:30:20 -0000	3.115
>+++ layout/html/base/src/nsGfxScrollFrame.cpp	14 Mar 2003 04:42:09 -0000
>@@ -1014,17 +1014,19 @@
> }
> 
> 
> nsIScrollableView*
> nsGfxScrollFrameInner::GetScrollableView(nsIPresContext* aPresContext)
> {
>-  nsIScrollableView* scrollingView;
>   nsIView*           view;
>   nsIFrame* frame = nsnull;
>   mScrollAreaBox->GetFrame(&frame);
>   frame->GetView(aPresContext, &view);
>+  if (!view) return nsnull;

Is there a reason this can be null now and it couldn't have been before?

Just to make sure, have you tested:

- keyboard scrolling of the listbox and dropdown
- mousewheel scrolling of the listbox and dropdown
- optgroups
- pageload performance

Comment 67

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

> I'd prefer NS_WARNING() to NS_ASSERTION(PR_FALSE, ...)

OK

> Is there a reason this can be null now and it couldn't have been before?

I don't think so. I was tracking down a crasher and I put this code here to see
what happened, but I fixed the crasher another way.

> - keyboard scrolling of the listbox and dropdown

Yep

> - mousewheel scrolling of the listbox and dropdown
> - optgroups

A little, but I should do this more thoroughly.

> - pageload performance

No. I need to set up my own pageload tests, but I would like to get jrgm to run
a set of tests for me to get "official" results.

Comment 68

[Boris Zbarsky [:bzbarsky]]

> I'd prefer NS_WARNING() to NS_ASSERTION(PR_FALSE, ...)

Or NS_ERROR() if you really want the assert.

Comment 69

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

Mousewheel was horked, optgroups worked. New patch coming momentarily.

Comment 70

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

Attachment: fixes

This makes nsIScrollableFrame implement nsIScrollableViewProvider, so when an
nsGfxScrollFrame is focused mousewheel events scroll that frame. I changed
nsIScrollableViewProvider::GetScrollabelView to take aPresContext just like
nsIScrollableFrame::GetScrollableView. The mousewheel works now.

I guess the last thing is to check pageload.

Comment 71

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

jrgm, bless his soul, found a 2% overall pageload hit, focused on pages like
bugzilla query with lots of listboxes/comboboxes.

On the assumption that the slowdown is due to scrollbar frame construction and
reflow, I think I need to a) make scrollbar frames be constructed lazily, only
when the scrollbar is actually shown and b) modify listboxes so that the
scrollbar only appears when it's needed. I probably can't make that happen this
week though, so this may miss 1.4a and hence be postponed to 1.5 :-(.

Comment 72

[Mike Kaply [:mkaply]]

I'm sorry, I don't buy it.

This bug is HUGE and 1.4 is going to be a MAJOR release.

We're letting other pageload increasers in the build as we speak.

This NEEDS to go in.

This makes DHTML based forms apps unusable.

Comment 73

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

Actually I'm working on it and making progress. I can make scrollbars construct
lazily by adding a rule "scrollbar[collapsed='true'] { -moz-binding:none; }".
The only problem is that we crash when a scrollbar dynamically appears, because
nsGfxScrollFrame can't handle scrollbar frames being torn down and recreated. I
should be able to fix that. Then I need to fix listboxes so a scrollbar only
appears if needed. The nice thing is that lazy scrollbars should actually make
pageload *faster* because we'll be able to avoid horizontal scrollbar
construction on a lot of pages.

I just don't have much time right now; big deadline tonight. During the weekend
and early next week i sould be able to work on this a bit more.

Comment 74

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

Attachment: new patch

Okay!!! This patch adds working "lazy scrollbar binding". The main part is the
very end of the patch where we set "-moz-binding:none" on collapsed scrollbars.
This avoids constructing anonymous content and frames for scrollbars which
aren't used. The rest of the changes to the patch fix bugs that were exposed by
that change.

nsGfxScrollFrame was forwarding Insert/Append/Remove/ReplaceFrame(s) to its
scroll box, for no reason I could see, and that was making scrollbar frame
recreation go haywire (the new scrollbar frames would be inserted under the
scroll box). I got rid of that forwarding hack and retested everything I could
think of (especially scrolling menus, which seemed to be why it was added) and
everything seems to work fine.

There were a few other quirky things. Normally frame recreation adds the new
frame as a child of the primary frame for the content parent, which is wrong
for the anonymous scrollbars; they need to be attached to the nsGfxScrollFrame
for the content parent. So I modified RecreateFramesForContent and
ContentInserted so RecreateFramesForContent can tell ContentInserted what
parent frame should be used (the parent frame for the old going-away frame).
There was some odd code around the dummy-frame-in-select logic that was
aborting ContentRemoved when the anonymous scrollbar frame was recreated,
because it didn't like to see frames that were direct children of the listbox
frame. I fixed that.

I have *not* yet made listboxes only show a vertical scrollbar when needed.
That's probably not too hard but it's a behavior change that I'm not sure we
want to swallow. Even with it, lazy scrollbar binding should improve pageload a
bit for all comboboxes and listboxes (because they never use their horizontal
scrollbar and comboboxes only use a vertical scrollbar when needed). It should
also improve pageload even for pages without form elements, because we often
don't need the horizontal scrollbar for a page. So I'll ask jrgm to rerun tests
with this patch.

Comment 75

[Boris Zbarsky [:bzbarsky]]

So... I've long been bothered by the fact that "ContentInserted" actually does 
the job of "ConstructFrameForContent".  The two tasks are logically somewhat 
different, as we see here....  Would it make sense to finally move away from 
making ContentInserted and ContentRemoved also serve 
as "ConstructFrameForContent" and "DestroyFrameForContent"?

Comment 76

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

perhaps, but not now...

This ContentInserted is nsIStyleConstructor::ContentInserted ... maybe we should
just rename it? Perhaps during a deCOMtamination/elimination of
nsIStyleConstructor. I'm pretty sure we arent' going to write any other style
constructors :-).

Comment 77

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

By the way, now that smooth scrolling is checked in, this patch makes
listboxes/comboboxes do smooth scrolling when that's enabled.

Comment 78

[jhp (no longer active)]

Robert:

This bug was reported by one of our customers:
When tabbing to a combobox to give it focus, if you then press Alt+DownArrow in
order to view the contents of the list, the box actually drops down from the top
of the combobox, obscuring the dropdown button.  However, by using the F4 key,
the box dops down as expected.  Once F4 has been used, the Alt+DownArrow
combination then begins to behave as expected (ie the same as pressing the F4 key).

Comment 79

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

Javier, that bug exists in the current code. I don't know whether it persists
with this patch since I haven't got a build with this patch in it. Regardless,
it should be filed as a separate bug.

Comment 80

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

jrgm (bless him) says of the new patch:
> So I ran through this with two tests with the patch and two test without
> (baseline pulled late Thursday evening). For the 500MHz/win2k machine where
> I ran the test, the 'with patch' version is equal or slightly faster in overall
> time. [The results I get are in the "0 to 1%, too close to call" region].
> The bugzilla query page still comes in at about 12% slower with the patch,
> although that is an extreme example relative to most real world form pages
> and probably not worth worrying about.

Some slowdown on combobox/listbox heavy pages is inevitable; it's simply more
work to support Mozilla-themed scrollbars.

We could do a little better by not showing vertical scrollbars in listboxes when
they're not needed, but it seems what we have here is good enough.

Comment 81

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 118263 [details] [diff] [review]
new patch


Why is the nsIStyleFrameConstruction change needed?

>Index: layout/html/base/src/nsHTMLContainerFrame.cpp
>+    // XXX roc --- shouldn't we just iterate through all the auxiliary child lists?
>     // Also check the overflow-list
>     aFrame->FirstChild(aPresContext, nsLayoutAtoms::overflowList, &childFrame);
>+    while (childFrame) {
>+      ReparentFrameViewTo(aPresContext, childFrame, aViewManager, aNewParentView, aOldParentView);
>+      childFrame->GetNextSibling(&childFrame);
>+    }
>+
>+    // Also check the popup-list
>+    aFrame->FirstChild(aPresContext, nsLayoutAtoms::popupList, &childFrame);
>     while (childFrame) {
>       ReparentFrameViewTo(aPresContext, childFrame, aViewManager, aNewParentView, aOldParentView);
>       childFrame->GetNextSibling(&childFrame);
>     }

Eh?  Your patch management system seems to have some problems.	(But why not
make this a for loop?)

>Index: layout/html/forms/src/nsComboboxControlFrame.cpp
>@@ -1591,14 +1585,14 @@
> 
>   //Set the desired size for the button and display frame
>   if (NS_UNCONSTRAINEDSIZE == firstPassState.mComputedWidth) {
>     REFLOW_DEBUG_MSG("Unconstrained.....\n");
>     REFLOW_DEBUG_MSG4("*B mItemDisplayWidth %d  dropdownRect.width:%d dropdownRect.w+h %d\n", PX(mItemDisplayWidth), PX(dropdownRect.width), PX((dropBorderPadding.left + dropBorderPadding.right)));
> 
>-    // Start with the dropdown rect's width
>-    mItemDisplayWidth = dropdownRect.width;
>+    // Start with the dropdown rect's width, add room for the button
>+    mItemDisplayWidth = dropdownRect.width + scrollbarWidth;

Is the button necessarily the same width as the scrollbar?  What if there is no
scrollbar?  (Why exactly is this change needed?  Could the comment reflect
that?)

>+  nsresult rv = mDropdownFrame->QueryInterface(NS_GET_IID(nsIScrollableFrame),
>+                                               (void**)&scrollable);
>+  if (NS_FAILED(rv) || !scrollable)
>+    return rv;

QueryInterface should always return NS_NOINTERFACE when there's a null
parameter, so you don't need the null-check.  (And if you did, would you want
to return a success result?)
>+
>+  return scrollable->GetScrollableView(aPresContext, aView);

Comment 82

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 118263 [details] [diff] [review]
new patch

>Index: layout/html/forms/src/nsListControlFrame.cpp
>+  // get it into the coordinates of containerFrame
>+  nsIFrame* frameIterator;
>+  childframe->GetParent(&frameIterator);
>+  while (frameIterator && frameIterator != containerFrame) {
>+    nsRect r;
>+    frameIterator->GetRect(r);
>+    fRect.MoveBy(r.x, r.y);

This could use |GetOrigin| instead of |GetRect|, and perhaps even |nsRect&
nsRect::operator+=(const nsPoint& aPoint)|.

>+    frameIterator->GetParent(&frameIterator);
>   }

Also, perhaps |ancestor| is better than |frameIterator|?


>+NS_IMETHODIMP
>+nsListControlFrame::SaveState(nsIPresContext* aPresContext,
>+                              nsIPresState** aState)

I know you're just moving code, but could you convert all the
nsComponentManager::CreateInstance calls to do_CreateInstance?

> NS_IMETHODIMP
> nsListControlFrame::CaptureMouseEvents(nsIPresContext* aPresContext, PRBool aGrabMouseEvents)
>+    NS_ASSERTION(scrolledFrame, "No scrolled frame found");
>+    if (!scrolledFrame)
>+      return NS_ERROR_FAILURE;

>+    NS_ASSERTION(scrollport, "No scrollport found");
>+    if (!scrollport)
>+      return NS_ERROR_FAILURE;

These could be NS_ENSURE_TRUE.

>+
>+    scrollport->GetView(aPresContext, &view);
>+  }
> 
>+  NS_ASSERTION(view, "no view???");
>   if (view) {

And perhaps this as well, except with NS_OK (unless you want to change the
return value from the current code).

Comment 83

[Boris Zbarsky [:bzbarsky]]

>+  nsresult rv = mDropdownFrame->QueryInterface(NS_GET_IID(nsIScrollableFrame),
>+                                               (void**)&scrollable);

Hey, as long as we're here, make this

nsresult rv = CallQueryInterface(mDropdownFrame, &scrollable);

Comment 84

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 118263 [details] [diff] [review]
new patch

> +  nsStyleContext* scrolledPseudoStyle;

How about moving this down to just before it's used?


Could you remove the |#ifdef DEBUG_dbaron_off| around the assertion in
nsContainerFrame::SetInitialChildList ?

Instead of the magic semantic change to BuildGfxScrollFrame, how about moving
the |if (!aNewFrame)| out to the one current caller and renaming it to
InitGfxScrollFrame?

In RecreateFramesForContent, how about putting the new variable |parent| and
the call to |GetParent| right before the only place you use |parent|.  This
also gets rid of an annoying |else if| that could just be an |if|.

-    PRInt32 indexInContainer;	  
+    PRInt32 indexInContainer = -1;

This shouldn't be necessary, should it, since the variable's only used if
|NS_SUCCEEDED(rv)|, right?

Do you know if the additional parameter to |CreateAnonymousFrames| is really
needed?  Can things be done in a way so that it *never* needs to clear existing
anonymous content because it's already been done?  (Is that true already?)

Comment 85

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

> Why is the nsIStyleFrameConstruction change needed?

From above:
! There were a few other quirky things. Normally frame recreation adds the new
! frame as a child of the primary frame for the content parent, which is wrong
! for the anonymous scrollbars; they need to be attached to the nsGfxScrollFrame
! for the content parent. So I modified RecreateFramesForContent and
! ContentInserted so RecreateFramesForContent can tell ContentInserted what
! parent frame should be used (the parent frame for the old going-away frame).

> Eh?  Your patch management system seems to have some problems.

How so? My attached patch seems to have the right amount of context.

> (But why not make this a for loop?)

Is it worth changing all of them? If so then we should probably do what my XXX
comment says and just iterate through all the child lists.

> Is the button necessarily the same width as the scrollbar?

That's what the code currently assumes elsewhere.

Actually I don't remember exactly why I changed this. I'll have to try removing
it and see what bug I thought I was fixing :-).

> QueryInterface should always return NS_NOINTERFACE when there's a null
> parameter, so you don't need the null-check.

Guess not. Will fix a la bz.

> This could use |GetOrigin| instead of |GetRect|, and perhaps even |nsRect&
> nsRect::operator+=(const nsPoint& aPoint)|.

OK

> Also, perhaps |ancestor| is better than |frameIterator|?

OK

> I know you're just moving code, but could you convert all the
> nsComponentManager::CreateInstance calls to do_CreateInstance?

OK

> These could be NS_ENSURE_TRUE.

OK

> And perhaps this as well, except with NS_OK

OK

> How about moving this down to just before it's used?

OK

> Could you remove the |#ifdef DEBUG_dbaron_off| around the assertion in
> nsContainerFrame::SetInitialChildList ?

OK

> Instead of the magic semantic change to BuildGfxScrollFrame, how about moving
> the |if (!aNewFrame)| out to the one current caller and renaming it to
> InitGfxScrollFrame?

OK

> In RecreateFramesForContent, how about putting the new variable |parent| and
> the call to |GetParent| right before the only place you use |parent|.  This
> also gets rid of an annoying |else if| that could just be an |if|.

OK

> This shouldn't be necessary, should it, since the variable's only used if
> |NS_SUCCEEDED(rv)|, right?

Yeah. I'll remove it.

> Do you know if the additional parameter to |CreateAnonymousFrames| is really
> needed?  Can things be done in a way so that it *never* needs to clear
> existing anonymous content because it's already been done?  (Is that true
> already?)

As far as I can tell, we currently only clear the anonymous content association
when we either tear down the entire document, or when we set the anonymous
content for the associated "real" content to someting else! In fact I think we
must be leaking anonymous content in several situations, e.g., when you destroy
an element that had anonymous content associated with it.

I'd really like to just get rid of this. The anonymous content association is
only used by nsBindingManager to reset the document of anonymous content when
the real content's document is changed. I don't understand XBL but why can
anonymous content even survive a document change? Won't that cause frame
teardown, that will release the only owning refs to the anonymous content, which
will make the anonymous content go away?

Comment 86

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

Attachment: revised patch

Patch addressing dbaron's comments.

The only thing I'm not 100% sure about is the justification for
"mItemDisplayWidth = dropdownRect.width + scrollbarWidth;". I believe my
comment is correct; it's consistent with the other comments about dropdownRect;
and it passes the testcases ... but I haven't unravelled the reflow logic
enough to fully understand why the dropdown frame width never includes the
scrollbar width at that program point.

Comment 87

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

Attachment: incremental reflow test

This tests incremental reflow. It works fine.

Comment 88

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

Comment on attachment 119779 [details] [diff] [review]
revised patch

oops

Comment 89

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

Comment on attachment 119779 [details] [diff] [review]
revised patch

oops

Comment 90

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 119779 [details] [diff] [review]
revised patch

r+sr=dbaron.  I assume you're also going to remove nsScrollFrame.{h,cpp} from
the build and cvs-remove them.

Comment 91

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Actually, that should probably be done in a separate bug, since one would have
to remove a good bit of other code, since we still have native-scrollbars
codepaths in nsCSSFrameConstructor.)

Comment 92

[John Keiser (jkeiser)]

Comment on attachment 119779 [details] [diff] [review]
revised patch

Looks good to me too.

Comment 93

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

Yeah, the code-ripping-out should be done separately. nsScrollingView goes away
too (YAY!) and there are other code paths in views and layout that can be
eliminated (like the path I added NS_ERROR too in this patch).

Comment 94

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

Fixed was checked in. Unfortunately this caused a 1-2% pageload hit on btek and
other tinderboxes; however, it also caused a 1-2% improvement in Txul and Ts.
I'm not planning to back this out unless someone asks. I will look into reducing
the impact further by making listboxes only show their scrollbar if needed.

Comment 95

[Adam D. Moss]

One interesting observation -- smooth-scrolling works for listboxes but not
combo-boxes (as seen on the Bugzilla query page).

Comment 96

[Mike Kaply [:mkaply]]

This is causing major problems printing forms, unfortunately.

Try printing:

http://www.mozilla.org/quality/browser/front-end/testcases/printing/widgets.html

Extra scrollbars appear and other strange things happen.

Comment 97

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

File a new bug against me, please

Comment 98

[Boris Zbarsky [:bzbarsky]]

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