Scrollbars overpaint divs

RESOLVED FIXED in mozilla1.4alpha

Status

()

Core
Layout: Form Controls
P3
normal
RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: mkaply, Assigned: roc)

Tracking

({testcase, topembed+})

Trunk
mozilla1.4alpha
x86
All
testcase, topembed+
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.3b -
blocking1.4a -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fix], URL)

Attachments

(5 attachments, 16 obsolete attachments)

15.84 KB, image/gif
Details
45.36 KB, text/html
Details
7.45 KB, text/html
Details
108.00 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
641 bytes, text/html
Details
(Reporter)

Description

16 years ago
Go to the above testcase.

Click on Action, then Print.

Notice the scrollbar for the listbox overpaints the menu.

Comment 1

16 years ago
When I print, I don't see the menu and I don't see the scrollbars printing over top.
Status: NEW → ASSIGNED
Summary: Scrollbars overpaint divs → [WFM]Scrollbars overpaint divs
Target Milestone: --- → mozilla1.0
(Reporter)

Comment 2

16 years ago
Created attachment 70528 [details]
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?
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
Priority: -- → P2

Updated

16 years ago
Priority: P2 → P3
Target Milestone: mozilla1.0 → Future
(Reporter)

Comment 4

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

Updated

16 years ago
QA Contact: madhur → tpreston
(Reporter)

Comment 5

16 years ago
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.
Summary: [WFM]Scrollbars overpaint divs → Scrollbars overpaint divs
Created attachment 83580 [details] [diff] [review]
patch to use nsGfxListControlFrame
Created attachment 83581 [details]
layout/html/forms/src/nsGfxListControlFrame.cpp
Created attachment 83582 [details]
layout/html/forms/src/nsGfxListControlFrame.h

Updated

16 years ago
Attachment #83581 - Attachment description: nsGfxListControlFrame.cpp → layout/html/forms/src/nsGfxListControlFrame.cpp
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.

Updated

16 years ago
Attachment #83582 - Attachment description: nsGfxListControlFrame.h → layout/html/forms/src/nsGfxListControlFrame.h
(Reporter)

Comment 10

15 years ago
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.
Created attachment 85468 [details] [diff] [review]
patch to use nsGfxListControlFrame for Listboxes only
Attachment #83580 - Attachment is obsolete: true
Created attachment 85469 [details]
layout/html/forms/src/nsGfxListControlFrame.cpp
Attachment #83581 - Attachment is obsolete: true
Created attachment 85470 [details]
layout/html/forms/src/nsGfxListControlFrame.h
Attachment #83582 - Attachment is obsolete: true
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

15 years ago
Created attachment 86286 [details] [diff] [review]
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
Attachment #85468 - Attachment is obsolete: true

Updated

15 years ago
Attachment #85469 - Attachment is obsolete: true
Attachment #85470 - Attachment is obsolete: true

Comment 16

15 years ago
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.
Created attachment 90823 [details] [diff] [review]
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.
Attachment #86286 - Attachment is obsolete: true
*** Bug 86736 has been marked as a duplicate of this bug. ***
*** Bug 166119 has been marked as a duplicate of this bug. ***
*** Bug 175205 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 21

15 years ago
So are XBL form controls completely dead at this point?

Comment 22

15 years ago
It's a good question for Bryner
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.
(Reporter)

Comment 24

15 years ago
OK, what about the possibility of taking this patch Javier created that switches 
Mozilla to use non native scrollbars?
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.

Updated

15 years ago
Keywords: nsbeta1, topembed

Updated

15 years ago
Target Milestone: Future → mozilla1.3alpha

Comment 26

15 years ago
Marking topembed+ as part of topembed triage
Keywords: topembed → topembed+
-> jkeiser
Assignee: rods → jkeiser
Status: ASSIGNED → NEW

Updated

15 years ago
Keywords: nsbeta1 → nsbeta1+

Updated

15 years ago
Keywords: testcase

Comment 28

15 years ago
Created attachment 109667 [details]
Test Case
(Reporter)

Comment 29

15 years ago
This bug is VERY VERY bad.

We have put a fix together in this bug.

Is anyone interested in this?

Nominating for 1.3b
Flags: blocking1.3b?

Updated

15 years ago
Target Milestone: mozilla1.3alpha → mozilla1.4alpha
(Reporter)

Comment 30

15 years ago
There is a patch. In the bug.

Why is this moved out two releases?

This bug makes DHTML SUCK hard on Mozilla.

Updated

