Bug 53717 – REGRESSION (r61234): washingtonpost.com top bar looks wrong, doesn't animate

• Component: DOM
• Status: RESOLVED FIXED

[Andy Estes]

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.

[Andy Estes]

<rdar://problem/8955448>

[Andy Estes]

Minefield doesn't have this bug.

[Andy Estes]

I see this error in the Web Inspector:

TypeError: 'undefined' is not a function (evaluating '$('#skybox_slideshow').cycle({timeout:4000,pause:true,speed:800})')

[Adam Barth]

Nothing obvious jumps out at my from looking at the page.  We'll need to create a reduction.

[Andy Estes]

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.

[Andy Estes]

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.

[Adam Barth]

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.

[Adam Barth]

This is a bug in KURL.  The test case works fine with GURL.  KURL is supposed to trim leading whitespace from URLs.

[Eric Seidel (no email)]

Will fix.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Created attachment 82566 [details]
Patch

[Alexey Proskuryakov]

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?

[Adam Barth]

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.

[Eric Seidel (no email)]

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

[WebKit Commit Bot]

Comment on attachment 82566 [details]
Patch

Clearing flags on attachment: 82566

Committed r78747: <http://trac.webkit.org/changeset/78747>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.