To reproduce: 1. Install attached test files (style.html, style.css, style2.css) onto your hard drive. 2. Run viewer. 3. Load 'style.html' 4. Click "Add Style Sheet" button. Expected behavior: style2.css is loaded and BOLD style is applied to "Hello, world" text. Current behavior: link tag is created, but style sheet is not loaded or parsed.

Yup. This will change when I start using Peter's new CSS loader. M8 for now.

And one more punt to M9 so that it goes along with the content sink changes.

*** Bug 9885 has been marked as a duplicate of this bug. ***

The duplicate bug also mentioned the following test cases: http://www.fas.harvard.edu/~dbaron/dom/test/one-core-xml/ProcessingInstruction http://www.fas.harvard.edu/~dbaron/dom/test/one-core-xml/ProcessingInstruction_2

Bug 9885 dealt with stylesheets linked by processing-instructions, not LINK elements. If you don't want to fix both at once, it should be reopened.

Do we need this for beta? I'm going to say ``yes'' for now, but I'm prepared to triage it out later.

Moving to M12. Marking [TESTCASE].

Moving off M12 radar for the time being. One or more might get back once I get a chance to really look at them.

Per comments above, adding dependency on bug 21771: "Factor out code among the HTML/XML/XUL content sinks". Also adding dependency on HTML4 compliance bug.

(Oops, got the dependencies the wrong way round. Fixing...)

(Oops, got the dependencies the wrong way round. Fixing...)

(And after all that I still forgot to add myself to the cc list. D'oh! Ok, I'm going to bed now. Honest.)

It'd be really great if I got this in for beta 1, wouldn't it? :-)

Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!

Still happens on all platforms with the 2000-01-26-xx M13 commercial builds... Setting bug severity to critical...

Adding beta1 keyword

Hmmm, I wonder why this bug hasn't been looked at by pdt yet, it has the magic beta1 in keyword.

rickg or buster, need help on a PDT+ or PDT- call here please.

Not necessary for beta 1. There are no existing pages that depend on this.

I agree with V -- marking PDT-.

Moving forward to M16.

nominating for nsbeta2 based on: - severity - feature (DOM Level 1 Standards Compliance) broken

Tested on: - MacOS9 2000-04-27-08-M16 Commercial Build - Linux6 2000-04-27-09-M16 Commercial Build - Win98 2000-04-27-09-M16 Mozilla Build and the bug still exists.

eric, need your input here.

Important reasons we'd like to get this in for FCS (and therefore nsbeta2): 1) HTML 4.0 says LINK can be used to link in style sheets 2) DOM1 says that JS can be used through DOM1 to modify page contents, including (for example) adding LINKs to style sheets 3) Supporting CSS is a key objective 4) developer functional requirement: we want to enable easy reformatting of a page by adding/deleting LINKs to style sheet files; that makes CSS powerful, pages dynamic, and XML/HTML files flexibly reformattable Application examples for this functionality: A) Reformatting a page by applying a new style sheet: If we support dynamic adding of LINK, a developer can dynamically apply an arbitrarily-named style sheet to a page (even one newly-created just-in-time and written out on the server side on the fly) by adding in a LINK to it. If not, the developer has to hard-code links to all the style sheets she might wish to select among, and then choose which ones to apply by enabling and disabling them with JavaScript. (This is not the end of the world; we have demos that do this already, and, in general, you'll have a limited number of pre-created style sheets from which you'll be selecting one. So this may be an acceptable workaround if we don't get this feature in.) B) Something I'm not sure about: what if the HTML page is dynamically generated by JavaScript through document.write()s, including a LINK to a style sheet--will that work? I suspect so even if we don't support dynamic LINKing of style sheets after a page has loaded. Both for reasons of standards compliance and for practical developer needs for a robust platform, we want this functionality in. Vidur estimates that fixing this would be 1-2 days work. If we *don't* get it in by 6/15, developers may be able to get their work done by using the workarounds I've mentioned. (dbaron, Ian, dannyg, vidur, jst: please check if you think I'm correct about that. If you can, please explain counterexamples of things that *can't* be done without supporting the dynamic linking of style sheets via LINK. I'm not clear on whether this should be categorized as a bug (dynamically adding a style sheet LINK has no effect) or a feature (need to support dynamic adding of style sheet LINKs). Because it's 1-2 days work, marking [FEATURE]. Setting to [nsbeta2+][6/15].

I think this is a bug, not a missing feature. (Can you do <style src=>? If so, is that sufficient workaround?)

Note related bug 34849 "dynamically added STYLE element doesn't alter style." However, re: Mike's question: if you already have a hardcoded STYLE element and then set its SRC attribute, I have no idea what happens. (FWIW, if you have a SCRIPT element and set SRC, or if you dynamically write a new SCRIPT SRC=, that's currently not being loaded.)