15 years ago
Attachment #90823 - Flags: superreview?(bzbarsky)
Attachment #90823 - Flags: review?(jkeiser)
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?
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 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.
Attachment #90823 - Flags: review?(jkeiser) → review-
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.
Status: NEW → ASSIGNED
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.
(Reporter)

Comment 36

15 years ago
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 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?
*** Bug 175700 has been marked as a duplicate of this bug. ***
Boris, we would need this for 1.3.
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.
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...
Created attachment 111655 [details] [diff] [review]
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.
Attachment #90823 - Attachment is obsolete: true
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.
*** Bug 190497 has been marked as a duplicate of this bug. ***
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.
Flags: blocking1.3b? → blocking1.3b-
Created attachment 113523 [details] [diff] [review]
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.
Attachment #111655 - Attachment is obsolete: true
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>.
Created attachment 114452 [details] [diff] [review]
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.
Attachment #113523 - Attachment is obsolete: true
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.
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?
Attachment #90823 - Flags: superreview?(bzbarsky)
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.
Depends on: 194240
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.
Created attachment 115297 [details] [diff] [review]
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.
Attachment #114452 - Attachment is obsolete: true
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...
I'm taking this over
Assignee: jkeiser → roc+moz
Status: ASSIGNED → NEW
*** Bug 194748 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

15 years ago
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.
Created attachment 116830 [details] [diff] [review]
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.
Attachment #115297 - Attachment is obsolete: true
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).
Status: NEW → ASSIGNED
Flags: blocking1.4a?
Whiteboard: [fix]
Attachment #116830 - Flags: superreview?(bryner)
Attachment #116830 - Flags: review?(dbaron)
Created attachment 116831 [details]
testcase

Here's a testcase I was using.
Created attachment 117200 [details] [diff] [review]
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
Attachment #116830 - Attachment is obsolete: true
Attachment #116830 - Flags: superreview?(bryner)
Attachment #116830 - Flags: review?(dbaron)
Attachment #117200 - Flags: superreview?(dbaron)
Attachment #117200 - Flags: review?(bryner)
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.
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.
(Reporter)

Comment 64

15 years ago
Should the scrollbar go away when it isn't needed?
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 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
> 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.
> I'd prefer NS_WARNING() to NS_ASSERTION(PR_FALSE, ...)

Or NS_ERROR() if you really want the assert.
Mousewheel was horked, optgroups worked. New patch coming momentarily.
Created attachment 117315 [details] [diff] [review]
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.
Attachment #117200 - Attachment is obsolete: true
Attachment #117200 - Flags: superreview?(dbaron)
Attachment #117200 - Flags: review?(bryner)
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 :-(.
(Reporter)

Comment 72

15 years ago
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.
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.

Updated

15 years ago
Flags: blocking1.4a? → blocking1.4a-
Created attachment 118263 [details] [diff] [review]
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.
Attachment #117315 - Attachment is obsolete: true
Attachment #118263 - Flags: superreview?(dbaron)
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"?
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 :-).
By the way, now that smooth scrolling is checked in, this patch makes
listboxes/comboboxes do smooth scrolling when that's enabled.
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).
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.
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.
Attachment #118263 - Flags: review?(dbaron)
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 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).
>+  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 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?)
> 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?
Created attachment 119779 [details] [diff] [review]
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.
Attachment #118263 - Attachment is obsolete: true
Created attachment 119780 [details]
incremental reflow test

This tests incremental reflow. It works fine.
Attachment #119779 - Flags: superreview+
Attachment #119779 - Flags: review+
Comment on attachment 119779 [details] [diff] [review]
revised patch

oops
Attachment #119779 - Flags: superreview?
Attachment #119779 - Flags: review?
Comment on attachment 119779 [details] [diff] [review]
revised patch

oops
Attachment #119779 - Flags: superreview?(dbaron)
Attachment #119779 - Flags: review?(dbaron)
Attachment #119779 - Flags: superreview?
Attachment #119779 - Flags: superreview+
Attachment #119779 - Flags: review?
Attachment #119779 - Flags: review+
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.
Attachment #119779 - Flags: superreview?(dbaron)
Attachment #119779 - Flags: superreview+
Attachment #119779 - Flags: review?(dbaron)
Attachment #119779 - Flags: review+
(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 on attachment 119779 [details] [diff] [review]
revised patch

Looks good to me too.
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).
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 95

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

Comment 96

15 years ago
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.
File a new bug against me, please
Attachment #118263 - Flags: superreview?(dbaron)
Attachment #118263 - Flags: review?(dbaron)

Updated

15 years ago
Depends on: 201767
*** Bug 86263 has been marked as a duplicate of this bug. ***
Depends on: 234468
You need to log in before you can comment on or make changes to this bug.