Implement CSS Editing Object Model

NEW
Unassigned

Status

()

Core
DOM: CSS Object Model
--
enhancement
13 years ago
5 years ago

People

(Reporter: glazou, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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

13 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 1

13 years ago
Created attachment 145757 [details] [diff] [review]
First candidate patch

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?
(Reporter)

Comment 3

13 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

13 years ago
Created attachment 146257 [details] [diff] [review]
better fix

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

13 years ago
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)?
(Reporter)

Updated

13 years ago
Blocks: 255016
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

Updated

5 years ago
Assignee: daniel → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.