Taking this bug off Vidurs list.

STYLE has no SRC attribute.

PDT: per ekrock@netscape.com 2000-05-25 15:26 comments, please leave this nsbeta2+. jst has taken the bug from vidur, and it should only take 1-2 days to fix.

Marking nsbeta2-, now that the 6/15 deadline has passed.

Nominating this for beta3, it's arguable if this is a feature or a bug, but I don't think we should release a product without this bug fixed.

A long term fix to this and several other LINK-related bugs would be to provide an API that gives a consistent interface for querying the document about all LINKs in it, including from the <link> element, the HTTP 'Link' header, and the <meta http-equiv="Link"> element. This API would then provide a callback mechanism which would fire whenever any change occured to the document's LINK and META elements (and HTTP headers, if it possible to change those dynamically). This is needed by the style system (this bug) but also for fixing bug 2800 in all cases. In fact, for the latter case this API would need to be accessible from JavaScript. cc'ing tim@prismelite.com, who needs this API for bug 2800.

Removing misleading [FEATURE] notation. This is a bug with our DOM standards support (manipulating/adding document elements through the DOM should alter the document in the same way as if the element was statically present in the first place). *** This should be an nsbeta3 stopper. *** It needs to be fixed because if it's not, we cripple the power of the DOM for dynamically styling documents by pointing them to different non-predetermined LINKed style sheets on the fly. More fundamentally, if elements dynamically added via the DOM don't have the same effect on the document as elements that were statically present, our DOM support is broken.

P.S. Netscape lacks the resources to undertake a LINK-specific set of DOM extensions for Netscape 6 FCS. However mozilla.org contributors are of course free to continue work on that idea, which can be tracked separately from this report through bug 2800.

*** Bug 43984 has been marked as a duplicate of this bug. ***

*** Bug 43984 has been marked as a duplicate of this bug. ***

Whoa, whoa, whoa... When did we decide DOM compliance would be fixed in nsbeta3? In ekrock comments from 2000-05-25 15:26 & 2000-06-26 10:36, it seems pretty clear that this is something that won't be in FCS if it's not in nsbeta2. What I don't understand is why we've changed our mind to push this off until nsbeta3... Are we declaring this a bug or a feature?

Repeating my comments of 2000-06-26: Removing misleading [FEATURE] notation. This is a bug with our DOM standards support (manipulating/adding document elements through the DOM should alter the document in the same way as if the element was statically present in the first place). *** This should be an nsbeta3 stopper. *** It needs to be fixed because if it's not, we cripple the power of the DOM for dynamically styling documents by pointing them to different non-predetermined LINKed style sheets on the fly. More fundamentally, if elements dynamically added via the DOM don't have the same effect on the document as elements that were statically present, our DOM support is broken.

Marking nsbeta3+

Not critical to Netscape 6 ship. Moving to P3. Adding [PDTP3]

Mass update of qa contact

The fact that there's no workarround for this bug IMO makes this a P2, if not even a P1. Setting to P2. I also have a fix for this that almost ready to go.

PDT thinks this is P3, looks like a feature that missed several cutoff dates, and will now have to wait for 6.0.1.

Marking nsbeta3-, relnote, helpwanted and futuring the bug.

updating summary to include <?xml-stylesheet?> as well This is needed badly for XSLT. Without this the resault from XSLT cannot be styled!

I don't know when that bug will be approved for checkin so, per jst's request, I'm going to attach some code snippets that allow to interrupt the loading of stylesheets. These are snippets, not diffs, because by the time we get approval to checkin, real diffs could become completely obsolete.

*** Bug 61335 has been marked as a duplicate of this bug. ***

*** Bug 38761 has been marked as a duplicate of this bug. ***

I'm gonna try to get this fixed for next release, I have this mostly working in one of my trees and I'll try to get the fix completed.

Taking QA Contact on all open or unverified DOM Style bugs...

Nominating nsbeta1. Would like dynamically generated content to work the same as static content.

There's another aspect to this bug that wasn't mentioned in the original description namely: - setting the href property of a LINK element to a new url doesn't cause the new stylesheet to be loaded and applied automatically. This is regardless of whether the link element existed previously or was dynamically created and inserted after load. Another scenario that works in IE 4+ is to start with a 'blank' link element in the head of the document and then as the document loads. This 'theme' url could for example be obtained from an xml document. <link href="" id="system" rel="stylesheet" type="text/css" /> I mention this because I am trying to 'port' Lotus K-station portal to run in Mozilla and am currently stuck by this inability to change a stylesheet dynamically. I've attached additional test cases.

Peter is working on this, reassigning.

