Bug 12305 – REGRESSION: Images do not load in video sections on CNN.com homepage

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julian Grybowski]

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...

[Julian Grybowski]

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.

[Julian Grybowski]

Created attachment 12520 [details]
Screenshot of bad behavior in current Webkit nightly (r18901). Highlighted for clarity.

[Julian Grybowski]

Reflecting my discovery of the regression in the bug's title.

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.

(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).

[David Kilzer (:ddkilzer)]

Bug 11624 was filed to support the lowsrc attribute.

[Mark Rowe (bdash)]

<rdar://problem/4931480>

[David Kilzer (:ddkilzer)]

Created attachment 12526 [details]
Patch v1

Proposed patch.

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".

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

[David Kilzer (:ddkilzer)]

Created attachment 12530 [details]
Patch v2

Addresses issues in Comment #9.

[Darin Adler]

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.

[Darin Adler]

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

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

[Darin Adler]

I'll land this (working on it now).

[Darin Adler]

Committed revision 18940.

[David Kilzer (:ddkilzer)]

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