Bug 26396 - Percentage top value on position:relative descendant not resolved correctly if containing block height is not specified explicitly
Summary: Percentage top value on position:relative descendant not resolved correctly i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P1 Normal
Assignee: Andy Estes
URL: http://www.gtalbot.org/BrowserBugsSec...
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2009-06-14 20:38 PDT by Gérard Talbot
Modified: 2010-08-26 19:39 PDT (History)
9 users (show)

See Also:


Attachments
Patch (35.49 KB, patch)
2010-01-04 06:29 PST, Mihnea Ovidenie
adele: review-
eric: commit-queue-
Details | Formatted Diff | Diff
Second patch (35.50 KB, patch)
2010-02-15 08:35 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Another version of patch, try to make the bots happy (35.47 KB, patch)
2010-02-22 02:44 PST, Mihnea Ovidenie
abarth: review-
Details | Formatted Diff | Diff
Patch w/ WinIE quirk handling (35.62 KB, patch)
2010-08-25 22:46 PDT, Andy Estes
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gérard Talbot 2009-06-14 20:38:09 PDT
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
Comment 1 Mihnea Ovidenie 2010-01-04 06:29:34 PST
Created attachment 45798 [details]
Patch

I've taken a peek at bug 14749 and tried to fix this one.
Comment 2 Mihnea Ovidenie 2010-01-11 06:52:14 PST
Would be somebody so kind to take a look at this patch? 
Regards,
Mihnea
Comment 3 Eric Seidel (no email) 2010-01-26 14:26:24 PST
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.
Comment 4 Mihnea Ovidenie 2010-01-26 22:37:50 PST
(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
Comment 5 Eric Seidel (no email) 2010-02-01 16:30:14 PST
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.
Comment 6 Adele Peterson 2010-02-01 21:46:19 PST
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!
Comment 7 Mihnea Ovidenie 2010-02-02 08:25:30 PST
(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,
Comment 8 Mihnea Ovidenie 2010-02-15 08:35:05 PST
Created attachment 48755 [details]
Second patch

Hope i created a better patch this time.
Comment 9 Mihnea Ovidenie 2010-02-22 02:44:40 PST
Created attachment 49194 [details]
Another version of patch, try to make the bots happy
Comment 10 Mihnea Ovidenie 2010-02-22 03:22:29 PST
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.
Comment 11 Adam Barth 2010-03-22 10:34:11 PDT
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.
Comment 12 Andy Estes 2010-08-25 22:04:35 PDT
<rdar://problem/7769859>
Comment 13 Andy Estes 2010-08-25 22:46:46 PDT
Created attachment 65525 [details]
Patch w/ WinIE quirk handling
Comment 14 Andy Estes 2010-08-25 22:47:38 PDT
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
Comment 15 Joseph Pecoraro 2010-08-25 23:25:20 PDT
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.
Comment 16 Andy Estes 2010-08-25 23:34:11 PDT
(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.
Comment 17 Mihnea Ovidenie 2010-08-26 05:11:16 PDT
(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
Comment 18 Darin Adler 2010-08-26 11:18:23 PDT
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.
Comment 19 Andy Estes 2010-08-26 19:39:39 PDT
Committed http://trac.webkit.org/changeset/66170.