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)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla1.9beta4
People
(Reporter: masa141421356, Assigned: masa141421356)
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
1012 bytes,
text/html
|
Details | |
1.77 KB,
patch
|
roc
:
review+
roc
:
superreview+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•18 years ago
|
||
Assignee | ||
Comment 2•18 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Summary: Scrollbar of a element that contains too large sized font does not work correctly. → ScrollHeight is incorrect when lineHeight is too large.
Assignee | ||
Comment 3•17 years ago
|
||
I tested by changing nscoord(scrollArea.height - fontHeight).
But , this issue is still reproduced.
It seems to caused by other code.
Assignee | ||
Comment 4•17 years ago
|
||
(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.
Assignee | ||
Comment 5•17 years ago
|
||
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
Assignee | ||
Comment 6•17 years ago
|
||
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.
Assignee | ||
Comment 7•17 years ago
|
||
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
Assignee | ||
Comment 8•17 years ago
|
||
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 ?)
Assignee | ||
Comment 9•17 years ago
|
||
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
Assignee | ||
Comment 10•17 years ago
|
||
Assignee | ||
Comment 11•17 years ago
|
||
I filed Bug 412904 for issue of "increment" value.
Assignee | ||
Updated•17 years ago
|
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
Assignee | ||
Updated•17 years ago
|
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
Assignee | ||
Comment 14•17 years ago
|
||
Attachment #297688 -
Attachment is obsolete: true
Attachment #304887 -
Flags: review?(roc)
Attachment #297688 -
Flags: review?(roc)
Assignee | ||
Updated•17 years ago
|
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 16•17 years ago
|
||
Comment on attachment 304887 [details] [diff] [review]
Patch rv.1.1
a=beltzner for 1.9
Attachment #304887 -
Flags: approval1.9? → approval1.9+
Updated•17 years ago
|
Keywords: checkin-needed
Updated•17 years ago
|
Assignee: nobody → masa141421356
Comment 17•17 years ago
|
||
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.
Description
•