Attaching two patches, one to mozilla/content to implement dynamic stylesheet loading and as an added bonus, I'm adding a small patch to mozilla/xpfe/browser/resources/content/navigator.js to fix the View->Use Stylesheet popup menu. Note that I've changed regular stylesheet loading (through the content sinks), so I hope I haven't regressed anything. I've run it through Hixie's evil test suites at www.bath.ac.uk/%7Epy8ieh/internet/importtest/ and didn't notice any regessions.

For navigator.js: just change var lastWithSameTitle; to var lastWithSameTitle = null; and you can leave the rest of the vars where they are. r=jag on the navigator.js if you only make that change.

These are basically just syntactic checks, I would need to talk to you on the phone to discuss what some methods are doing and why... 1) ParseLinkTypes() is evil, use string iterators. Read the string manual, see 6). (Oh well, you just moved it... but still it is evil.) 2) In "aValue.FindChar(PRUnichar(';'));" you can leave out the PRUnichar cast. 3) "(aValue.Length() - semiIndex) - 1" has unneeded parenthesis. 4) Do you really need the static cast here? NS_STATIC_CAST(nsIDOMLinkStyle*, this)->QueryInterface 5) This cast here feels evil, could you change one method's name if you had to avoid ambiguity? NS_STATIC_CAST(nsGenericElement*, it)->Init 6) Although this isn't your doing, could you change all the occurrences like |NS_ConvertASCIItoUCS2("stylesheet")| (where "stylesheet" could be any literal string) into |NS_LITERAL_STRING("stylesheet")| in the files you touched. I believe the latter can be more efficient. This is not necessary, though, we use the former all over the place. Although still incomplete, you might want to to read scc's string manual at http://lxr.mozilla.org/seamonkey/source/string/doc/string-guide.html 7) Replace "0 == preferredStyle.Length()" with "preferredStyle.IsEmpty()", other instances of this as well. 8) You can change pattern: |mTarget.EqualsWithConversion("xml-stylesheet")| into this: |mTarget.Equals(NS_LITERAL_STRING("xml-stylesheet"))|. In any case, this checkin is big and can affect performance and cause serious regressions; it must go in a carpool, with all the hassle that goes with it (provide builds for QA, provide builds for jrgm to run page load tests, run leak and bloat tests etc.). Look for the carpool link on Tinderbox, you can add this one there. In any case, too big for 0.9 this late.

Hmm... didn't mean to sound harsh. The bottom line: great to finally have a fix for this!

> 2) In "aValue.FindChar(PRUnichar(';'));" you can leave out the PRUnichar cast. Is this the case on compiler/cpu architectures where char is signed, or will you get a warning? Good questions on those NS_STATIC_CAST uses. I'm curious to know the answers. /be

Peter, what is the status on this? If this does not get in during this week it will have a hard time trying to get in afterwards.

Vidur landed related work yesterdya (dynamically added script elements now work). It would be great to have this too. Status?

I have fixes for your comments. I've recovered the patch from the xpcdom landing. Jst also gave me a first review and I'm trying to get changes for all his comments in. I hope to be able to attach a patch late tonight or tomorrow.

moving to TM of 0.9.2 per PDT triage (you can check it into 0.9.1 until Friday, 18/May/01 or into 0.9.2 after the tree opens)

Peter, I think this is the most important thing on your list right now. I'd really, really want to get this in before tomorrow evening. Forget everything else if you can, lets get this baby in...

Some comments on the latest patch: 1. When checking if something is true or not true, do NOT check if the value is equal or unequal to PR_TRUE or PR_FALSE. There are no quarantees that the methods will return those values. To check if something is true, you simply do: if (something) and if you want to act if it is not true, do: if (!something) At least these lines need to be changed: + if (PR_FALSE == type.Equals(textHtml)) { + (PR_FALSE == title.EqualsIgnoreCase(data))); 2. "0 < title.Length()" is hard to read, make it "!title.IsEmpty()". 3. Please format the license template in the new files as it is in the other files, because then it will be easier to change all of the license templates at once if we get MPL 1.2 etc. 4. I am concerned that your SplitMimeType() implementation does not do exactly the same thing as the old one. The old one does StripWhitespace(), but there is no method like that in the new string APIs. You are only cutting ws from the beginning and end. I'll go and get coffee, and then continue... Have you run the page load time tests? I think we need to do that. I can maybe do that after I have finished this review. I asked harishd to also have a look at the patch.

2. "0 < title.Length()" is hard to read, make it "!title.IsEmpty()". those two aren't equivalent, although i usually like seeing ! syntax...

