WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
53717
REGRESSION (
r61234
): washingtonpost.com top bar looks wrong, doesn't animate
https://bugs.webkit.org/show_bug.cgi?id=53717
Summary
REGRESSION (r61234): washingtonpost.com top bar looks wrong, doesn't animate
Andy Estes
Reported
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
.
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
View All
Add attachment
proposed patch, testcase, etc.
Andy Estes
Comment 1
2011-02-03 14:22:20 PST
<
rdar://problem/8955448
>
Andy Estes
Comment 2
2011-02-03 14:48:09 PST
Minefield doesn't have this bug.
Andy Estes
Comment 3
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})')
Adam Barth
Comment 4
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.
Andy Estes
Comment 5
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.
Andy Estes
Comment 6
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.
Adam Barth
Comment 7
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.
Adam Barth
Comment 8
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.
Eric Seidel (no email)
Comment 9
2011-02-15 17:19:42 PST
Will fix.
Eric Seidel (no email)
Comment 10
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).
Eric Seidel (no email)
Comment 11
2011-02-15 18:04:02 PST
Created
attachment 82566
[details]
Patch
Alexey Proskuryakov
Comment 12
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?
Adam Barth
Comment 13
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.
Eric Seidel (no email)
Comment 14
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.
WebKit Commit Bot
Comment 15
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
>
WebKit Commit Bot
Comment 16
2011-02-16 16:31:27 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug