Closed Bug 750293 Opened 12 years ago Closed 8 years ago

Right margin (in ltr context) can cause overflow

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
12 Branch
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INCOMPLETE
Tracking Flags:
Tracking Status
firefox13 + fixed
firefox14 + fixed
firefox15 --- affected

People

(Reporter: alterego85, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: regression, Whiteboard: [leave open], [close me 2016-05-04])

Attachments

(6 files)

Minimalish testcase
310 bytes, text/html
Details
fix
7.67 KB, patch
Details | Diff | Splinter Review
Testcase #2, over-constrained rel.pos.
285 bytes, text/html
Details
Testcase #3, over-constrained transform: translate
424 bytes, text/html
Details
branch patch
3.19 KB, patch
roc
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review
test.htm
1.06 KB, text/html
Details 
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

<html>
	<head>
		<title>BugTest Firefox 12</title>
		<style>
			html, body
			{
				font-family:Arial;
				margin:0;
				padding:0;
			}
			
			#content {
			  float: right;
			  overflow: auto;
			}
			.width100 {
			  float: left;
			  margin-left: 1%;
			  margin-right: 1%;
			  width: 98%;
			}
			.zoneWhite_body {
			  background-color: #FFFFFF;
			  border: 1px solid #C6C9D0;
			  color: #000000;
			  font-size: 10pt;
			  margin-bottom: 10px;
			  margin-top: 2px;
			  overflow: auto;
			  padding-bottom: 10px;
			  padding-top: 10px;
			}
			
			.zoneWhite_head {
			  background-color: #CACED6;
			  border: 1px solid #C6C9D0;
			  color: #313545;
			  font-size: 10pt;
			  font-weight: bold;
			  height: 25px;
			  line-height: 25px;
			  text-indent: 10px;
			}
			
		</style>
	</head>
	
	<body>
		<div id="content" style="width:100%">
			<div class="zoneWhite_head width100">
				TEST BUG
			</div>
			<div class="zoneWhite_body width100">
				<br/>
				There is a bug
			</div>
		</div>
	</body>
</html>


Actual results:

There is a scrollbar at the bottom of the "#content" element but my width is 98%(width) + 1%(margin left) + 1%(margin-right) = 100% on the width100 class and no other class is affecting the width... 

This bug appeared with firefox12 that I installed this morning, and it's still there on the nightly firefox 15. I don't have this problem with any other navigator or Firefox 11...


Expected results:

No scroll-bar on the bottom of the "#content" element.
Component: Untriaged → DOM: CSS Object Model
Product: Firefox → Core
QA Contact: untriaged → style-system
Component: DOM: CSS Object Model → Layout
QA Contact: style-system → layout
Keywords: regression
You have a child with the following styles on it:

  margin-left: 1%;
  margin-right: 1%;
  width: 98%;
  border: 1px solid #C6C9D0;

So its total width, including margins, is 100%+2px, which is greater than 100%.

We used to not show a scrollbar for overflowing margins, but now we do, I believe, as do other UAs in some cases.

That said, per spec this is overconstrained and the right margin value should be ignored (which is asymmetric with how vertical margins work), no?  I don't see Opera or WebKit creating overflow here no matter how big I make the right margin.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Element with a 100% width misscalculated (98+1+1) → Right margin (in ltr context) can cause overflow
In my case if I delete the right margin the scrollbar disapears. But why now firefox display scrollbars for an overflowing right margin ? 

None of the other navigator that I can get my hands on does.
Presumably because of bug 665597.  But I don't understand why we did that for horizontal margins.

Note also bug 724789, which seems pretty similar to this bug.

Requesting tracking for the web compat (and imo spec compliance) regression.
Blocks: 665597
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
Depends on: 724789
tracking this - would be good to have someone assigned though, bz we've got two more betas where a fix like this would be ok to land - do you have someone who can put together a patch?
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox13: ? → +
tracking-firefox14: ? → +
tracking-firefox15: ? → ---
Mats is the expert on this stuff; he's already cced.
Assigning to Mats then, now that it's been a week since the last comment and we're one week closer to shipping 13.
Assignee: nobody → matspal
Attached patch fix — — Splinter Review 
GetUsedMargin() reports the wrong value!!!

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=59afb58d95cd
Comment on attachment 623923 [details] [diff] [review]
fix

