Closed Bug 383267 Opened 18 years ago Closed 17 years ago

When fontHeight is very large, pagescroll height(pageincrement value) is too small

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: masa141421356, Assigned: masa141421356)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; ja; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a6pre) Gecko/20070604 When a element that has a vertical scrollbar (i.e, <div style="overflow:auto;">), and that contains LARGE font-sized text, clickink background of scrollbar does not works correctly. Reproducible: Always Steps to Reproduce: 1.Display testcase 2.Click background of scrollbar 3. Actual Results: Scroll invertly or not scroll. (It depends on font size and height of div element). If height of div element = 1em, it does not scroll. If height of div element < 1em, it scrolls invertly. Expected Results: When clicking background between top of scorollbar and slider, it should scroll toward top of its content. When clicking background between bottom of scorollbar and slider, it should scroll toward bottom of its content. This bug is also be reproducable at Fx2.0.0.4 at Windows2000. Vertical scorlling size seems to be (element_height - 1em). I think it is good when element_height >> 1em. But I think, it SHOULD NOT be smaller than 1px. I think , minimum-vertical-scorlling-size is needed. I have some ideas for it. (1)1em , 0.5em , etc (XX percent of font size) (2)49% of element_height etc. (XX percent of element-height)
Attached file Testcase
I think this bug is caused by http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsGfxScrollFrame.cpp&rev=3.307&root=/cvsroot#2357 It uses nscoord(scrollArea.height - fontHeight) without checking its value.
Summary: Scrollbar of a element that contains too large sized font does not work correctly. → ScrollHeight is incorrect when lineHeight is too large.
I tested by changing nscoord(scrollArea.height - fontHeight). But , this issue is still reproduced. It seems to caused by other code.
(In reply to comment #3) > I tested by changing nscoord(scrollArea.height - fontHeight). > But , this issue is still reproduced. > It seems to caused by other code. > Oops, It is my mistake.
I think ,scrolling height when clicking button of scrollbar is too large (It is now same as fontHeight. But I think , when fontHeight is too large is should be limited,).
OS: Windows 2000 → All
Hardware: PC → All
Summary: ScrollHeight is incorrect when lineHeight is too large. → When fontHeight is too large, pageincrement is too small, and increment is too large in vertical scroll bar
I wrote patch to fix this. Please confirm this bug. (In reply to comment #5) > I think ,scrolling height when clicking button of scrollbar is too large (It is > now same as fontHeight. But I think , when fontHeight is too large is should be > limited,). > Oops, I want to say is think ,scrolling height when clicking button of scrollbar is too large if fontHeight is very large. Because It is scrolling height is same as fontHeight.
Issue about "pageincrement" value is regression of Bug 21348. Issue obout "increment" value (scrolling height when clicking button of scrollbar ) is exist on nsGfxScrollFrame.cpp (3.1).
Component: Layout → XP Toolkit/Widgets
Keywords: regression
At first , pageincrement value was nscoord(float(scrollAreaSize.height)*0.8)), After fix of Bug 21348, it is nscoord(scrollAreaSize.height - fontHeight). I think largest valu in them should be used. And, I think "increment" value should smaller than "pageincrement" value. (or smaller than scrollAreaSize.height*0.5 ?)
I think is is needed to discuss about good maximum valu of "increment". I'll file it as another bug.
Summary: When fontHeight is too large, pageincrement is too small, and increment is too large in vertical scroll bar → When fontHeight is very large page, pagescroll height(pagecrement value) is too small
Attached patch Patch rv1.0 for Trunk (obsolete) — Splinter Review
I filed Bug 412904 for issue of "increment" value.
Summary: When fontHeight is very large page, pagescroll height(pagecrement value) is too small → When fontHeight is very large, pagescroll height(pageincrement value) is too small
Attachment #297688 - Flags: review?(jonas)
Comment on attachment 297688 [details] [diff] [review] Patch rv1.0 for Trunk I'm not the right person to review this.
Attachment #297688 - Flags: review?(jonas) → review?(roc)
+ pageincrement > pageincrementMin ? pageincrement + : pageincrementMin, Use PR_MAX. Add a comment explaining why you're doing this. Other than that, nice patch!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Patch rv.1.1Splinter Review
Attachment #297688 - Attachment is obsolete: true
Attachment #304887 - Flags: review?(roc)
Attachment #297688 - Flags: review?(roc)
Attachment #304887 - Attachment is patch: true
Comment on attachment 304887 [details] [diff] [review] Patch rv.1.1 excellent, thank you. release drivers: this is not a blocker, but it's a very low-risk fix from a contributor that will fix keyboard scrolling in some situations where it's currently unusable.
Attachment #304887 - Flags: superreview+
Attachment #304887 - Flags: review?(roc)
Attachment #304887 - Flags: review+
Attachment #304887 - Flags: approval1.9?
Comment on attachment 304887 [details] [diff] [review] Patch rv.1.1 a=beltzner for 1.9
Attachment #304887 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Assignee: nobody → masa141421356
Checking in layout/generic/nsGfxScrollFrame.cpp; /cvsroot/mozilla/layout/generic/nsGfxScrollFrame.cpp,v <-- nsGfxScrollFrame.cpp new revision: 3.333; previous revision: 3.332 done
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: