Closed Bug 34849 Opened 25 years ago Closed 23 years ago

dynamically added STYLE element doesn't alter style

Categories

(Core :: DOM: CSS Object Model, defect, P3)

Product:
Core
Component:
DOM: CSS Object Model
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: myk, Assigned: peterv)

References

(Blocks 1 open bug)

Details

(Keywords: dom1, Whiteboard: EDITORBASE-, [DIGBug] [ADT3])

Attachments

(3 files, 7 obsolete files)

test case
970 bytes, text/html
Details
test case v2
1.13 KB, text/html
Details
patch v2.6
56.76 KB, patch
peterv
: review+
peterv
: superreview+
roc
: approval+
Details | Diff | Splinter Review
Overview Description: Adding a STYLE element to the HEAD of a document via JavaScript does not alter the style of the document. Here's some code that adds a stylesheet that makes the background color black and the text color white: foo = document.createElement('style'); foo.setAttribute('type', 'text/css'); bar = document.createTextNode('BODY { background-color: black; color: white; }'); foo.appendChild(bar); document.getElementsByTagName('head')[0].appendChild(foo); Actual Results: document colors remain the same Expected Results: document colors change Build Date & Platform Bug Found: Linux 2000-03-27-11 Additional Builds and Platforms Tested On: Linux 2000-04-05-08 Additional Information: possibly related to bug 7515
Attached file test case —
This would be nice to have for beta2, trying M17 for now...
Status: NEW → ASSIGNED
Keywords: nsbeta2
Target Milestone: --- → M17
No way we need this for beta2, removing the keyword (which I added myself).
Keywords: nsbeta2
Putting on [nsbeta2-] radar. Not critical to beta2.
Whiteboard: [nsbeta2-]
Note related bug 7515 "dynamically adding LINK-ed style sheets does nothing."
Since there is no nsbeta2 keyword, the nsbeta2- entry in the status whiteboard also does not make any sense.
Whiteboard: [nsbeta2-]
This bug has been marked "future" because the original netscape engineer workingon this is over-burdened. If you feel this is an error, that you or another known resource will be working on this bug,or if it blocks your work in some way -- please attach your concern to the bug for reconsideration.
OS: Linux → All
Hardware: PC → All
Target Milestone: M17 → Future
Mass update of qa contact
QA Contact: gerardok → janc
Keywords: dom1
Component: DOM Level 1 → DOM Style
Taking QA Contact on all open or unverified DOM Style bugs...
QA Contact: janc → ian
Ok so there is an ugly workaround (and not very flexible) for this (but you probably figured it out by now): ************************ <head> <script type="text/javascript"> document.write("<style> BODY { background-color: black; } <\/style>"); </script> </head> ************************ OTOH : what is the problem here? Is it that we don't attempt to repaint after the style tag has been added to the head tag? Is it that adding a child to the head tag fails? Is it possible this is related to bug 18843 "dynamically added SCRIPT not executable"?
The bug is that adding a <style> element does not create an entry in document.styleSheets. So any styles that the element includes are naturally not applied...
No longer blocks: 53030
*** Bug 60841 has been marked as a duplicate of this bug. ***
Solving this bug is important for the CSSization of Composer. When the user changes embedded css rules in the source view, the changes are not reflected by Composer when back to normal mode because of the current bug. Also voting for this bug.
Blocks: 16255, 17533, 77705
Patch v 1.0. Tested successfully with (a) test case in the current bug (b) Composer adding style rules in the source view and switching back to normal view to check if the new rules are correctly applied (c) web sites using CSS embedded and external stylesheets :-) jst, can you r= please ?
Just one extra comment : the code is 100% 'inherited' from peterv's fix for bug 7515, which had r=heikki,harishd, sr=jst...
woohoo! this is very cool!! However, I have two requests: 1. It does not like adding multiple textnodes to the <style>, only the first one is honored. 2. Changes made to the <style> after it has been added to the tree is ignored I'll attach a testcase that tests both cases
I now understand the purpose of the sheetMap in CSSLoaderImpl, this code being untouched since Peter Linss made it a loooong time ago. It flattens in a single nsVoidArray all the stylesheets attached to the document wherever they come from; these sheets can be directly referenced by the doc, or imported by other sheets, etc. Having a flat array allows the CSSLoader to determine faster where to insert a sheet. This code has clearly two problems : a) the index used in CSSLoaderImpl::InsertSheetInDoc uses a bad index for insertion of the sheet into the document b) the sheetMap is never updated when a stylesheet is removed from the document Working on it and taking ownership of this bug... jst, if you disagree, please reassign to yourself.
Assignee: jst → glazman
Status: ASSIGNED → NEW
What I found looking at CSSLoader is absolutely scary : it is a real miracle, or let's say a coincidence if this code works today and never generated crashes or leaks. Still working on it.
Comment on attachment 54854 [details] [diff] [review] patch v1.0 patch v1.0 obsolete
Attachment #54854 - Attachment is obsolete: true
Comment on attachment 55924 [details] [diff] [review] patch v 2.0 obsoleting this patch just for making sicking smile ;-)
Attachment #55924 - Attachment is obsolete: true
*** Bug 83787 has been marked as a duplicate of this bug. ***
Attached patch patch v2.2 (obsolete) — — Splinter Review
new patch for this bug, fix needed by Composer and Transformiix. patch tested with XPCOM_MEM_LEAK_LOG=1 ; saw nothing special Jst, sicking, peterv : care to test and review ?
Attachment #55925 - Attachment is obsolete: true
Comment on attachment 66875 [details] [diff] [review] patch v2.2 To avoid copying the text of the entire stylesheet *twice*, could you change this code to, instead of using |nsAutoString content|, allocate the nsString object up front and append directly to it? Also, the return value check should be |NS_FAILED(rv)| rather than |NS_OK != rv|. >+ nsCOMPtr<nsIUnicharInputStream> uin; >+ nsAutoString content; >+ nsCOMPtr<nsIDOMNode> tcNode; >+ GetFirstChild(getter_AddRefs(tcNode)); >+ if (!tcNode) return NS_OK; >+ while (tcNode) { >+ nsCOMPtr<nsIDOMText> tc = do_QueryInterface(tcNode); >+ if (!tc) return NS_OK; Should this be |continue| instead? Or an assertion? Does this handle CDATA sections? >+ nsAutoString tcString; >+ tc->GetData(tcString); >+ nsCOMPtr<nsIDOMNode> tmp; >+ tcNode->GetNextSibling(getter_AddRefs(tmp)); >+ tcNode = tmp; >+ if (tcNode) { >+ content.Append(PRUnichar('\n')); >+ } >+ content.Append(tcString); >+ } >+ if (content.IsEmpty()) return NS_OK; >+ PRBool doneLoading; >+ rv = NS_NewStringUnicharInputStream(getter_AddRefs(uin), new nsString(content)); >+ if (NS_OK != rv) { >+ return rv; >+ }
Very good catches, thank you David. About CDATA : this code clearly misses to handle comments ! Producing a new patch asap.
Status: NEW → ASSIGNED
Attached patch patch v2.3 (obsolete) — — Splinter Review
Attachment #66875 - Attachment is obsolete: true
I don't think this should be in Future milestone ;) Tentatively moving to 0.9.9. This is blocking some serious features...
Keywords: nsbeta1
Target Milestone: Future → mozilla0.9.9
one again : jst, sicking, peterv : reviews/test pls ?
I have a few regarding nsHTMLStyleElement.cpp: in the nsIContent methods, you ignore the rv of the nsGenericHTMLContainerElement calls, then call UpdateStyleSheet without catching that rv, and return the rv of the nsGHCE call. Is that wrong, or does it serve a purpose? Should you update the stylesheets if the nsGHCE call failed? If you can't get the loader in UpdateStyleSheet, you return NS_OK. I see that that's copy'n'paste, but is it right? UpdateStyleSheet shouldn't be inline, IMHO. And at least the indention is wrong. And cut the line lengths down to something below 80 chars.
You've moved most of the logic from the content-sink to the element, but some things now live in both places. Checking the src/title/media argument is now done in both places and checking type is done in content-sink and half-done in the element (the element doesn't contain the mimeType.EqualsIgnoreCase("text/css") test that the contentsink does). IMHO all the logic should live in the element, except that the content-sink might need to handle the default-style-title. The spec doesn't give any special meaning to the title, but i'm not sure if there are any special reasons why we might wanna do so anyway. I'd prefer if you did if (!src.IsEmpty()) return NS_OK; rather then if (src.IsEmpty()) { // Whole lot'a code here } but that's style. We should ignore comment-nodes for xhtml <style>s, but that's a different bug. Iterating using GetNextSibling is O(n^2) and usually get jst a bit upset, but since this should not be that many nodes we're iterating it might be ok. + nsIDocument *oldDoc = mDocument; + nsresult rv = nsGenericHTMLContainerElement::AppendChildTo(aKid, aNotify, + UpdateStyleSheet(aNotify, oldDoc); Why not just? + nsresult rv = nsGenericHTMLContainerElement::AppendChildTo(aKid, aNotify, + UpdateStyleSheet(aNotify, mDocument); + nsCOMPtr<nsIDocument> doc; + GetDocument(*getter_AddRefs(doc)); just use mDocument You don't need to handle the case when the parent is a document, <style> should never be the root element. You should use aDocIndex when >= 0 since you return rv, would you mind doing |nsresult rv = NS_OK;|? Just to be safe. (or return NS_OK) I'm not sure if you should only call nsStyleLinkElement::UpdateStyleSheet only when src is set. It seems so, but i'm not entierly sure
oh, wait, comments in <style> in html gets created as textnodes, so just QI to nsIDOMText instead of nsIDOMCharacterData and you should be safe (nsIDOMCDATASection inherits nsIDOMText so CDATAs will be ok) You need to remove some things from the xml-contentsink as well: mStyleText and much of ::ProcessSTYLETag
Marking nsbeta1+
Keywords: nsbeta1 → nsbeta1+
nominating this bug for EDITORBASE given its impact on Composer (see Comment #13 below). Peterv's gonna work on it this week-end if he can and I'll take it back on monday if needed.
Whiteboard: EDITORBASE
*** Bug 125710 has been marked as a duplicate of this bug. ***
dup is a DIGBug. A few thoughts I had while looking over the patch: 1) We're duplicating all the insertion point logic... Wouldn't it make more sense to make GetStyleSheetInfo set a boolean that indicates inline style and then make some slight tweaks to nsStyleLinkElement::UpdateStyleSheet to handle the creation of the string and the calling of ParseInlineStyle? 2) + if (aNotify && mStyleSheet && !doc && aOldDocument) { This is already handled by nsStyleLinkElement::UpdateStyleSheet 3) + nsString * content = new nsString(); + if (!tcNode) return NS_OK; + if (!tc) return NS_OK; + if (content->IsEmpty()) return NS_OK; + rv = NS_NewStringUnicharInputStream(getter_AddRefs(uin), content); + if (NS_FAILED(rv)) { + return rv; + } Any of those returns will leak |content| 4) You should use aDocIndex for the insertion point unless that's -1. The content sink knows best during page loads.... 5) You should not be blocking the parser for alternate inline sheet loads.... currently you are.
Whiteboard: EDITORBASE → EDITORBASE, DIGBug
Oh, one more problem. If one creates a <style> element and appends it into the document we should create a stylesheet (an empty)... we do not with that patch because there are no child text nodes....
Blocks: 125746
Blocks: 125710
Taking from Daniel. I'll attach a new patch rsn.
Assignee: glazman → peterv
Status: ASSIGNED → NEW
Not EDITORBASE
Whiteboard: EDITORBASE, DIGBug → EDITORBASE-, DIGBug
Target Milestone: mozilla0.9.9 → mozilla1.0
oh, according to bug 82829 we ignore comments in <script>s so it should be safe to do it in <style> too.
ADT3 per ADT triage.
Whiteboard: EDITORBASE-, DIGBug → EDITORBASE-, DIGBug [ADT3]
Will probably have a new patch tomorrow. Tracking down a weird bug with a missing loadgroup for a stylesheet. I doubt I cause it.
Status: NEW → ASSIGNED
Attached patch patch v2.4 (obsolete) — — Splinter Review
This one should take care of the comments made. Still going over it myself but I think it's ready for review.
Attachment #66888 - Attachment is obsolete: true
// The way we determine the stylesheet's position in the cascade is by lookin - // at the first of the previous siblings that are style linking elements, and + // at the first of the next siblings that are style linking elements, and // insert just after that one. I'm not sure this is correct for every case fo change "after" to "before" + if (isInline) { + PRInt32 count; + thisContent->ChildCount(count); + if (count < 0) + return NS_OK; return for count <= 0 while you're at it gotta get sleep, more to come
Regarding the count, see comment 39 and http://bugzilla.mozilla.org/show_bug.cgi?id=125710. I'll leave count < 0.
It sure would be great to have this for 1.0... I'm itching to a= it!
Comment on attachment 75578 [details] [diff] [review] patch v2.4 Have you run page load tests? >Index: base/src/nsStyleLinkElement.cpp >=================================================================== >- // at the first of the previous siblings that are style linking elements, and >+ // at the first of the next siblings that are style linking elements, and You changed this; I don't know enough about the CSS cascading rules to say this or that but are you sure this is correct? >+ while (++index && index < count) { This looks a bit strange. The loop skips case where index==0, is that intentional? If it is intentional, please comment why. In any case, below (either prefix or postfix depending on which is correct) is how I would have written it and it is also slightly more efficient as it avoids one test: + if (index >= 0) { + while (++index < count) { >+ nsCOMPtr<nsIDOMText> tc = do_QueryInterface(node); >+ // Ignore nodes that are not DOMText. >+ if (!tc) { >+ nsCOMPtr<nsIDOMComment> comment = do_QueryInterface(node); >+ if (comment) >+ continue; >+ else >+ break; >+ } You missed CDATA sections. >Index: html/content/src/nsHTMLLinkElement.cpp >=================================================================== >+nsHTMLLinkElement::GetStyleSheetURL(PRBool* aIsInline, >+ nsAString& aUrl) > { >- nsresult rv = NS_OK; >- >+ *aIsInline = PR_FALSE; > aUrl.Truncate(); >+ GetHref(aUrl); >+ return; >+} Please check GetHref() impls; I am pretty sure they also do Truncate(). If they don't, I think you should fix them instead. Also, if GetHref() returns empty for inline stylesheets, aIsInline parameter is redundant. What does GetHref() do? I would also suggest adding NS_ENSURE_ARG if aIsInline is indeed needed in case someone passes a null to us. That holds true for other functions, like GetStyleSheetInfo() as well, so please check all new functions. >Index: html/document/src/nsHTMLContentSink.cpp >=================================================================== >+ if (NS_OK == rv) { NS_SUCCEEDED(rv) >+ nsCOMPtr<nsIDOMText> tc = do_QueryInterface(text, &rv); >+ if (NS_OK == rv) { >+ tc->SetData(content); >+ } >+ element->AppendChildTo(text, PR_FALSE, PR_FALSE); >+ text->SetDocument(mDocument, PR_FALSE, PR_TRUE); >+ } I don't think we can have CDATA sections in HTML so this is probably ok. But there are some people working on supporting them, dunno if that includes changes in DOM or if we just convert them to text in the sink/parser. Other than those it looks really good! It was cool that you noticed some code reuse opportunities and spotted and fixed some methods that had reinvented existing methods.
Attachment #75578 - Flags: needs-work+
>+ nsCOMPtr<nsIDOMText> tc = do_QueryInterface(text, &rv); >+ if (NS_OK == rv) { Oops, one more nit: never ever compare XPCOM method return value to NS_OK (unless you were copying some old code that may have depended on that). Do NS_SUCCEEDED or NS_FAILED instead.
there's a break after else after continue :-) i'd suggest removing it before brendan finds it ;-)
> You missed CDATA sections. Nope: class NS_NO_VTABLE nsIDOMCDATASection : public nsIDOMText
- GetStyleSheetInfo(url, title, type, media, &isAlternate); + GetStyleSheetURL(&isInline, url); + if (!url.IsEmpty() || isInline) { + GetStyleSheetInfo(title, type, media, &isAlternate); + } you could wait with the GetStyleSheetInfo call to right before the type.EqualsIgnoreCase("text/css") |if| i think. if (mStyleSheet && url.IsEmpty()) { // Inline stylesheets have the document's URL as their URL internally nsCOMPtr<nsIURI> docURL, styleSheetURL; mStyleSheet->GetURL(*getter_AddRefs(styleSheetURL)); doc->GetBaseURL(*getter_AddRefs(docURL)); if (docURL && styleSheetURL) { PRBool inlineStyle; docURL->Equals(styleSheetURL, &inlineStyle); if (inlineStyle) { return NS_OK; } } } This |if| should be removed, afaict it should make any changes to an inline- stylesheet break, so i suspect there is something that doesn't work. Anyway, now that we handle inline stylesheets properly it should be gone with. - // at the first of the previous siblings that are style linking elements, and + // at the first of the next siblings that are style linking elements, and What's the reason for turning this around? more to come...
nsresult rv = nsGenericHTMLLeafElement::SetAttr(aNameSpaceID, aName, aValue, aNotify); - UpdateStyleSheet(aNotify); + if (NS_SUCCEEDED(rv)) { + UpdateStyleSheet(); + } this'll make UpdateStyleSheet be called with aOldDocument==null, so we won't remvoe the stylesheet from the document at the top of UpdateStyleSheet. Does the CSS-loader handle this for us or?
in nsHTMLLinkElement: nsresult rv = nsGenericHTMLLeafElement::SetAttr(aNameSpaceID, aName, aValue, aNotify); - UpdateStyleSheet(aNotify); + if (NS_SUCCEEDED(rv)) { + UpdateStyleSheet(); + } should this be |if (NS_SUCCEEDED(rv) && aNotify)|? same in UnSetAttr +nsHTMLLinkElement::GetStyleSheetURL(PRBool* aIsInline, + nsAString& aUrl) { - nsresult rv = NS_OK; - + *aIsInline = PR_FALSE; aUrl.Truncate(); + GetHref(aUrl); + return; +} the old code had some base-url handling: - nsCOMPtr<nsIURI> baseURL; - GetBaseURL(*getter_AddRefs(baseURL)); - rv = NS_MakeAbsoluteURI(aUrl, href, baseURL); which i think you still need, same in nsHTMLStyleElement + if (!*aIsAlternate && !aTitle.IsEmpty()) { // possibly preferred sheet + nsAutoString prefStyle; + mDocument->GetHeaderData(nsHTMLAtoms::headerDefaultStyle, prefStyle); + + if (prefStyle.IsEmpty()) { + mDocument->SetHeaderData(nsHTMLAtoms::headerDefaultStyle, aTitle); IMHO this shouldn't live in a GetFoo-type function, however since it did in the old code too it's ok to live with it for now. However I'm a bit worried that since you've split it up into two functions the second might not always be called, and therefor the code isn't always ran. Please check if that could be a problem. in nsHTMLStyleElement + NS_IMETHOD InsertChildAt(nsIContent* aKid, PRInt32 aIndex, PRBool aNotify, PRBool aDeepSetDocument) + { + nsresult rv = nsGenericHTMLContainerElement::InsertChildAt(aKid, aIndex, aNotify, aDeepSetDocument); + if (NS_SUCCEEDED(rv)) { + UpdateStyleSheet(); + } should this be |if (NS_SUCCEEDED(rv) && aNotify)|? same applies to the other child-add/replace functions +nsHTMLStyleElement::GetStyleSheetURL(PRBool* aIsInline, + nsAString& aUrl) { trunkate aUrl in the beginning in case it's an inline-stylesheet. just the contentsink left
+ if (NS_OK == rv) { + nsCOMPtr<nsIDOMText> tc = do_QueryInterface(text, &rv); + if (NS_OK == rv) { tssk tssk, use NS_SUCCEEDED instead. And use |if (tc)| on the second. + tc->SetData(content); i think AppendData is faster. + element->AppendChildTo(text, PR_FALSE, PR_FALSE); + text->SetDocument(mDocument, PR_FALSE, PR_TRUE); does AppendChildTo call SetDocument for you? - nsresult ProcessSTYLETag(); WOOHOO!! :-) can you remove mStyleElement too? ok, i'm done
> this'll make UpdateStyleSheet be called with aOldDocument==null, so we won't > remvoe the stylesheet from the document at the top of UpdateStyleSheet. Does > the CSS-loader handle this for us or? We don't want to remove it from an old document, that should only happen when the element is removed froma document or moved to a different document. > should this be |if (NS_SUCCEEDED(rv) && aNotify)|? same in UnSetAttr aNotify shouldn't affect stylesheet loading. I've verified that all the url getters correctly use the base url. No need to do it twice. Other comments addressed as discussed. New patch coming up.
Attached patch patch v2.5 (obsolete) — — Splinter Review
Attachment #75578 - Attachment is obsolete: true
- if (aNotify && mStyleSheet && !doc && aOldDocument) { + if (mStyleSheet && !doc && aOldDocument) { i think you should remove the !doc too. If an element is moved from one document to another doc will point to the new document, but we still want to remove it from the old document. - if (NS_OK == rv) { + if (NS_SUCCEEDED(rv)) { nsCOMPtr<nsIDOMText> tc = do_QueryInterface(text, &rv); - if (NS_OK == rv) { + if (tc) { please remove the &rv argument to the QI + nsCOMPtr<nsIContent> content = do_QueryInterface(element); + if (content && + !content->HasAttr(kNameSpaceID_None, nsHTMLAtoms::src)) { // The skipped content contains the inline style data const nsString& content = aNode.GetSkippedContent(); IMHO it would be nicer if you always got the skipped content and |if|ed on it being IsEmpty instead. But that's optional with those r=sicking
Comment on attachment 76347 [details] [diff] [review] patch v2.5 - If performance of nsParserUtils::GetQuotedAttributeValue() matters it could be made faster if it returned an nsDependentSubstring by value, thus avoiding the copying of the attribute value. I didn't really find anything wrong with the patch worth mentioning here (except for some unnecessary .get()'s on some nsCOMPtr's), so sr=jst
Attachment #76347 - Flags: superreview+
I am currently doing performance tests. I'll attach the latest patch tomorrow and request a= then.
Speaking as one driver, I'm eager to approve this :-).
Pageload numbers without patch: Test id: 3CAAF14401 Avg. Median : 2638 msec Minimum : 606 msec Average : 2650 msec Maximum : 9219 msec Pageload numbers with patch: Test id: 3CAACD35E1 Avg. Median : 2667 msec Minimum : 597 msec Average : 2684 msec Maximum : 9147 msec
Attached patch patch v2.6 — — Splinter Review
Attachment #76347 - Attachment is obsolete: true
Comment on attachment 77441 [details] [diff] [review] patch v2.6 Carrying forward r and sr.
Attachment #77441 - Flags: superreview+
Attachment #77441 - Flags: review+
Ran through Hixie's import test suite (basic and evil), no regressions. Surfed around a bit and looked for leaks, I am not seeing more leaks after the patch.
Peter did you run the layout regression tests? http://www.mozilla.org/newlayout/regress.html catch me on IRC if you need help.
No, I did not. This patch only affects loading of stylesheets and Hixie's test suite does extensive testing of that. The patch touches nothing in layout.
That is about 1% perf hit on page load, but peterv is located in France and run the tests just once. I am compiling an optimized build and will run the tests locally several times to see if peterv's result was noise. In the mean time, try to see if there is anything that could cause the slowdown (if it is real) and how to improve.
I can't really find any sigificant difference in the amount of work we do. In fact most changes should make us *faster*. One easy way to save a few cycles would be to make UpdateStyleSheet non-virtual in nsStyleLinkElement.h, that way it is call non-virtually from all nodes that inherit nsStyleLinkElement
The last patch is strange, did you edit it by hand? It does not apply cleanly on Unix/Windows; it appears it is 3 diffs concatenated because there are 3 different "root" directories. It was easy to fix, but still... wanna make sure you did not miss anything. Then the perf numbers (RH Linux 7.2, 384MB RAM, 500MHz, opt build): TRUNK1 Test id: 3CAB96B4C9 Avg. Median : 1265 msec Minimum : 228 msec Average : 1296 msec Maximum : 4654 msec TRUNK2 Test id: 3CAB996952 Avg. Median : 1258 msec Minimum : 228 msec Average : 1269 msec Maximum : 4228 msec TRUNK3 Test id: 3CAB9F9AB4 Avg. Median : 1262 msec Minimum : 227 msec Average : 1273 msec Maximum : 4234 msec STYLE1 Test id: 3CABA8D74E Avg. Median : 1260 msec Minimum : 216 msec Average : 1287 msec Maximum : 4526 msec STYLE2 Test id: 3CABAB0E55 Avg. Median : 1256 msec Minimum : 225 msec Average : 1269 msec Maximum : 4271 msec STYLE3 Test id: 3CABAD0336 Avg. Median : 1265 msec Minimum : 233 msec Average : 1275 msec Maximum : 4244 msec Trunk avg med 1262 Trunk avg avg 1279 Style avg med 1260 (0.2% improvement) Style avg avg 1277 (0.2% improvement)
Thanks Heikki. Yeah, the diff was concatenated manually, MacCVS usually doesn't care about that. I try to make sure that the diffs are correct for other platforms too ;-) but I forgot to attach that one here.
Keywords: adt1.0.0
adt1.0.0+ (on ADT's behalf) for checkin approval into 1.0, pending greenlight Gerardo.
No longer depends on: 21771
Keywords: adt1.0.0 → adt1.0.0+
Whiteboard: EDITORBASE-, DIGBug [ADT3] → EDITORBASE-, [DIGBug] [ADT3]
no bugs out of the ordinary for smoketests on the above build (same as this mornings build that is) Heikki, is there a particular test page to make certain this works on?
Well, there are several test cases which don't work without this patch. But you don't need to test those: they work (otherwise we would not be checking this in). Thanks for testing!
Gerardo said this is ok. He will also get people to do extensive testing as soon as you have checked in so we will notice regressions soon if there are any.
It's pretty obvious to determine if the fix didn't work, because style wouldn't be applied on any page, or even the UI. We think the fix can be checked-in now. We'll be running CSS and Layout tests on 4/6. According to Heikki, if any regression is found it'll be easy to pull back the code.
Checked in. Pageload on btek went up slightly and went down slightly on sleestack. I'll take that as a yes.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
No longer depends on: 107567
After this bug was checked in I tested the tables and frames with CSS for regressions. It appears that this fix dint cause any regression in these areas.
VERIFIED FIXED on trunk
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: