Bug 53717 - REGRESSION (r61234): washingtonpost.com top bar looks wrong, doesn't animate
Summary: REGRESSION (r61234): washingtonpost.com top bar looks wrong, doesn't animate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL: http://washingtonpost.com
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks: 41115
  Show dependency treegraph
 
Reported: 2011-02-03 14:22 PST by Andy Estes
Modified: 2011-02-16 16:31 PST (History)
7 users (show)

See Also:


Attachments
Screenshot (154.40 KB, application/pdf)
2011-02-03 14:22 PST, Andy Estes
no flags Details
Test case (587 bytes, text/html)
2011-02-03 15:12 PST, Andy Estes
no flags Details
Patch (4.75 KB, patch)
2011-02-15 18:04 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2011-02-03 14:22:11 PST
Created attachment 81115 [details]
Screenshot

* SUMMARY
In washingtonpost.com the news bar on top that animates between headlines shows all items and fails to animate. See the screen shot.

* REGRESSION
Works in shipping Safari. Regression due to http://trac.webkit.org/changeset/61234.
Comment 1 Andy Estes 2011-02-03 14:22:20 PST
<rdar://problem/8955448>
Comment 2 Andy Estes 2011-02-03 14:48:09 PST
Minefield doesn't have this bug.
Comment 3 Andy Estes 2011-02-03 14:56:54 PST
I see this error in the Web Inspector:

TypeError: 'undefined' is not a function (evaluating '$('#skybox_slideshow').cycle({timeout:4000,pause:true,speed:800})')
Comment 4 Adam Barth 2011-02-03 15:06:51 PST
Nothing obvious jumps out at my from looking at the page.  We'll need to create a reduction.
Comment 5 Andy Estes 2011-02-03 15:12:54 PST
Created attachment 81120 [details]
Test case

Reduction attached. The issue seems to be with this snippet:

<script src="
http://media3.washingtonpost.com/wp-srv/wpost/javascript/plugin/plugin.jquery.cycle.all-2.88.0.js
" type="text/javascript" charset="utf-8"></script>

If you remove the line break between 'src="' and 'http://...' the bug goes away.
Comment 6 Andy Estes 2011-02-03 15:15:04 PST
Just to explain the test case, the correct behavior is to see text that cycles between the words 'FIRST' and 'SECOND' once every 800 ms. If you see both 'FIRST' and 'SECOND' at the same time, this is a bug.
Comment 7 Adam Barth 2011-02-03 15:21:38 PST
Interesting.  This is probably related to the uncertainty as whose job it is to trim whitespace from before and after URLs.  Should be easy to fix.  The question is mostly at which layer to trim the whitespace.
Comment 8 Adam Barth 2011-02-15 17:18:43 PST
This is a bug in KURL.  The test case works fine with GURL.  KURL is supposed to trim leading whitespace from URLs.
Comment 9 Eric Seidel (no email) 2011-02-15 17:19:42 PST
Will fix.
Comment 10 Eric Seidel (no email) 2011-02-15 17:37:48 PST
There were already tests for this in fast/url/scheme.html, but they're disabled:

  // These tests trigger the relative URL resolving behavior of
  // HTMLAnchorElement.href.  In order to test absolute URL parsing, we'd need
  // an API that always maps to absolute URLs.  If you know of one, please
  // enable these tests!
  // [" HTTP ", "%20http%20"],
  // ["htt: ", "htt%3A%20"],
  // ["\xe4\xbd\xa0\xe5\xa5\xbdhttp", "%E4%BD%A0%E5%A5%BDhttp"],
  // ["ht%3Atp", "ht%3atp"],

I'll add the parser version of this test case since that's testable (as demonstrated by the attachment).
Comment 11 Eric Seidel (no email) 2011-02-15 18:04:02 PST
Created attachment 82566 [details]
Patch
Comment 12 Alexey Proskuryakov 2011-02-15 18:08:01 PST
Comment on attachment 82566 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82566&action=review

> Source/WebCore/platform/KURL.cpp:349
> +    // characters from URLs.  Note that c is an *unsigned* char here
> +    // so this comparison should only catch control characters.

Is this really worth explaining in a comment?
Comment 13 Adam Barth 2011-02-16 00:02:49 PST
Comment on attachment 82566 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82566&action=review

I believe this change is correct.  I don't really have an opinion on the comment.

>> Source/WebCore/platform/KURL.cpp:349
>> +    // Browsers ignore leading/trailing whitespace and control
>> +    // characters from URLs.  Note that c is an *unsigned* char here
>> +    // so this comparison should only catch control characters.
> 
> Is this really worth explaining in a comment?

Most of the rest of KURL uses plain "char", which can be subtly confusing.
Comment 14 Eric Seidel (no email) 2011-02-16 00:06:19 PST
(In reply to comment #12)
> (From update of attachment 82566 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82566&action=review
> 
> > Source/WebCore/platform/KURL.cpp:349
> > +    // characters from URLs.  Note that c is an *unsigned* char here
> > +    // so this comparison should only catch control characters.
> 
> Is this really worth explaining in a comment?

Most of the rest of KURL actually gets it wrong. :)  Compares with < to a char and assumes it works.  I added the comment after I got it wrong myself.

I'll go through and fix the other KURL char compares in a separate patch.
Comment 15 WebKit Commit Bot 2011-02-16 16:31:21 PST
Comment on attachment 82566 [details]
Patch

Clearing flags on attachment: 82566

Committed r78747: <http://trac.webkit.org/changeset/78747>
Comment 16 WebKit Commit Bot 2011-02-16 16:31:27 PST
All reviewed patches have been landed.  Closing bug.