Closed Bug 293504 Opened 20 years ago Closed 20 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: 20 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: