NEW 37169
Fail WIT XHTML image attribute test (height=15% should be ignored)
https://bugs.webkit.org/show_bug.cgi?id=37169
Summary Fail WIT XHTML image attribute test (height=15% should be ignored)
Yong Li
Reported 2010-04-06 14:09:49 PDT
http://spe.mobilephone.net/wit/xhtmlv2/imageattr.xhtml The 2 images following "50% and 75% of screen width wide (15% high should be ignored resulting in normal aspect ratio, Globe should not be streched.)" shouldn't be stretched. Safari win32 fails, and I believe this is cross-platform issue. This should be easy to fix in CSSParser.cpp. But I'm not sure if the assertion in the test is correct. Can anyone with XHTML knowledge take a look?
Attachments
Proposed patch (5.76 KB, patch)
2010-04-30 04:56 PDT, Robin Cao
no flags
Patch (5.58 KB, patch)
2010-05-03 21:17 PDT, Robin Cao
staikos: review-
Revised patch (1.44 KB, patch)
2010-07-22 07:19 PDT, Robin Cao
no flags
Updated patch (5.98 KB, patch)
2010-07-23 06:29 PDT, Robin Cao
bdakin: review-
bdakin: commit-queue-
Alexey Proskuryakov
Comment 1 2010-04-06 21:06:10 PDT
FWIW, Firefox passes.
Yong Li
Comment 2 2010-04-07 07:02:27 PDT
(In reply to comment #1) > FWIW, Firefox passes. Yes. Also, if I save the source into an HTML file and open it, FireFox gives same behavior as Safari. So it must be difference between HTML and XHTML, but I haven't been able to find this at http://www.w3.org/TR/xhtml2/.
Robin Cao
Comment 3 2010-04-11 23:41:31 PDT
I checked the test page with FireFox and Opera, they both pass. I guess the reason is, for HTML 4.01, the height attribute is not ignored even when a percentage value is given. See <http://www.w3.org/TR/REC-html40/struct/objects.html#h-13.7.1>.
Yong Li
Comment 4 2010-04-12 16:27:11 PDT
(In reply to comment #3) > I checked the test page with FireFox and Opera, they both pass. > I guess the reason is, for HTML 4.01, the height attribute is not ignored even > when a percentage value is given. > See <http://www.w3.org/TR/REC-html40/struct/objects.html#h-13.7.1>. Is xhtml different?
Alexey Proskuryakov
Comment 5 2010-04-12 18:19:33 PDT
The difference is more likely to be between strict and quirks mode.
Robin Cao
Comment 6 2010-04-30 04:53:07 PDT
(In reply to comment #5) > The difference is more likely to be between strict and quirks mode. Yes, other browsers give different results in different modes. As per CSS2, percentage based height computes to 'auto', if the height of the containing block is not specified explicitly, and this element is not absolutely positioned.
Robin Cao
Comment 7 2010-04-30 04:56:23 PDT
Created attachment 54795 [details] Proposed patch
Darin Adler
Comment 8 2010-04-30 10:47:12 PDT
Comment on attachment 54795 [details] Proposed patch I don't have the background to know if this is a desirable change or not, so I'm not reviewing. Hyatt is one person I'd like to hear from on this, but others may have the expertise as well. Many web pages are in standards mode, so we need to be careful about changing standarsd mode behavior. Does this new behavior match Firefox too? > Property changes on: LayoutTests/fast/replaced/replaced-percent-height.html > ___________________________________________________________________ > Added: svn:executable > + * HTML files should not be executable.
Robin Cao
Comment 9 2010-05-03 21:13:11 PDT
(In reply to comment #8) > (From update of attachment 54795 [details]) > I don't have the background to know if this is a desirable change or not, so > I'm not reviewing. Hyatt is one person I'd like to hear from on this, but > others may have the expertise as well. Many web pages are in standards mode, so > we need to be careful about changing standarsd mode behavior. Does this new > behavior match Firefox too? Yes, the new behavior matches Firefox. Actually, most browsers (such as IE, Firefox, Opera) ignore the percentage height in standards mode. IMHO, this bug worth fixing for compatibility and standard compliance. > > Property changes on: LayoutTests/fast/replaced/replaced-percent-height.html > > ___________________________________________________________________ > > Added: svn:executable > > + * > > HTML files should not be executable. Fixed.
Robin Cao
Comment 10 2010-05-03 21:17:38 PDT
George Staikos
Comment 11 2010-05-20 02:37:25 PDT
Comment on attachment 54989 [details] Patch Just came across this issue myself, verified against FireFox. I believe the change is correct.
Eric Seidel (no email)
Comment 12 2010-05-21 11:08:13 PDT
I would really like someone who works in rendering more often than I or george do these days to comment before this get committed. This is a tiny change, but a comment from Hyatt, or Mitz, or James or Simon would make me more sure that it's right.
George Staikos
Comment 13 2010-05-22 23:47:27 PDT
I'm working with this stuff on a daily basis. You're just not seeing it yet - so I guess I have no real argument for you. However this is more about compliance, and so far it's pretty solid - not failing regression tests and causing problems that I can see.
Eric Seidel (no email)
Comment 14 2010-06-24 16:31:39 PDT
Comment on attachment 54989 [details] Patch Eh. Nm. i still feel this needs an OK from someone who works in rendering.
George Staikos
Comment 15 2010-07-21 19:16:02 PDT
Comment on attachment 54989 [details] Patch regression discovered in this patch causing infinite recursion in some cases.
Robin Cao
Comment 16 2010-07-22 07:19:08 PDT
Created attachment 62293 [details] Revised patch To avoid infinite recursion, check the length type of width before calling into calcReplacedWidth. The length type must be Fixed or Percent. I will add a test case later.
Robin Cao
Comment 17 2010-07-23 06:29:19 PDT
Created attachment 62422 [details] Updated patch Add a test case for the infinite recursion and update the patch with ToT.
Benjamin Poulain
Comment 18 2010-11-09 05:32:10 PST
*** Bug 40475 has been marked as a duplicate of this bug. ***
Beth Dakin
Comment 19 2011-04-26 15:54:23 PDT
Comment on attachment 62422 [details] Updated patch R-minusing because this patch will not longer apply cleanly. I haven't looked far enough to see where this code was moved to or what it was renamed to, but RenderBox::calcReplacedHeightUsing() does not seem to exist anymore.
Note You need to log in before you can comment on or make changes to this bug.