Last Comment Bug 750293 - Right margin (in ltr context) can cause overflow
: Right margin (in ltr context) can cause overflow
Status: RESOLVED INCOMPLETE
[leave open], [close me 2016-05-04]
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 12 Branch
: x86 Windows 7
: -- normal (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
:
Mentors:
Depends on:
Blocks: 665597 724789 733711
  Show dependency treegraph
 
Reported: 2012-04-30 08:51 PDT by Pierre SERIN
Modified: 2016-05-10 04:20 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
affected


Attachments
Minimalish testcase (310 bytes, text/html)
2012-05-02 08:14 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
fix (7.67 KB, patch)
2012-05-14 20:13 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Testcase #2, over-constrained rel.pos. (285 bytes, text/html)
2012-05-15 19:00 PDT, Mats Palmgren (:mats)
no flags Details
Testcase #3, over-constrained transform: translate (424 bytes, text/html)
2012-05-15 19:04 PDT, Mats Palmgren (:mats)
no flags Details
branch patch (3.19 KB, patch)
2012-05-15 20:29 PDT, Mats Palmgren (:mats)
roc: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
test.htm (1.06 KB, text/html)
2016-04-04 07:26 PDT, Paul Pasca[Away. Please needinfo? Paul Oiegas [:pauloiegasSV]]
no flags Details

Description Pierre SERIN 2012-04-30 08:51:08 PDT
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.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-05-02 08:14:04 PDT
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.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-05-02 08:14:39 PDT
Created attachment 620319 [details]
Minimalish testcase
Comment 3 Pierre SERIN 2012-05-03 01:25:21 PDT
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.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-05-03 10:49:45 PDT
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.
Comment 5 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-03 12:10:39 PDT
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?
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-05-03 12:25:55 PDT
Mats is the expert on this stuff; he's already cced.
Comment 7 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-09 17:01:15 PDT
Assigning to Mats then, now that it's been a week since the last comment and we're one week closer to shipping 13.
Comment 8 Mats Palmgren (:mats) 2012-05-14 20:13:33 PDT
Created attachment 623923 [details] [diff] [review]
fix

GetUsedMargin() reports the wrong value!!!

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=59afb58d95cd
Comment 9 Mats Palmgren (:mats) 2012-05-15 19:00:55 PDT
Created attachment 624272 [details]
Testcase #2, over-constrained rel.pos.
Comment 10 Mats Palmgren (:mats) 2012-05-15 19:04:22 PDT
Created attachment 624274 [details]
Testcase #3, over-constrained transform: translate
Comment 11 Mats Palmgren (:mats) 2012-05-15 20:23:19 PDT
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.
Comment 12 Mats Palmgren (:mats) 2012-05-15 20:29:09 PDT
Created attachment 624292 [details] [diff] [review]
branch patch

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
Comment 13 Mats Palmgren (:mats) 2012-05-16 16:12:39 PDT
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 14 Ed Morley [:emorley] 2012-05-17 03:17:23 PDT
https://hg.mozilla.org/mozilla-central/rev/25995acb6675
Comment 15 Alex Keybl [:akeybl] 2012-05-18 10:37:40 PDT
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!
Comment 16 Mats Palmgren (:mats) 2012-05-20 21:38:31 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/31e0f582ea19
waiting for mozilla-beta tree to open...
Comment 17 Mats Palmgren (:mats) 2012-05-21 08:41:46 PDT
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...
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-21 11:50:40 PDT
(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.
Comment 19 Alex Keybl [:akeybl] 2012-05-21 15:48:04 PDT
(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.
Comment 20 Mats Palmgren (:mats) 2012-05-21 23:14:13 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/8072115a9e89
Comment 21 Paul Pasca[Away. Please needinfo? Paul Oiegas [:pauloiegasSV]] 2016-04-04 07:26:07 PDT
Created attachment 8737790 [details]
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.
Comment 22 Paul Pasca[Away. Please needinfo? Paul Oiegas [:pauloiegasSV]] 2016-05-10 04:20:56 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.