Bug 11824

Summary:

CSSStyleSheet.title property is always null

Product:

WebKit

Reporter:

Johnvey Hwang <johnvey>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

mitz, rwlbuis

Priority:

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+

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-	Details Formatted Diff Diff
Improved patch (4.63 KB, patch) 2006-12-14 12:48 PST, Rob Buis	darin: review-	Details Formatted Diff Diff
Improved testcase + patch (8.55 KB, patch) 2006-12-14 13:59 PST, Rob Buis	darin: review-	Details Formatted Diff Diff
Now with svg test (16.47 KB, patch) 2006-12-15 03:11 PST, Rob Buis	darin: review+	Details Formatted Diff Diff
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.