Bug 3616

Summary: RSS search field, Dashboard widgets failing due to CSS exception
Product: WebKit Reporter: Darin Adler <darin>
Component: DOMAssignee: Darin Adler <darin>
Status: VERIFIED FIXED    
Severity: Blocker    
Priority: P1    
Version: 412   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Patch that ignores exceptions when setting a CSS property via the style.xxx = yyy syntax
mjs: review-
First cut at a test case
none
Newer patch, includes test case and ChangeLog sullivan: review+

Description Darin Adler 2005-06-19 23:44:55 PDT
5/17/05 2:54 PM John Sullivan:
To reproduce:

1. go to feed://slashdot.org/index.rss (or any other RSS feed page) in Safari
2. At this point, note the following console message:

[5711] feed://slashdot.org/index.rss:Error - CSS exception 0

3. Type a character into the search field. Note that the following message appears in the console:
(event handler):CSS exception 0

4. Continue typing characters into the search field until there are no articles found. Note that in 
addition to the CSS exception message for each typed character, a previous/more selector thingy 
appears on the page that should be blank except for the big gray "No Articles" text. (Maybe this thingy 
might be some holdover from an earlier design that's normally always hidden?)

5. Backspace until there are found articles again. Note that the big gray "No Articles" text isn't erased, 
and neither is the mysterious previous/more selector thingy. See screenshot.
Comment 1 Darin Adler 2005-06-19 23:45:45 PDT
In Radar:

<rdar://problem/4122131> REGRESSION (412+): Typing in RSS page's search field causes CSS exception 
in console and bad display
Comment 2 Darin Adler 2005-06-19 23:46:44 PDT
5/17/05 2:56 PM John Sullivan:
This doesn't happen with stock Safari, so it's a regression in tip of tree. I'm guessing that it's caused by 
recent JavaScript changes, but maybe it's something else.

5/26/05 3:35 PM Darin Adler:
The problem is that the RSS template has code that depends on something broken in WebKit. The code 
says:

    noarticles.style.display = null;

The DOM specification says this is illegal and should result in an exception. Gecko and older Safari 
versions don't raise an exception here, but WinIE matches the DOM spec.

The reason earlier Safari versions didn't raise an exception was a bug.

5/26/05 3:43 PM Darin Adler:
I filed a bug about fixing RSS to not use style.display = null. But I was thinking that we still might need 
to fix WebCore, so keeping this one.

6/17/05 1:23 PM Jessica Kahn:
I see a lot of exceptions thrown from Dashboard widgets with TOT WebKit installed, which look a lot 
like the exception that is thrown from Safari RSS when loading feeds (fixed by our use of "" instead of 
null, as described above). Just FYI... we apparently weren't the only ones doing something wrong.

6/19/05 7:42 PM Darin Adler:
It would be very helpful to know which widgets were throwing those exceptions.

6/19/05 7:48 PM Darin Adler:
Attached a patch. But before I make a change I'd like to verify what's really going on with the Dashboard 
widgets, so someone (maybe me) will have to come up with reproducible cases with some of them to 
test.
Comment 3 Darin Adler 2005-06-19 23:47:34 PDT
Created attachment 2482 [details]
Patch that ignores exceptions when setting a CSS property via the style.xxx = yyy syntax
Comment 4 Darin Adler 2005-06-19 23:47:59 PDT
Comment on attachment 2482 [details]
Patch that ignores exceptions when setting a CSS property via the style.xxx = yyy syntax

No layout test yet, but I think maybe still ready for review.
Comment 5 Maciej Stachowiak 2005-06-20 00:51:06 PDT
Comment on attachment 2482 [details]
Patch that ignores exceptions when setting a CSS property via the style.xxx = yyy syntax

Code looks good, just needs a test case added (or an argument why one is
impossible or unneeded).
Comment 6 Darin Adler 2005-06-20 17:44:59 PDT
Created attachment 2512 [details]
First cut at a test case

I should probably roll this into the patch.
Comment 7 Darin Adler 2005-06-20 19:23:14 PDT
Created attachment 2514 [details]
Newer patch, includes test case and ChangeLog
Comment 8 Darin Adler 2005-06-21 10:02:46 PDT
Comment on attachment 2514 [details]
Newer patch, includes test case and ChangeLog

Meant to request review, not review my own patch!
Comment 9 John Sullivan 2005-06-22 09:31:10 PDT
Comment on attachment 2514 [details]
Newer patch, includes test case and ChangeLog

Maciej reviewed the patch before there were test cases, and I also reviewed it
when the test cases were present.