I believe this patch implements the correct layout per the CSS spec,
but I think it's too high risk for branches.  It needs baking on trunk
for a while to see if it's web compatible.  For example, it creates a
horizontal scrollbar for Testcase #2 and #3.  This is per CSS21 10.3.3
http://www.w3.org/TR/CSS21/visudet.html#blockwidth
which says that for these over-constrained cases the specified right
margin is ignored and instead calculated to satisfy the "width of
containing block" equation.  So for any block that is translated to the
right (like Testcase #2 and #3) will create scrollable overflow with
its right margin.  Note that 'left'/'right' is not a factor in 10.3.3
but instead calculated per 9.4.3:
http://www.w3.org/TR/CSS21/visuren.html#relative-positioning
which says:
"Once a box has been laid out according to the normal flow or floated, it may be shifted relative to this position."

and

"For relatively positioned elements, 'left' and 'right' move the box(es) horizontally, without changing their size. 'Left' moves the boxes to the right, and 'right' moves them to the left. Since boxes are not split or stretched as a result of 'left' or 'right', the used values are always: left = -right."

which I think make it's clear that the margin isn't affected in any way
by the translation.

(otherwise we could have set the right margin to fit the remaining space
at the box's translated position (and thereby not create overflow), but
I think 10.3.3 + 9.4.3 is clear that's not what should happen)

Unfortunately, Testcase #2 and #3 is probably quite common -- all rel.pos.
(or transformed) blocks with a non-auto width are affected.  And to "work
around" it you can't just say "margin-right:0" because that's just ignored
(per 10.3.3), you need "overflow:hidden" on the containing block.

This patch does fix this bug and bug 724789 though, since there's no
translation of the block's position involved.  And getting a correct
used margin out of GetUsedMargin() could help fix other bugs too.
Attached patch branch patch — — Splinter Review 
For Aurora and Beta I think we should just backout the addition of
block margins to scrollable overflow, since the above patch is too
risky IMO.

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=6f1d888de866
Attachment #624292 - Flags: review?(roc)
https://hg.mozilla.org/integration/mozilla-inbound/rev/25995acb6675

Landing the branch patch on trunk for baking and as a temporary
wallpaper until the first patch is ready.

Don't resolve the bug when merging.
Comment on attachment 624292 [details] [diff] [review]
branch patch

[Triage Comment]
Pre-approving for both Aurora and Beta. Please land on or prior to 5/21. Thanks!
Attachment #624292 - Flags: approval-mozilla-beta+
Attachment #624292 - Flags: approval-mozilla-aurora+
The mozilla-beta tree is still closed (blocked on bug 756365 for the past 3 days).
Not sure what I'm supposed to do here...
(In reply to Mats Palmgren [:mats] from comment #17)
> The mozilla-beta tree is still closed (blocked on bug 756365 for the past 3
> days).
> Not sure what I'm supposed to do here...

looking into it. will hopefully get tree open again soon.
(In reply to Lukas Blakk [:lsblakk] from comment #18)
> (In reply to Mats Palmgren [:mats] from comment #17)
> > The mozilla-beta tree is still closed (blocked on bug 756365 for the past 3
> > days).
> > Not sure what I'm supposed to do here...
> 
> looking into it. will hopefully get tree open again soon.

The beta tree is now open.
Blocks: 724789
No longer depends on: 724789
Blocks: 733711
Attached file test.htm — 
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160315153207

Hi reporter,

I have tested your issue on latest FF release (45.0.1) and latest Nightly build and could not reproduce it. I have created a HTML page using the code you provided and the scroll bar does not appear anymore. I've also tested this on Firefox 12 and it did reproduce, but it looks like it got fixed along the way.

Is this still reproducible on your end ? If yes, can you please retest this using latest FF release and latest Nightly build (https://nightly.mozilla.org/) and report back the results ? When doing this, please use a new clean Firefox profile, maybe even safe mode, to eliminate custom settings as a possible cause (https://goo.gl/PNe90E).

Thanks,
Paul.
Flags: needinfo?(alterego85)
Whiteboard: [leave open] → [leave open], [close me 2016-05-04]
Marking this as Resolved: Incomplete due to the lack of response from the reporter.
If anyone can still reproduce it on latest versions, feel free to reopen the issue and provide more information.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(alterego85)
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: