Bug 12305 - REGRESSION: Images do not load in video sections on CNN.com homepage
Summary: REGRESSION: Images do not load in video sections on CNN.com homepage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Darin Adler
URL: http://www.cnn.com
Keywords: InRadar, NeedsReduction, Regression
Depends on:
Blocks:
 
Reported: 2007-01-17 11:08 PST by Julian Grybowski
Modified: 2007-01-18 09:09 PST (History)
2 users (show)

See Also:


Attachments
Screenshot of (proper) behavior in released Safari (2.0.4 / 419.3) (316.66 KB, image/jpeg)
2007-01-17 11:19 PST, Julian Grybowski
no flags Details
Screenshot of bad behavior in current Webkit nightly (r18901). Highlighted for clarity. (323.14 KB, image/jpeg)
2007-01-17 11:26 PST, Julian Grybowski
no flags Details
Patch v1 (5.44 KB, patch)
2007-01-17 22:42 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (4.56 KB, patch)
2007-01-18 06:11 PST, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Grybowski 2007-01-17 11:08:37 PST
Steps to reproduce this bug:

1. Navigate to http://www.cnn.com

2. Bring your attention to the "WATCH VIDEO" and "CNN Pipeline" spots (below the main articles, center and right when the page isn't consumed by some breaking news item).

See the areas where one video article in particular is open so that a brief summary is displayed? There should be a (still) image to the right of it. You can see the outline of where the image is supposed to go in the "CNN Pipeline" box, but the image itself does not draw. These images load with no problems in Firefox.

As CNN is fairly major as sites go, this should probably be taken care of as soon as possible...
Comment 1 Julian Grybowski 2007-01-17 11:19:56 PST
Created attachment 12519 [details]
Screenshot of (proper) behavior in released Safari (2.0.4 / 419.3)

As you can see in this screenshot, the bug isn't an issue in released Safari, which makes this a regression.
Comment 2 Julian Grybowski 2007-01-17 11:26:06 PST
Created attachment 12520 [details]
Screenshot of bad behavior in current Webkit nightly (r18901). Highlighted for clarity.
Comment 3 Julian Grybowski 2007-01-17 11:27:22 PST
Reflecting my discovery of the regression in the bug's title.
Comment 4 mitz 2007-01-17 11:53:57 PST
JavaScript excerpt from cnn.com:

if (document.getElementById(realId).lowsrc){
	document.getElementById(realId).src = document.getElementById(realId).lowsrc;
}

In shipping Safari any node attribute (in this case, 'lowsrc') is accessible as a JavaScript property. Not so in TOT.
Comment 5 mitz 2007-01-17 12:49:05 PST
(In reply to comment #4)

> In shipping Safari any node attribute (in this case, 'lowsrc') is accessible as
> a JavaScript property. Not so in TOT.
> 

The above explains why the script works in shipping Safari and fails in TOT. In order to fix the bug it is enough to map the lowsrc attribute (even if it's not supported in WebKit).
Comment 6 David Kilzer (:ddkilzer) 2007-01-17 13:06:23 PST
Bug 11624 was filed to support the lowsrc attribute.

Comment 7 Mark Rowe (bdash) 2007-01-17 16:38:20 PST
<rdar://problem/4931480>
Comment 8 David Kilzer (:ddkilzer) 2007-01-17 22:42:22 PST
Created attachment 12526 [details]
Patch v1

Proposed patch.
Comment 9 mitz 2007-01-17 23:02:46 PST
Comment on attachment 12526 [details]
Patch v1

+        * fast/js/lowsrc-img-property.html: Added.

The test should be in fast/dom/HTMLImageElement. It's a DOM test, not a JS test.

The operators should go before the line breaks:

+    return attr->name() == srcAttr
+           || attr->name() == lowsrcAttr
+           || (attr->name() == usemapAttr && attr->value().domString()[0] != '#');

This attribute is not in the W3C spec. It should appear under "extensions".
Comment 10 mitz 2007-01-17 23:03:45 PST
(In reply to comment #9)
> This attribute is not in the W3C spec. It should appear under "extensions".
> 

This comment refers to the change to the .idl file.
Comment 11 David Kilzer (:ddkilzer) 2007-01-18 06:11:43 PST
Created attachment 12530 [details]
Patch v2

Addresses issues in Comment #9.
Comment 12 Darin Adler 2007-01-18 07:44:40 PST
Comment on attachment 12530 [details]
Patch v2

r=me

I know this is not in our style guide, but I personally like to format continued expressions like this:

    return attr->name() == srcAttr
        || attr->name() == lowsrcAttr
        || (attr->name() == usemapAttr && attr->value().domString()[0] != '#');

Somehow it just looks more "orderly" to me.
Comment 13 Darin Adler 2007-01-18 07:49:12 PST
(In reply to comment #12)
> I know this is not in our style guide, but I personally like to format
> continued expressions like this:

And it looks like Mitz mentioned this too, in comment #9.
Comment 14 mitz 2007-01-18 08:00:52 PST
(In reply to comment #13)
> (In reply to comment #12)
> > I know this is not in our style guide, but I personally like to format
> > continued expressions like this:
> 
> And it looks like Mitz mentioned this too, in comment #9.
> 

Actually Dave had it "the right way" in the first patch and I suggested changing it based on what I've seen in recent patches. There should be a style guideline on this.

Sorry, Dave!
Comment 15 Darin Adler 2007-01-18 08:51:43 PST
I'll land this (working on it now).
Comment 16 Darin Adler 2007-01-18 08:57:34 PST
Committed revision 18940.
Comment 17 David Kilzer (:ddkilzer) 2007-01-18 09:09:07 PST
(In reply to comment #12)
> I know this is not in our style guide, but I personally like to format
> continued expressions like this:
> 
>     return attr->name() == srcAttr
>         || attr->name() == lowsrcAttr
>         || (attr->name() == usemapAttr && attr->value().domString()[0] != '#');
> 
> Somehow it just looks more "orderly" to me.

I was told that putting the operators on the left (instead of the right) makes them more visible as you're reading the code.

It probably has to do with paying closer attention to the beginning of a line rather than the end of a line while reading code.