Bug 11824

Summary: CSSStyleSheet.title property is always null
Product: WebKit Reporter: Johnvey Hwang <johnvey>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: mitz, rwlbuis
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
First attempt
darin: review-
Improved patch
darin: review-
Improved testcase + patch
darin: review-
Now with svg test darin: review+

Description Johnvey Hwang 2006-12-13 19:00:12 PST
Trying to access the CSSStyleSheet.title property always returns null

If the following is inserted into the <head>:

<link media="all" href="/static345/css/skins/basic.css" rel="stylesheet" title="basic">

Then a subsequent access of:

document.styleSheets[0].title

returns null.  It should return "basic".
Comment 1 Rob Buis 2006-12-14 08:23:47 PST
I have a patch for this, will post tonight.
Cheers,

Rob.
Comment 2 Rob Buis 2006-12-14 12:04:54 PST
Created attachment 11840 [details]
First attempt

This patch implements setting of the title when the sheet is created, for HTML elements <style> and <link>, and so fixes the bug. Also contains a testcase that tests both element's titles.
Cheers,

Rob.
Comment 3 Darin Adler 2006-12-14 12:12:56 PST
Comment on attachment 11840 [details]
First attempt

-    void childrenChanged(Node*);
+    void childrenChanged(Node*, const String& = String());

This is not a good change. Even though the declaration does not say so, this is an override of the childrenChanged function from Node.

By changing the parameters this no longer overrides the Node function, so callers that call the virtual function on Node won't call this function any more. I think this subtle semantic change probably creates bugs.

To make this clearer, the virtual keyword should be added to these functions in StyleElement.h. And we need to rethink how to do this -- I think it should be a separate function rather than a new parameter to childrenChanged.
Comment 4 Rob Buis 2006-12-14 12:46:01 PST
Hi Darin,

(In reply to comment #3)
> (From update of attachment 11840 [details] [edit])
> -    void childrenChanged(Node*);
> +    void childrenChanged(Node*, const String& = String());
> 
> This is not a good change. Even though the declaration does not say so, this is
> an override of the childrenChanged function from Node.
> 
> By changing the parameters this no longer overrides the Node function, so
> callers that call the virtual function on Node won't call this function any
> more. I think this subtle semantic change probably creates bugs.
> 
> To make this clearer, the virtual keyword should be added to these functions in
> StyleElement.h. And we need to rethink how to do this -- I think it should be a
> separate function rather than a new parameter to childrenChanged.

You are right, I was already worried when writing it that way but did not really have time to do better. Now I have and I'll apply a better patch (IMHO) soon.
Cheers,

Rob.
Comment 5 Rob Buis 2006-12-14 12:48:35 PST
Created attachment 11842 [details]
Improved patch

I think this is a bit cleaner than the previous patch.
Cheers,

Rob.
Comment 6 Darin Adler 2006-12-14 13:27:54 PST
Comment on attachment 11842 [details]
Improved patch

+                m_sheet->setTitle(static_cast<HTMLElement*>(e)->title());

title() is just a convenience to get the title attribute. It can be moved to Element if we want to avoid the type cast. Or we can call getAttribute(titleAttr) here. I suggest not limiting this to HTML elements, unless there's a reason to. I'd like the titles to work for SVG style elements too.
Comment 7 Rob Buis 2006-12-14 13:59:36 PST
Created attachment 11845 [details]
Improved testcase + patch

I tried to implement Darin'ssuggestions. Also Mitz had a good new testcase scenario where title attr is set dynamically from js. So the testcase is updated and works just like FF.
Cheers,

Rob.
Comment 8 Darin Adler 2006-12-14 14:04:19 PST
Comment on attachment 11845 [details]
Improved testcase + patch

-    void childrenChanged(Node*);
+    void childrenChanged(Element*);

This has the same overloading problem as the first patch; changing the type of the parameter prevents it from properly overriding the function in the base class.

The rest looks basically good. But what about SVG style elements?
Comment 9 Rob Buis 2006-12-14 14:30:24 PST
Hi Darin,

(In reply to comment #8)
> (From update of attachment 11845 [details] [edit])
> -    void childrenChanged(Node*);
> +    void childrenChanged(Element*);
> 
> This has the same overloading problem as the first patch; changing the type of
> the parameter prevents it from properly overriding the function in the base
> class.

Mitz and myself do not get what you mean here. childrenChanged(Element*) is not virtual, so I don't see  how overriding is involved. I think there is enough difference between the two functions for the compiler to distinguish them. I can see the name may be confusing, would it help if we name it process() for example? If you feel the problem is still there, can you explain it one more time?
 
> The rest looks basically good. But what about SVG style elements?

I'll add a test in the morning for svg style elements. Will likely be a separate svg test in svg/custom.
Cheers,

Rob.
Comment 10 Rob Buis 2006-12-15 03:11:29 PST
Created attachment 11856 [details]
Now with svg test

I added a svg test. Also I reverted sharing setStyle since it works differently for html and svg. For html it can be written, but svg should throw an exception, so it has a different function signature
Cheers,

Rob.
Comment 11 Darin Adler 2006-12-15 08:06:19 PST
Comment on attachment 11856 [details]
Now with svg test

r=me
Comment 12 Alexey Proskuryakov 2006-12-15 12:09:53 PST
Committed by Rob as r18233.