Open
Bug 240080
Opened 20 years ago
Updated 2 years ago
Implement CSS Editing Object Model
Categories
(Core :: DOM: CSS Object Model, enhancement, P5)
Core
DOM: CSS Object Model
Tracking
()
NEW
People
(Reporter: glazou, Unassigned)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file, 1 obsolete file)
25.86 KB,
patch
|
bzbarsky
:
review-
|
Details | Diff | Splinter Review |
Implement the CSS Editing Object Model (see URL above). This is needed for direct class assignment to the selection (bug 16255) in the editor. Patch pending.
Reporter | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 1•20 years ago
|
||
This candidate patch is fully functional but not optimized/verified. I already use it in Nvu and it's very very cool.
Comment 2•20 years ago
|
||
If the string list allocation fails, this code crashes, no?
Reporter | ||
Comment 3•20 years ago
|
||
(In reply to comment #2) > If the string list allocation fails, this code crashes, no? Boris: right. I said not optimized, not verified. But the basis is here.
Reporter | ||
Comment 4•20 years ago
|
||
This is a better patch. I am ok to put #ifdef MOZ_STANDALONE_COMPOSER everywhere but I feel this could be useful (a) for Midas and other self-modifyable web pages (b) for tests.
Attachment #145757 -
Attachment is obsolete: true
Reporter | ||
Comment 5•20 years ago
|
||
Comment on attachment 146257 [details] [diff] [review] better fix bz, could you review please ? Thanks a lot.
Attachment #146257 -
Flags: review?(bzbarsky)
Comment 6•20 years ago
|
||
I won't be able to get to a review for at least a week... At first glance, though, this interface needs some documentation (like clearly explaining what it actually does). The question I raised in your blog still stands. Why should this be on a selector-specific interface as opposed to a more general CSSOM interface (like nsIDOMDocumentCSSStyle or something)?
Comment 7•20 years ago
|
||
Comment on attachment 146257 [details] [diff] [review] better fix Marking r- in the hope of getting a response to comment 6 and because of the problems listed below: >Index: dom/public/nsIDOMClassInfo.h >+ eDOMClassInfo_CSSSelectorQuery_id, Please add this at the end of the list like the comments say to do. >Index: content/html/style/src/nsCSSStyleRule.cpp >+ mAtom->ToString(atomString); >+ classString.Append(atomString); I think it would be better to get the UTF8 string from the atom (no copy) and then AppendUTF8toUTF16. Saves a string copy... >+nsCSSSelector::GetSelectorList(PRUint32 aSelectorFilter, nsIDOMDOMStringList >+ mTag->ToString(tagString); This clobbers the namespace prefix, no? I don't think you want that. >+ nsCOMPtr<nsDOMStringList> list = do_QueryInterface(aStringList); I'm not sure how this is supposed to work, exactly... You're not QIing to an interface here. >+ mClassList->ToDOMStringList(NS_LITERAL_STRING("."), aStringList); It makes sense to call ToDOMStringList AppendToDOMStringList instead. Perhaps the first arg should be a PRUnichar, not an nsAString? Why is this code passing around the sheet? It just wants the nsINameSpace, so it should pass that around, I think. This code doesn't seem to differentiate between negated and non-negated simple selectors. This should probably be clearly documented in the interface. >+DOMCSSStyleRuleImpl::GetSelectorList(PRUint32 aSelectorFilter, nsIDOMDOMStringList ** aStringList) >+{ >+ if (!Rule()) { >+ return NS_OK; You need to null out *aStringList before returning here. >+ *aStringList = list; >+ NS_IF_ADDREF(*aStringList); No need for the IF part. "list" cannot be null here. Just do: NS_ADDREF(*aStringList = list); Given this function, why doesn't the rule's GetSelectorList just take an nsDOMStringList* instead of taking nsIDOMDOMStringList*? Since the rule has to add things to the list, that would make a lot of sense to me... >+CSSStyleRuleImpl::GetSelectorList(PRUint32 aSelectorFilter, nsIDOMDOMStringList * aStringList) >+ nsCSSSelectorList * selectorList = mSelector; >+ while (selectorList) I think this wants to be a for loop. >Index: content/html/style/src/nsICSSStyleRuleDOMWrapper.h >+class nsICSSStyleRuleDOMWrapper : public nsISupports Why this change? This makes all DOM style rules 4 bytes bigger; is there really a good reason for it? >Index: content/html/style/src/nsCSSStyleSheet.cpp >@@ -1002,6 +1014,7 @@ >+ Why this change? >+CSSStyleSheetImpl::GetSelectorListInternal(PRUint32 aSelectorFilter, nsIDOMDOMStringList * aStringList, nsIDOMCSSRuleList * aRuleList) Wouldn't a lot of this logic be better off in the relevant rules themselves? Then new rule impls would not require changes to this code (eg see @-moz-document rules). >+CSSStyleSheetImpl::GetSelectorList(PRUint32 aSelectorFilter, nsIDOMDOMStringList ** aStringList) >+ if (nsnull == mRuleCollection) { !mRuleCollection, please. Any reason this code isn't just using GetRules()? >+ *aStringList = list; >+ NS_IF_ADDREF(*aStringList); Again, "list" can't be null here. >Index: content/html/style/public/nsICSSStyleSheet.h >+ NS_IMETHOD GetSelectorListInternal(PRUint32 aSelectorFilter, nsIDOMDOMStringList * aStringList, Weird indent. >Index: content/base/src/nsDocument.cpp >+ PRInt32 sheetCount = GetNumberOfStyleSheets(false); The GetNumberOfStyleSheets() call no longer needs a boolean (and needed a PRBool before anyway; the code as written would break some platforms). >+ nsIStyleSheet *sheet = GetStyleSheetAt(sheetIndex, PR_FALSE); Again, no need for the PR_FALSE >+ *aStringList = list; >+ NS_IF_ADDREF(*aStringList); Again, no need for the "IF" part.
Attachment #146257 -
Flags: review?(bzbarsky) → review-
Updated•15 years ago
|
QA Contact: ian → general
QA Contact: general → style-system
Comment 8•6 years ago
|
||
https://bugzilla.mozilla.org/show_bug.cgi?id=1472046 Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5. If you have questions, please contact :mdaly.
Priority: -- → P5
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•