Hmm... jst said he has builds also ready to do the page load tests, and he mentioned he send you some review comments in email... 5. Please put some null checks and debug warnings in StopLoadingSheetCallback to check that you have data, data->mLoader and data->mURL before using them, or otherwise make sure that they can never be nsnull. Also is StopLoadingSheet safe is aURL == nsnull? 6. "!prefStyle.Length()" is hard to read, "prefStyle.IsEmpty()" is easier. I guess that is all I could find. I realize some of the things I would want fixed were copied from old code so it isn't really your fault ;) But it would be nice if you could fix it.

I agree with heikki on the readability question, but wanted to add a practical performance point. Don't ask for Length() if you don't need it -- IsEmpty should be cheaper for scattered-buffer string implementations. /be

timeless: How so? Initialize() {mLength = 0;} // Also other places where mLength is manipulated Length() {return mLength;} IsEmpty() {return Length() == 0;} string Length() 0 < Length() IsEmpty() !IsEmpty() Equivalent? empty 0 0 1 0 1 "foo" 3 1 0 1 1 To me they seem equivalent.

Index: base/src/nsDocument.cpp =================================================================== RCS file: /cvsroot/mozilla/content/base/src/nsDocument.cpp,v retrieving revision 3.297 @@ -949,8 +951,32 @@ if (0 < aData.Length()) { *lastPtr = new nsDocHeaderData(aHeaderField, aData); } } + if (aHeaderField == nsHTMLAtoms::headerDefaultStyle) { + // switch alternate style sheets based on default + nsAutoString type; + nsAutoString title; + nsAutoString textHtml; textHtml.AssignWithConversion("text/html"); Move AssignWithConversion to the next line. Index: base/src/nsStyleLinkElement.cpp =================================================================== RCS file: nsStyleLinkElement.cpp void nsStyleLinkElement::ParseLinkTypes(const nsAReadableString& aTypes, + nsStringArray& aResult) +{ + nsReadingIterator<PRUnichar> current; + nsReadingIterator<PRUnichar> done; + + aTypes.BeginReading(current); + aTypes.EndReading(done); + if (current == done) + return; I prefer one exit point per function [ but not an absolute requirement :) ] +NS_IMETHODIMP +nsStyleLinkElement::UpdateStyleSheet(PRBool aNotify, nsIDocument *aOldDocument, PRInt32 aDocIndex) +{ + if (!mUpdatesEnabled) + return NS_OK; + + nsCOMPtr<nsIContent> thisContent; + QueryInterface(NS_GET_IID(nsIContent), getter_AddRefs(thisContent)); + + if (!thisContent) + return NS_ERROR_FAILURE; + + nsCOMPtr<nsIDocument> doc; + thisContent->GetDocument I prefer one exit point per function [ but not an absolute requirement :) ] Also, I noticed that some places you use IsEmtpy and and in other places you use Length(). It would be nice if it was consistent. + nsCOMPtr<nsIStyleSheetLinkingElement> ssle(do_QueryInterface(element)); + + if (ssle) { + // XXX need prefs. check here. + if(!mInsideNoXXXTag) { may be NS_IMETHODIMP HTMLContentSink::GetPref(PRInt32 aTag,PRBool& aPref) should help you out here. That is, we should try to get rid of mInsideNoXXXTag.

Forgive the unsolicited review. Bouncing off harishd's comments: + nsAutoString type; + nsAutoString title; + nsAutoString textHtml; textHtml.AssignWithConversion("text/html"); >Move AssignWithConversion to the next line. Don't use unnecessary ns*String copies at all -- in this case, why wouldn't NS_LITERAL_STRING or one of its siblings suffice here? + + mCSSLoader->SetPreferredSheet(nsString(aData)); OTOH, if aData can't be passed directly to SetPreferredSheet, shouldn't it be copied into an nsAutoString instead of a guaranteed-malloc imposed by this nsString construction? Maybe it doesn't matter if aData is always > 64 chars? One exit per function went out with Pascal, IMNSHO. Your preference is a fine style guide for code you own, not sure who owns nsDocument.cpp, but layout is full of early returns, and much less contorted by gratuitous booleans, and over-indented normal cases after the exceptions have been pruned, for it. /be

Checked in with r=heikki,harishd, sr=jst.

VERIFIED on win2k per the original test case. However, issues remain in dbaron's tests. I have contacted desale@netscape.com (DOM Core QA) regarding those.

Please see http://www.dieselnet.com/ -- it may be a test case for this bug. In the header portion click on the "Links are *underlined*" link. Repeated clicking should toggle underline links in the page back and forth (it's what is does in IE5.5/win). This is done through dynamically changing a style sheet. In Mozilla, you can do it just once (and with a nasty screen flicker). Repeated clicking doesn't change the link underline state any more.

Reopened bug 38761 to cover the dieselnet issue. (Nice page, btw!)