812899 - absolutely positioned element should be vertically centered even if the height is bigger than that of the containing block

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Kang-Hao (Kenny) Lu [:kennyluck]]

http://hg.mozilla.org/mozilla-central/annotate/0ef0c9db3b2b/layout/generic/nsHTMLReflowState.cpp#l1548

Note that the spec is ok clear about this and there's already FIXME in the comment of the above file. All IE10, Opera12, WebKit follow the spec. See also bug 419100 for the relevant test case.

Comment 1

[Po-chiang Chao [:bobchao]]

I'll try to take this

Comment 2

[Kang-Hao (Kenny) Lu [:kennyluck]]

This is the relevant spec text in CSS 2.1 10.6.4[1]:

"If none of the three ('top', 'bottom', 'height') are 'auto': If both 'margin-top' and 'margin-bottom' are 'auto', solve the equation under the extra constraint that the two margins get equal values." 

as compared to

"If none of the three ('left', 'right', 'width') is 'auto': If both 'margin-left' and 'margin-right' are 'auto', solve the equation under the extra constraint that the two margins get equal values, unless this would make them negative, in which case when direction of the containing block is 'ltr' ('rtl'), set 'margin-left' ('margin-right') to zero and solve for 'margin-right' ('margin-left')." in CSS 2.1 10.3.7. 

[1] http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-height

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Kang-Hao (Kenny) Lu [:kennyluck] from comment #0)
> http://hg.mozilla.org/mozilla-central/annotate/0ef0c9db3b2b/layout/generic/
> nsHTMLReflowState.cpp#l1548
> 
> Note that the spec is ok clear about this and there's already FIXME in the
> comment of the above file. All IE10, Opera12, WebKit follow the spec. See
> also bug 419100 for the relevant test case.

Do they also follow the spec for the negative case for horizontal margins?  I tend to think this is probably a mistake in the spec.

Comment 4

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to David Baron [:dbaron] from comment #3)
> Do they also follow the spec for the negative case for horizontal margins? 

Yes.

> I tend to think this is probably a mistake in the spec.

Maybe, but this is vaguely explainable. For horizontal margins, you probably want to always ignore 'margin-right' so that it doesn't overflow to the left, but for vertical margins, unless the element is at the top, there's no such concern.

This is pure guessing though. I know CSS 2.1 has all bunch of oddness.

Comment 5

[Mina Almasry]

Hi is this bug still open? I am new here and I would like to take a shot at it.

While I have your attention, any pointers to where I should start with this? Confirm I should look at this file no?

http://hg.mozilla.org/mozilla-central/annotate/0ef0c9db3b2b/layout/generic/nsHTMLReflowState.cpp#l1548

Thanks,

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Bob Chao, are you still working on this bug? Can you let us know in a comment? Thanks!

Comment 7

[Po-chiang Chao [:bobchao]]

Sorry I didn't notice that we have new comments.

Actually I have finished the work (only a few line of code modified) but haven't write a new test about it. Will do it.

Thanks!

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Perhaps you could post the patch so that somebody else could write the tests?

Comment 9

[mjh563]

What tests are needed for this? Would it be enough to write a single test for this specific case (non-auto top, bottom and height, auto margin-top and margin-bottom, available margin space < 0)?

Comment 10

[Sathyanarayanan Gunasekaran [:gsathya]]

Attachment: 812899.patch

Hey. New contributor here. Not sure if this bug is still valid, but I've attached a patch. Review would be much appreciated. Thanks!

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8365592 [details] [diff] [review]
812899.patch

You should have set the review flag to ? to request review (rather than + to grant it).

The code change looks good here.  But could you make the commit message more specific?  Rather than:
Bug 812899- fix behavior of absolutely positioned elements according to spec
you should probably say
Make vertical 'auto' margins on absolutely positioned elements always center, even when the margins are negative.

Also, can you add a reftest?  It should probably go in layout/reftests/abs-pos/ (it needs to be listed in reftest.list).  See:
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt
https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #11)
> The code change looks good here.  But could you make the commit message more
> specific?  Rather than:
> Bug 812899- fix behavior of absolutely positioned elements according to spec
> you should probably say
> Make vertical 'auto' margins on absolutely positioned elements always
> center, even when the margins are negative.

(And the new message should still have the bug number; sorry for leaving it off.)

Comment 13

[Mike Hoye [:mhoye]]

gsathya - You're pretty close to getting this done, are you still working on it?

Comment 14

[Karl Dubost💡 :karlcow]

gsathya at mozilla is not valid anymore. I guess the person has left.
Maybe it's time to finish it up. What's the process usually at this stage?

