Bug 37169 – Fail WIT XHTML image attribute test (height=15% should be ignored)

• Component: WebCore Misc.
• Status: NEW

[Yong Li]

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?

[Alexey Proskuryakov]

FWIW, Firefox passes.

[Yong Li]

(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]

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]

(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]

The difference is more likely to be between strict and quirks mode.

[Robin Cao]

(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]

Created attachment 54795 [details]
Proposed patch

[Darin Adler]

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]

(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]

Created attachment 54989 [details]
Patch

[George Staikos]

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)]

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]

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 on attachment 54989 [details]
Patch

Eh.  Nm.  i still feel this needs an OK from someone who works in rendering.

[George Staikos]

Comment on attachment 54989 [details]
Patch

regression discovered in this patch causing infinite recursion in some cases.

[Robin Cao]

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]

Created attachment 62422 [details]
Updated patch

Add a test case for the infinite recursion and update the patch with ToT.

[Benjamin Poulain]

*** Bug 40475 has been marked as a duplicate of this bug. ***

[Beth Dakin]

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.