Summary: | Percentage top value on position:relative descendant not resolved correctly if containing block height is not specified explicitly | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gérard Talbot (no longer involved) <browserbugs2> | ||||||||||
Component: | Layout and Rendering | Assignee: | Andy Estes <aestes> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | adele, aestes, bdakin, eric, hyatt, jamesr, mihnea, mitz, simon.fraser | ||||||||||
Priority: | P1 | Keywords: | HasReduction, InRadar, Regression | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Windows XP | ||||||||||||
URL: | http://www.gtalbot.org/BrowserBugsSection/MSIE7Bugs/PercentualRelativePositioning.html | ||||||||||||
Attachments: |
|
Description
Gérard Talbot (no longer involved)
2009-06-14 20:38:09 PDT
Created attachment 45798 [details] Patch I've taken a peek at bug 14749 and tried to fix this one. Would be somebody so kind to take a look at this patch? Regards, Mihnea 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.
(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 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. 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! (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, Created attachment 48755 [details]
Second patch
Hope i created a better patch this time.
Created attachment 49194 [details]
Another version of patch, try to make the bots happy
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 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. Created attachment 65525 [details]
Patch w/ WinIE quirk handling
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 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.
(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. (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 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. Committed http://trac.webkit.org/changeset/66170. |