Closed Bug 293504 Opened 19 years ago Closed 19 years ago

margin problems on a certain page

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: bugs.caleb, Assigned: roc)

References

(
URL
)

Details

(Keywords: regression, testcase)

Attachments

(5 files, 1 obsolete file)

Testcase
514 bytes, text/html
Details
modified testcase
894 bytes, text/html
Details
screenshot
3.83 KB, image/png
Details
further reduced
156 bytes, text/html
Details
fix #2
7.76 KB, patch
dbaron
: review+
dbaron
: superreview+
chofmann
: approval1.8b3+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050509 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050509 Firefox/1.0+

The checkin for bug 292370 seemed to cause a regression that messed up the
layout on http://www.steampowered.com.

The navigation images on top have some spaces between them that should not be
there and the middle box with the content seems to have a glitch on the right
hand side.

Testcase coming up soon.

Reproducible: Always
Blocks: 292370
Keywords: regression
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050508
Firefox/1.0+

confirming

regressed between between 20050503 13:25pdt and 14:50pdt

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-05-03+14%3A03%3A00&maxdate=2005-05-03+15%3A25%3A00&cvsroot=%2Fcvsroot

cc roc
Status: UNCONFIRMED → NEW
Ever confirmed: true
need a reduced testcase here
http://validator.w3.org/check?uri=http%3A%2F%2Fwww.steampowered.com%2F
about 125 errors, mostly like:
Line 74, column 75: element "SPACER" undefined
...<spacer type="block" width="1" height="36"></td>

http://jigsaw.w3.org/css-validator/validator?uri=http%3A%2F%2Fwww.steampowered.com%2Fsteampowered03.css&usermedium=all
Attached file Testcase — 
I think this is the initial cause of why the page looks different now, which
makes it a bug in current trunk builds.
Keywords: testcase
Attached file modified testcase — 
the div presents itself a scrollbar wider than it should
click to change the style.overflow
Attached image screenshot — 
the up arrow was manually added to prove the increase in width=scrollbarwidth
some addintinal notes:
1.for <html dir="rtl"> the behaviour is the same.
2.if style.margin-right is added to the outer <div> the margin-right value is
ADDED to the width of the scrollbar.

because this easily breaks layouts -> ?1.8b2
Flags: blocking1.8b2?
Flags: blocking1.8b3?
Flags: blocking1.8b2?
Flags: blocking1.8b2-
*** Bug 295814 has been marked as a duplicate of this bug. ***
Blocks: 294264
*** Bug 296968 has been marked as a duplicate of this bug. ***
Assignee: nobody → roc
Attached file further reduced — 
It's really nothing to do with padding ... it's just the vertical scrollbar in
the table that's messing things up.
Attached patch fix (obsolete) — — Splinter Review 
Okay, so the problem is in MEW calculation, of course. We do this rather weird
thing (which Hixie assured me is correct) where if an element with a vertical
scrollbar has a CSS-specified content width, then the width of the scrollbar is
subtracted from the used content width. In implementation, we add the vertical
scrollbar width to the scrolled child's desired width, and then override that
with the CSS computed width (if there is one) and add padding to determine the
inside-borders-width.

So in the MEW we need to do something similar ... use the vertical scrollbar
width as the basic min-intrinsic-width and then apply styles to override that,
then add padding. Making this consistent fixes the bug.
Attachment #186434 - Flags: superreview?(dbaron)
Attachment #186434 - Flags: review?(dbaron)
Attachment #186434 - Flags: superreview?(dbaron)
Attachment #186434 - Flags: superreview+
Attachment #186434 - Flags: review?(dbaron)
Attachment #186434 - Flags: review+
Comment on attachment 186434 [details] [diff] [review]
fix

Requesting approval
Attachment #186434 - Flags: approval1.8b3?
I've tried this fix, it works on initial load, but when dynamically changing the
overflow property, I still get the bug, see "modified testcase" (=
https://bugzilla.mozilla.org/attachment.cgi?id=183447 )
Underneath the div there you can see these lines:
style overflow:hidden
style overflow:scroll
style overflow:auto
You can click on them to change from overflow propery, toggling to overflow:auto
or oveflow:scroll still shows the bug for me.
Attached patch fix #2 — — Splinter Review 
This extends the previous patch to do the same thing for maximum-width
computation.

David, I'm a bit worried that maximum-width computation is setting
mComputedWidth to unconstrained. Do you have any insight into whether that is
right or wrong? It was added by troy long ago...
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/generic&command=DIFF_FRAMESET&file=nsBlockReflowContext.cpp&rev2=1.51&rev1=1.50
Attachment #186434 - Attachment is obsolete: true
Attachment #186515 - Flags: superreview?(dbaron)
Attachment #186515 - Flags: review?(dbaron)
Comment on attachment 186515 [details] [diff] [review]
fix #2

>Index: layout/generic/nsGfxScrollFrame.cpp
>       nscoord kidContentMaxWidth = kidMaxWidth -
>-        aState->mReflowState.mComputedPadding.LeftRight();
>+        aState->mReflowState.mComputedPadding.LeftRight() + vScrollbarActualWidth;

It might be good to keep the + terms before the - terms (a little less
confusing).

r+sr=dbaron
Attachment #186515 - Flags: superreview?(dbaron)
Attachment #186515 - Flags: superreview+
Attachment #186515 - Flags: review?(dbaron)
Attachment #186515 - Flags: review+
Comment on attachment 186515 [details] [diff] [review]
fix #2

fixes significant layout regression
Attachment #186515 - Flags: approval1.8b3?
Comment on attachment 186515 [details] [diff] [review]
fix #2

a=chofmann
Attachment #186515 - Flags: approval1.8b3? → approval1.8b3+
checked in
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Could this have caused a small (1% or so) Tdhtml hit on luna?  It's the only
thing that seems to be in the relevant regression range...
Stephen: can you verify by backout?

Boris: I don't know. I wouldn't have thought so, but of course it's possible.
(In reply to comment #21)
> Stephen: can you verify by backout?

No build tree here, sorry...

OK, I finally got a chance to read the patch, and I agree that it should have no
Tdhtml impact.
Flags: blocking1.8b3?
Depends on: 355413
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: