RESOLVED FIXED 11824
CSSStyleSheet.title property is always null 
https://bugs.webkit.org/show_bug.cgi?id=11824
Summary CSSStyleSheet.title property is always null
Johnvey Hwang
Reported 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".
Attachments
First attempt (5.53 KB, patch)
2006-12-14 12:04 PST, Rob Buis 		darin: review-
DetailsFormatted DiffDiff
Improved patch (4.63 KB, patch)
2006-12-14 12:48 PST, Rob Buis 		darin: review-
DetailsFormatted DiffDiff
Improved testcase + patch (8.55 KB, patch)
2006-12-14 13:59 PST, Rob Buis 		darin: review-
DetailsFormatted DiffDiff
Now with svg test (16.47 KB, patch)
2006-12-15 03:11 PST, Rob Buis 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Rob Buis
Comment 1 2006-12-14 08:23:47 PST
I have a patch for this, will post tonight. Cheers, Rob.
Rob Buis
Comment 2 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.
Darin Adler
Comment 3 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.
Rob Buis
Comment 4 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.
Rob Buis
Comment 5 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.
Darin Adler
Comment 6 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.
Rob Buis
Comment 7 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.
Darin Adler
Comment 8 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?
Rob Buis
Comment 9 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.
Rob Buis
Comment 10 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.
Darin Adler
Comment 11 2006-12-15 08:06:19 PST
Comment on attachment 11856 [details] Now with svg test r=me
Alexey Proskuryakov
Comment 12 2006-12-15 12:09:53 PST
Committed by Rob as r18233.
Note You need to log in before you can comment on or make changes to this bug.