Comment 15

[Karl Dubost💡 :karlcow]

Attachment: auto-centering.png

Left: Blink
Right: Gecko


That code doesn't exist anymore at all. see the current 
https://hg.mozilla.org/mozilla-central/file/ce863f9d8864/layout/generic/nsHTMLReflowState.cpp
So the patch is obsolete.

The bug on the other hand still exists.
The Markup in the URL is different in between blink and gecko (latest versions)

Comment 16

[Karl Dubost💡 :karlcow]

Code with new variable names is at 
 https://hg.mozilla.org/mozilla-central/file/ce863f9d8864/layout/generic/nsHTMLReflowState.cpp#l1720

Comment 17

[Karl Dubost💡 :karlcow]

Attachment: bug-812899.patch

This patch 

* Updated for the new code
* Contain two files: a ref and the test
  layout/reftests/abs-pos/abs-pos-auto-margin-centered.html
  layout/reftests/abs-pos/abs-pos-auto-margin-centered-ref.html
* added the test to reftest.list
  layout/reftests/abs-pos/reftest.list

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8622921 [details] [diff] [review]
bug-812899.patch

Please don't add a blank line at the end of nsHTMLReflowState.cpp.
(The diff should only show one chunk of changes.)

reftest.list:
>+== abs-pos-auto-margin-centered.html abs-pos-auto-margin-centered-ref.html # bug 812899

Don't put the bug number there; that's how we annotate failures.

If you want to include the bug number somewhere, put it in the <title>
of the test.


Also, to make the reference a little more reference-like, please use
"margin: -23px auto" instead of "margin: auto auto -23px", which depends
on auto margin-top.

And in both test and reference, please add an explicit font-size: 16px
since you depend on that.

r=dbaron with those changes

Comment 19

[Karl Dubost💡 :karlcow]

Attachment: bug-812899-v2.patch

fixed the attachment according to Comment #18

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5ea84a0a779

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out for reftest failures on at least Mulet. Please verify that this is green on Try before re-requesting checkin.
https://treeherder.mozilla.org/logviewer.html#?job_id=11831552&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/65c3a87e04fb

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Pretty sure this is also yours: https://treeherder.mozilla.org/logviewer.html#?job_id=11838318&repo=mozilla-inbound

Comment 23

[Karl Dubost💡 :karlcow]

hmmm Thanks Ryan, I will investigate.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Karl, any luck on the investigation?

Comment 26

[Karl Dubost💡 :karlcow]

Boris,

Rha stupid me. 
Yes found out what was failing. I had forgotten a semi colon in one of my tests. 

https://hg.mozilla.org/integration/mozilla-inbound/diff/a5ea84a0a779/layout/reftests/abs-pos/abs-pos-auto-margin-centered-ref.html#l1.20

I fixed the test in my local repo. 
Once the comma is fixed this test passes locally.

I guess I need help here.


Ryan could you point me to the right URI on the wiki to do that? I'm using hg and patches queue.
> "Please verify that this is green on Try before re-requesting checkin."

I found TryChooser Syntax Builder
but then not sure what to do with the syntax I created :)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Karl,

If you have try push access, what you want to basically do is, in the tree that already has your patch qpushed:

  hg qnew -m "your-try-syntax-here" try-my-patch
  hg push -f ssh://[e-mail]/try

If you don't have such access, just attach your updated patch here and I'll push it to try for you.

Comment 28

[Karl Dubost💡 :karlcow]

Attachment: bug812899v3.patch

Boris, 

This is the new patch for Bug 812899. Could you try-push for me?

Thanks.

Comment 29

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0244ebd98eb9

Comment 30

[Karl Dubost💡 :karlcow]

Thanks Brian for pushing it. The build has completed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0244ebd98eb9

I need to learn to read this information now. 
But I don't see anything wrong in the list there. On the other hand, I noticed there is a reftest.list in the abs-pos/ directory and I didn't add to it the test + its ref. I wonder if it should be done or if it's used for something else.

Comment 31

[Karl Dubost💡 :karlcow]

Attachment: patch-bug812899-visual.jpg

Visual results of my patch.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So https://treeherder.mozilla.org/#/jobs?repo=try&revision=0244ebd98eb9&group_state=expanded might be easier to understand.  (I clicked the button that looks like a (+) to uncoalesce passes that got coalesced.)

In any case, the things you want to look at are the orange things (test failures).  Some of them happen intermittently; our automation auto-retriggers things that have a failure so that you get a second run and can see if the failure was happening intermittently.

There aren't failures in reftests on most platforms, only on Mulet:
 REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers-2.html | failed reftest-no-paint 
see the Mulet line, R (i.e., reftest) section, R6 build.

But you should also look at:
 721 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_bug369370.html | Checking scrollTop - got 150, expected 300 
which is in mochitest-plain, part 2 (M2).

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And the other key point here is that the failure is not a failure of your new test; it's a failure of an existing test.

Comment 34

[Karl Dubost💡 :karlcow]

Attachment: layers-2-test-fail.png

Ah thanks David. 
Interesting for 

 REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers-2.html | failed reftest-no-paint 

This one gives the same rendering 

* in my local patched build (45.0a1 (2015-12-04)), 
* in the latest nightly (45.0a1 (2015-12-03)) 
* and in Aurora (44.0a2 (2015-12-03)) 

but different in Opera Blink 34.0.2036.24

So I guess not my patch. Let's see the other failure.

Comment 35

[Karl Dubost💡 :karlcow]

Attachment: image-magnifier

So for this dom/html/test/test_bug369370.html

Indeed changing 300 to 150 makes the test a pass. As for the why I don't know.
https://dxr.mozilla.org/mozilla-central/source/dom/html/test/test_bug369370.html#78
https://bugzilla.mozilla.org/show_bug.cgi?id=369370

Using the example in the bug 369370 aka

window.open('http://www.freescale.com/files/graphic/product_freescale/AP13192USLK_IMG1.jpg','product_picture','menubar=no,width=500,height=500,resizable=yes');


So indeed there is a difference of behavior.
On the right side my local build (45.0a1 (2015-12-04)), 
on the left side the nightly build (45.0a1 (2015-12-03)) 

On clicking the same top left hole of the card, for zooming in, the image is centered on the chosen point for zooming in in the nightly build, but not in the case of my local build. So I guess there is a dependency in between vertically centered image and the zoom function in Firefox for images.

Comment 36

[Boris Zbarsky [:bzbarsky]]

> I noticed there is a reftest.list in the abs-pos/ directory and I didn't add to it the
> test + its ref

Er, yes.  You need to add it there for the test to get run.

> So I guess there is a dependency in between vertically centered image and the zoom
> function in Firefox for images.

Well.  So images in toplevel image documents are generally centered with the styles in http://mxr.mozilla.org/mozilla-central/source/layout/style/TopLevelImageDocument.css which include auto margins.

I expect what that means with your patch is that once you zoom out and the image is taller than the viewport it ends up being centered vertically so that you can't actually scroll to the top part of the image.  Is that the case?

If so, you will probably want to change the margin styles when the image is overflowing vertically (see ImageDocument::CheckOverflowing and the SetModeClass call in ImageDocument::RestoreImage, but you will need to have separate concepts of overflowing vertically and horizontally, I guess, since you want to keep centering vertically if the overflow is horizontal-only.

Comment 37

[Karl Dubost💡 :karlcow]

Boris,

> I expect what that means with your patch is that once you zoom out and the image is taller than the viewport it ends up being centered vertically so that you can't actually scroll to the top part of the image.  Is that the case?

Ah indeed! That's the case. 


> If so, you will probably want to change the margin styles when the image is overflowing vertically (see ImageDocument::CheckOverflowing and the SetModeClass call in ImageDocument::RestoreImage, but you will need to have separate concepts of overflowing vertically and horizontally, I guess, since you want to keep centering vertically if the overflow is horizontal-only.

Houla.
https://dxr.mozilla.org/mozilla-central/source/dom/html/ImageDocument.cpp

Comment 38

[Karl Dubost💡 :karlcow]

Boris,

I'm happy if someone wants to take over this bug. I would love to dig in, but not sure I have the right set of competences and time to be effective. I would need a lot of step by step assistance. It's becoming a bit harder to fix.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Split the "image is overflow" concept in ImageDocument into two separate booleans for vertical and horizontal overflow

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change the centering code in nsImageDocument to only try vertically centering via auto margins when we're not overflowing in the vertical direction, because if we _are_ overflowing that should cut off part of the image per spec

Comment 41

[Boris Zbarsky [:bzbarsky]]

With those patches (which also beef up the dom/html/test/test_bug369370.html test), the test passes even with the main patch to fix this bug applied.

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/791f13f9512b
https://hg.mozilla.org/integration/mozilla-inbound/rev/994cd45fade9
https://hg.mozilla.org/integration/mozilla-inbound/rev/c766b041302f

Comment 43

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/791f13f9512b
https://hg.mozilla.org/mozilla-central/rev/994cd45fade9
https://hg.mozilla.org/mozilla-central/rev/c766b041302f