Open Bug 240080 Opened 20 years ago Updated 2 years ago

Implement CSS Editing Object Model

Categories

(Core :: DOM: CSS Object Model, enhancement, P5)

enhancement

Tracking

()

People

(Reporter: glazou, Unassigned)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 1 obsolete file)

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.
Status: NEW → ASSIGNED
Attached patch First candidate patch (obsolete) — Splinter Review
This candidate patch is fully functional but not optimized/verified.
I already use it in Nvu and it's very very cool.
If the string list allocation fails, this code crashes, no?
(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.
Attached patch better fixSplinter Review
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
Comment on attachment 146257 [details] [diff] [review]
better fix

bz, could you review please ? Thanks a lot.
Attachment #146257 - Flags: review?(bzbarsky)
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 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-
QA Contact: ian → general
QA Contact: general → style-system
Assignee: daniel → nobody
Status: ASSIGNED → NEW
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
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: