Bug 26396 – Percentage top value on position:relative descendant not resolved correctly if containing block height is not specified explicitly

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Gérard Talbot (no longer involved)]

This bug spun from comment #14 of bug 14749 and also (related) from bug 15930.

Steps to reproduce
------------------
Load provided URL

Expected results
----------------
The 400px wide by 200px tall red background box should appear toward the top of the document content box, very close to the top of the document content box (as if it had been defined as position: static)

Actual results in Safari 4.0 build 530.17
-----------------------------------------
The 400px wide by 200px tall red background box appears toward the middle of the browser window viewport height

Explanation
-----------
When the height of the containing block of a relatively positioned element (child or descendant) is auto (i.e., it depends on content height), then top: <percentage>; must be resolved as top: auto;.

References
----------

 * "Percentages: refer to height of containing block" 
coming from CSS 2.1, Section 9.3.2 Box offsets: 'top', 'right', 'bottom', 'left'
http://www.w3.org/TR/CSS21/visuren.html#position-props

  * Percentage values for the 'top' property are relative to the containing block.
  "if the element's position is 'relative' or 'static', the containing block is formed by the content edge of the nearest block-level, table cell or inline-block ancestor box." 
coming from CSS 2.1, Section 10.1 Definition of "containing block"
http://www.w3.org/TR/CSS21/visudet.html#containing-block-details

    * "If the height of the containing block is not specified explicitly (i.e., it depends on content height), and this element is not absolutely positioned, [then] the [height] value computes to 'auto'." 
coming from CSS 2.1, Section 10.5 Content height: the 'height' property
http://www.w3.org/TR/CSS21/visudet.html#the-height-property


An important sentence has been added to the CSS 2.1, section 10.5 recently which makes it possible to work around some difficulties in specific CSS-webpage-templates:
"A percentage height on the root element is relative to the initial containing block."
And the initial containing block "has the dimensions of the viewport and is anchored at the canvas origin". So, defining 
html {height: 100%;}
is the way I worked around distinct difficulties in the webpages
http://www.gtalbot.org/BrowserBugsSection/Safari3Bugs/PositionRelativeTopNegativePercentage.html
and
http://www.gtalbot.org/BrowserBugsSection/Safari3Bugs/LiquidScrollerTabMenu.html

When the height of containing block of a relatively positioned with top: percentage value is not specified explicitly (its height is/will be defined by its intrinsec content, once its intrinsec content has been rendered/loaded), then top: percentage value should be resolved as top: auto.

The spec makes sense (performance-wise) also in that it prevents browsers from making lots of computation and re-rendering, reflowing of document content.

height: auto makes the height entirely dependent and defined by the intrinsec
content. 
height: 100% for the root element (topmost containing block in the document containment hierarchy) refers to the initial containing
block's height which is defined by the browser window viewport height. The former can not be computed in advance; the latter can be computed in advance.

regards, Gérard

[Mihnea Ovidenie]

Created attachment 45798 [details]
Patch

I've taken a peek at bug 14749 and tried to fix this one.

[Mihnea Ovidenie]

Would be somebody so kind to take a look at this patch? 
Regards,
Mihnea

[Eric Seidel (no email)]

Comment on attachment 45798 [details]
Patch

Looks like this patch fails to apply.  Looks like an OK patch though, we just would want a CSS expert to take a look.

[Mihnea Ovidenie]

(In reply to comment #3)
> (From update of attachment 45798 [details])
> Looks like this patch fails to apply.  Looks like an OK patch though, we just
> would want a CSS expert to take a look.
Thx a lot Eric for trying to apply the patch,
Regards,
Mihnea

[Eric Seidel (no email)]

I didn't try myself, but the EWS bots did (see the bubbles below the patch links).  You should strongly consider posting an updated patch.

[Adele Peterson]

CC'ing a few more CSS experts.  I'm going to r- the patch for now.  Please post an updated version for the reviewers.  Thanks!

[Mihnea Ovidenie]

(In reply to comment #6)
> CC'ing a few more CSS experts.  I'm going to r- the patch for now.  Please post
> an updated version for the reviewers.  Thanks!

The error i get from the build bots is:
fatal: pathspec 'LayoutTests/fast/css/percent-top-relative-container-height-unspecified.html' did not match any files

I do not know how to interpret it. Before posting the patch, i svn add the above file because it is a new layout test. I also called svn add for the other files in platform/mac/fast/css/percent-*(checksum,png,txt) that are needed to match the layout test. Any suggestions to what i may doing wrong? Should i use git instead of svn?

Regards,

[Mihnea Ovidenie]

Created attachment 48755 [details]
Second patch

Hope i created a better patch this time.

[Mihnea Ovidenie]

Created attachment 49194 [details]
Another version of patch, try to make the bots happy

[Mihnea Ovidenie]

Sadly, i did not notice the empty lines added in the previous patches by svn-create-patch. Because of those lines, the commands that were trying to apply the patch failed with an error message that i failed to interpret correctly.

[Adam Barth]

Comment on attachment 49194 [details]
Another version of patch, try to make the bots happy

Sorry for the delay in reviewing your patch.  The comments added in this patch are redundant with the code that they describe.  Please either remove the comments or replace them with the "why" of the code instead of the "how."

As mentioned in the comments above, we need one of our CSS folks to review the technical content of this patch.  You might find some of those folks by seeing who in <https://trac.webkit.org/wiki/WebKit%20Team> has "CSS" listed next to their name.

[Andy Estes]

<rdar://problem/7769859>

[Andy Estes]

Created attachment 65525 [details]
Patch w/ WinIE quirk handling

[Andy Estes]

Hi Mihnea,

I hope you don't mind me taking your patch and re-posting it for review. I encountered this issue independently so I decided to help get this landed. The only changes I made were to the structure of the if expressions and to account for a sizing quirk we have for compatibility with IE. Otherwise, it's your patch.

Thanks!
Andy

[Joseph Pecoraro]

Comment on attachment 65525 [details]
Patch w/ WinIE quirk handling

LayoutTests/fast/css/percent-top-relative-container-height-unspecified.html:2
 +  <div style="position: relative; top: 50%;">This text with top percent relative and containing block auto should apear inside the red border</div>
LayoutTests/fast/css/percent-top-relative-container-height-unspecified.html:5
 +  <div style="position: relative; bottom: 50%;">This text with bottom percent relative and containing block auto should apear inside the red border</div>

Just an observation, not a review.
Nit: Typo "apear" should be "appear" in both of these lines.
This would also require updated results.

[Andy Estes]

(In reply to comment #15)
> (From update of attachment 65525 [details])
> LayoutTests/fast/css/percent-top-relative-container-height-unspecified.html:2
>  +  <div style="position: relative; top: 50%;">This text with top percent relative and containing block auto should apear inside the red border</div>
> LayoutTests/fast/css/percent-top-relative-container-height-unspecified.html:5
>  +  <div style="position: relative; bottom: 50%;">This text with bottom percent relative and containing block auto should apear inside the red border</div>
> 
> Just an observation, not a review.
> Nit: Typo "apear" should be "appear" in both of these lines.
> This would also require updated results.

Good catch! I'll fix it.

[Mihnea Ovidenie]

(In reply to comment #14)
> Hi Mihnea,
> 
> I hope you don't mind me taking your patch and re-posting it for review. I encountered this issue independently so I decided to help get this landed. The only changes I made were to the structure of the if expressions and to account for a sizing quirk we have for compatibility with IE. Otherwise, it's your patch.
> 
> Thanks!
> Andy

Hi Andy,

I am really glad that you took it and make it land in the trunk. 

Thanks,
Mihnea

[Darin Adler]

Comment on attachment 65525 [details]
Patch w/ WinIE quirk handling

> +    RenderBlock* theContainingBlock = containingBlock();

We normally don't use "the" in this way. Instead we do this:

    RenderBlock* containingBlock = this->containingBlock();

> +<div style="position: relative; top: 50%;">This text with top percent relative and containing block auto should apear inside the red border</div>

The word appear is misspelled.

[Andy Estes]

Committed http://trac.webkit.org/changeset/66170.