Closed
Bug 293504
Opened 19 years ago
Closed 19 years ago
margin problems on a certain page
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bugs.caleb, Assigned: roc)
References
()
Details
(Keywords: regression, testcase)
Attachments
(5 files, 1 obsolete file)
514 bytes,
text/html
|
Details | |
894 bytes,
text/html
|
Details | |
3.83 KB,
image/png
|
Details | |
156 bytes,
text/html
|
Details | |
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
Comment 1•19 years ago
|
||
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
Assignee | ||
Comment 2•19 years ago
|
||
need a reduced testcase here
Comment 3•19 years ago
|
||
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
Comment 4•19 years ago
|
||
I think this is the initial cause of why the page looks different now, which makes it a bug in current trunk builds.
Comment 5•19 years ago
|
||
the div presents itself a scrollbar wider than it should click to change the style.overflow
Comment 6•19 years ago
|
||
the up arrow was manually added to prove the increase in width=scrollbarwidth
Comment 7•19 years ago
|
||
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?
Updated•19 years ago
|
Flags: blocking1.8b3?
Flags: blocking1.8b2?
Flags: blocking1.8b2-
Comment 8•19 years ago
|
||
*** Bug 295814 has been marked as a duplicate of this bug. ***
Comment 9•19 years ago
|
||
*** Bug 296968 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Assignee: nobody → roc
Assignee | ||
Comment 10•19 years ago
|
||
It's really nothing to do with padding ... it's just the vertical scrollbar in the table that's messing things up.
Assignee | ||
Comment 11•19 years ago
|
||
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+
Reporter | ||
Comment 12•19 years ago
|
||
Comment on attachment 186434 [details] [diff] [review] fix Requesting approval
Attachment #186434 -
Flags: approval1.8b3?
Comment 13•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #186434 -
Flags: approval1.8b3?
Assignee | ||
Comment 14•19 years ago
|
||
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+
Assignee | ||
Comment 16•19 years ago
|
||
Comment on attachment 186515 [details] [diff] [review] fix #2 fixes significant layout regression
Attachment #186515 -
Flags: approval1.8b3?
Comment 17•19 years ago
|
||
Comment on attachment 186515 [details] [diff] [review] fix #2 a=chofmann
Attachment #186515 -
Flags: approval1.8b3? → approval1.8b3+
Assignee | ||
Comment 18•19 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 19•19 years ago
|
||
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...
Might it have caused bug 299467, too?
Assignee | ||
Comment 21•19 years ago
|
||
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...
Comment 23•19 years ago
|
||
OK, I finally got a chance to read the patch, and I agree that it should have no Tdhtml impact.
Updated•19 years ago
|
Flags: blocking1.8b3?
You need to log in
before you can comment on or make changes to this bug.
Description
•