Closed
Bug 363528
Opened 18 years ago
Closed 18 years ago
[FIX][reflow branch] scrollbar overlaps text in Modern theme when shrink-wrapping
Categories
(Core :: Layout, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: MatsPalmgren_bugz, Assigned: bzbarsky)
References
Details
(Keywords: regression, testcase)
Attachments
(5 files, 1 obsolete file)
870 bytes,
text/html
|
Details | |
42.05 KB,
image/png
|
Details | |
42.78 KB,
image/png
|
Details | |
167 bytes,
text/html
|
Details | |
1.29 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
[reflow branch] <select> scrollbar overlap option text in Modern theme. STEPS TO REPRODUCE 1. load attached testcase ACTUAL RESULT The scrollbar overlaps option text. See attached screenshot #1 EXPECTED RESULT The <select> should be wide enough to fit the widest <option> + scrollbar. See attached screenshot #2 PLATFORMS AND BUILDS TESTED Bug occurs in SeaMonkey 2006-12-11-02 on Linux in Modern theme Bug does NOT occur in SeaMonkey 2006-12-11-02 on Linux in Classic theme Bug does NOT occur in SeaMonkey 2006-12-07-01 on Linux in Modern theme Bug does NOT occur in SeaMonkey 2006-12-07-01 on Linux in Classic theme
Reporter | ||
Comment 1•18 years ago
|
||
Reporter | ||
Comment 2•18 years ago
|
||
Reporter | ||
Comment 3•18 years ago
|
||
Comment 4•18 years ago
|
||
The actual results happen in windows (XP Pro SP2) build as well. rv:1.9a1 Gecko/20061209 SeaMonkey/1.5a OS -> All
OS: Linux → All
Assignee | ||
Comment 5•18 years ago
|
||
Does this happen with other scrollframes (e.g. overflow:scroll divs) too?
Reporter | ||
Comment 6•18 years ago
|
||
I checked <textarea> <div> <iframe> <frame> and viewport scrollbars - all of those seems to work correctly, in both themes.
Assignee | ||
Comment 7•18 years ago
|
||
Assignee | ||
Updated•18 years ago
|
Component: Layout: Form Controls → Layout
Flags: blocking1.9?
QA Contact: layout.form-controls → layout
Summary: [reflow branch] <select> scrollbar overlap option text in Modern theme → [reflow branch] scrollbar overlaps text in Modern theme when shrink-wrapping
Assignee | ||
Comment 8•18 years ago
|
||
The problem is that for a native-themed scrollbar we get the right min widget size, but for a non-native-themed one we end up with 6px or something. I wonder whether this should just get fixed in the theme somehow....
Assignee | ||
Comment 9•18 years ago
|
||
Attachment #248812 -
Flags: superreview?(dbaron)
Attachment #248812 -
Flags: review?(neil)
Assignee | ||
Comment 10•18 years ago
|
||
Still need to write a testcase for reftest.
Assignee: nobody → bzbarsky
Flags: in-testsuite?
Priority: -- → P1
Summary: [reflow branch] scrollbar overlaps text in Modern theme when shrink-wrapping → [FIX][reflow branch] scrollbar overlaps text in Modern theme when shrink-wrapping
Target Milestone: --- → mozilla1.9alpha
Why should that change be needed? Shouldn't 'width' and 'height' be enough? (They were enough before, right?)
Comment 12•18 years ago
|
||
Comment on attachment 248812 [details] [diff] [review] This fixes all the testcases for me Fortunately as reviewer I just have to be relieved that all my Bugzilla selects (particularly the r+ one!) are working again ;-)
Attachment #248812 -
Flags: review?(neil) → review+
Assignee | ||
Comment 13•18 years ago
|
||
This change is needed because the new scrollframe code uses GetMinSize on the scrollbar when computing the min-intrinsic-width of the scrollframe. And nsIBox::AddCSSMinSize only looks at the min-width and min-height. And nsSprocketLayout::GetMinSize calls GetMinSize on all the kids. It probably worked before because we were calling GetPrefSize under reflow, not GetMinSize...
So why don't we just make nsHTMLScrollFrame::GetPrefWidth use the scrollbar's pref width? I think using the min width was an accident.
(In fact, I wonder if anything should use the scrollbar's min size. The only other user is the "we don't have room for the scrollbar" check in TryLayout.)
(Well, I can understand the "we don't have room for the scrollbar" check checking vScrollbarMinSize.height and hScrollbarMinSize.width -- that part makes sense, so we do need it for that, at least.)
Comment 17•18 years ago
|
||
Actually even with the patch, selects are still 3px narrower than originally.
Assignee | ||
Comment 18•18 years ago
|
||
Yeah, using the pref width would also probably work. I'll try to test it before I lose network on the 20th, I guess. Neil, the 3px is the difference between the slider width and the thumb width in Modern, I bet.
Assignee | ||
Comment 19•18 years ago
|
||
Attachment #248812 -
Attachment is obsolete: true
Attachment #248963 -
Flags: superreview?(dbaron)
Attachment #248963 -
Flags: review?(dbaron)
Attachment #248812 -
Flags: superreview?(dbaron)
Attachment #248963 -
Flags: superreview?(dbaron)
Attachment #248963 -
Flags: superreview+
Attachment #248963 -
Flags: review?(dbaron)
Attachment #248963 -
Flags: review+
Comment 20•18 years ago
|
||
(In reply to comment #18) >Neil, the 3px is the difference between the slider width and the thumb width in >Modern, I bet. The thumb width is only set for horizontal scrollbars. (The thumb height is set for both vertical and horizontal scrollbars but the slider frame overrides the height of a horizontal thumb.) I investigated the layout in more detail and discovered that in older releases the width of the select did not appear to depend on the theme, although this may have been because the dropmarker was always wider than the scrollbar. Additionally the dropdown was 3px wider than the select which could make it look too wide. With the current code (either patch) the select's width does not appear to depend on the dropmarker width, which makes selects with narrow scrollbars look odd, although maybe this should be fixed by making the dropmarker width the same as the scrollbar width.
Assignee | ||
Comment 21•18 years ago
|
||
The dropmarker is the width that's reported for scrollbars by the device context (calls GetScrollBarDimensions). I suppose we could ask our scrollframe instead; file a bug on this, please?
Assignee | ||
Comment 22•18 years ago
|
||
> suppose we could ask our scrollframe instead Filed bug 364301.
Assignee | ||
Comment 23•18 years ago
|
||
Checked in. I have no idea how to write a regression test for this given that it needs to not run under the default theme...
Flags: blocking1.9?
Assignee | ||
Updated•18 years ago
|
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•