Open Bug 1086641 Opened 10 years ago Updated 2 years ago

convert editor styles from _moz_anonclass to :-moz-native-anonymous

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

People

(Reporter: dbaron, Unassigned, Mentored, NeedInfo)

References

Details

Attachments

(2 files, 4 obsolete files)

1086441-moz-anonclass-2.diff
36.53 KB, patch
dbaron
: review-
Details | Diff | Splinter Review
1086441-create-anon-el-3.diff
5.51 KB, patch
cojo
: review+
Details | Diff | Splinter Review 
We should convert the editor styles that use _moz_anonclass:
http://mxr.mozilla.org/mozilla-central/search?string=moz_anonclass
to use :-moz-native-anonymous so that they're not exposing magically-named styles to content.
This should look a lot like the patch in bug 1088179.
This looks like a good candidate for a good-first-bug. What do you think?
Perhaps not -- it requires patching both C++ and CSS, and understanding how they relate to each other.
Would it make sense to break this into smaller patches to tackle different instances of _moz_anonclass usage? I'd be interested in tackling a subset of the usage (maybe starting with replacing all instances of mozResizer) if it made a better candidate for a mentored bug (and someone was willing to mentor it).
Unsure whether splitting this patch is the best approach so I went ahead and began converting a few styles. This is still pretty rough, but it's a first pass on converting mozResizer and mozGrabber.

Some questions:
1) Currently anonymous elements are created like [1]. Note that in the patch attached I changed the third argument of CreateAnonymousElement to be an empty string (passing a non-empty string will add the _moz_anonclass attribute with the value being the passed string). Is there an alternative to this, or is it the best method for creating anonymous elements without the _moz_anonclass attribute?

2) I changed the check in [2] to use [3] since getting rid of _moz_anonclass means we'll need another way of detecting whether an element is anonymous.

3) I changed [4] to use |Find| instead of |EqualsLiteral| because I moved mozResizer and mozGrabber to values of the class attribute (as "moz-resizer" and "moz-grabber", respectively). I was unsure if the class attribute would always be exactly equal to something like 
> class="moz-resizer" 
> (what if another class is added? ie: class="moz-resizer another-class")
so |Find| seemed like a safer choice.

I was going to continue cleaning up this patch as well as convert the rest of the _moz_anonclass styles if this patch made sense. Just wanted to solicit feedback early on.

[1] http://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLObjectResizer.cpp#152
[2] http://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLObjectResizer.cpp#588
[3] http://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditor.cpp#4959
[4] http://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLObjectResizer.cpp#594
Attachment #8515749 - Flags: feedback?(dbaron)
Splitting it up is fine.  I'll take a closer look at the patch tomorrow.
Comment on attachment 8515749 [details] [diff] [review]
1086641-convert-styles-first-pass.diff

This looks pretty good.  A few comments:

>Convert editor styles using _moz_anonclass to use :-moz-native-anonymous.

Put the bug number at the beginning, and perhaps be a little more specific about which styles you're converting since this isn't all of them.

> nsresult
> nsHTMLEditor::CreateResizer(nsIDOMElement ** aReturn, int16_t aLocation, nsIDOMNode * aParentNode)
> {
>+  // Empty string as 3rd argument avoids adding a _moz_anonclass
>+  // to the element.
>   nsresult res = CreateAnonymousElement(NS_LITERAL_STRING("span"),
>                                         aParentNode,
>-                                        NS_LITERAL_STRING("mozResizer"),
>+                                        NS_LITERAL_STRING(""),
>                                         false,
>                                         aReturn);
> 
>   NS_ENSURE_SUCCESS(res, res);
>   NS_ENSURE_TRUE(*aReturn, NS_ERROR_FAILURE);
> 
>+  res = (*aReturn)->SetAttribute(NS_LITERAL_STRING("class"), NS_LITERAL_STRING("moz-resizer"));
>+

To avoid having to add a separate SetAttribute call, it would probably be a good idea to (in a separate patch, preceding this one) add a parameter to CreateAnonymousElement with the class attribute to set.  That will keep the code clean, especially once they're all converted to class.



I think you're also going to need to change the code that sets the mozGrabber anon class in nsHTMLAbsPosition.cpp.


What did you do to test this patch?  (It's probably good to test that the elements are still functioning -- e.g., by checking that having half the patch breaks them, and then that the complete patch makes them work again.  It's also probably worth running the relevant automated tests, but I can push the patch to the try server for you if you need that -- though it might be worth running the editor mochitests locally first.)


Also, I'd probably avoid a variable called _class when you can use classAttr or similar.
Attachment #8515749 - Flags: feedback?(dbaron) → feedback+
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) (away/busy Oct. 24-31) from comment #8)
> I think you're also going to need to change the code that sets the
> mozGrabber anon class in nsHTMLAbsPosition.cpp.

Er, never mind, that change was in there; I just missed it.
Oh -- should have mentioned -- can you post a revised patch addressing those comments, and request review? on it?
> To avoid having to add a separate SetAttribute call, it would probably be a good idea to (in a separate patch, preceding this one) add a parameter to CreateAnonymousElement with the class attribute to set. 

Should a separate ticket be opened for this or just add a separate patch to this ticket?

> What did you do to test this patch?
I ended up creating a small test snippet of html with a resizable image on it that I tested locally with the build (manually. I made sure the image had the correct css for resizing, that clicking and dragging to resize worked, etc). Do you know which tests are relevant off the top of your head? (I'll dig around a bit and see if I can figure it out on my own).

> Oh -- should have mentioned -- can you post a revised patch addressing those comments, and request review? on it?
Yep, will do.
(In reply to Connor [:cojo] from comment #11)
> > To avoid having to add a separate SetAttribute call, it would probably be a good idea to (in a separate patch, preceding this one) add a parameter to CreateAnonymousElement with the class attribute to set. 
> 
> Should a separate ticket be opened for this or just add a separate patch to
> this ticket?

A separate patch in this bug is fine.

> > What did you do to test this patch?
> I ended up creating a small test snippet of html with a resizable image on
> it that I tested locally with the build (manually. I made sure the image had
> the correct css for resizing, that clicking and dragging to resize worked,
> etc). Do you know which tests are relevant off the top of your head? (I'll
> dig around a bit and see if I can figure it out on my own).

Perhaps worth checking that you're testing what you think you are by explicitly breaking the styles (e.g., trying with the C++ half of the change but not the CSS half) and then making sure it breaks with one half and works again with both halves.

I'm really not sure which tests would be relevant, but maybe something in editor?  It seems low-risk, though, so we could just use the try server to check.
Attached patch 1086441-create-anon-el.diff (obsolete) — — Splinter Review 
Apologies for the delay. This patch should satisfy:
> To avoid having to add a separate SetAttribute call, it would probably be a
> good idea to (in a separate patch, preceding this one) add a parameter to
> CreateAnonymousElement with the class attribute to set.  That will keep the
> code clean, especially once they're all converted to class.

CreateAnonymousElement is only used in a handful of files [1] all of which are modified for this bug. So instead of adding another argument I modified the 3rd argument (what would normally set "_moz_anonclass") to set the "class" attribute.

[1] http://mxr.mozilla.org/mozilla-central/search?string=createAnonymousElement
Attachment #8532361 - Flags: review?(dbaron)
Attached patch 1086441-moz-anonclass.diff (obsolete) — — Splinter Review 
I realized that when working on this patch it was easier to separate it out into two parts. The first part is the modification to CreateAnonymousElement, the second part is all the necessary changes to migrate from "_moz_anonclass" to ":-moz-native-anonymous".

I didn't mark this patch for review because there's currently an issue with the conversion of mozGrabber [1]. When setting an element to be absolutely positioned, it correctly get's a "grabber" style applied, but it doesn't respond to mouse events (when trying to grab the "grabber" and move the element nothing happens. It also causes issues with mozResizer. An element will correctly resize unless it is absolutely positioned, at which point the correct styles are applied (all resizers show up and the "grabber" shows up) but it doesn't respond to mouse events)

I haven't yet checked to see which tests would be appropriate for this patch. I'd like to fix mozGrabber first. I did want to get some feedback on it just to make sure I haven't done anything too odd.

Also, I can separate this patch out into smaller chunks if that makes more sense. I think a more appropriate separation would be all styles related to tables [2] and whatever styles are left over (resizing and grabbing).

[1] http://mxr.mozilla.org/mozilla-central/search?string=mozGrabber
[2] http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLInlineTableEditor.cpp
Attachment #8515749 - Attachment is obsolete: true
Attachment #8532362 - Flags: feedback?(dbaron)
Comment on attachment 8532361 [details] [diff] [review]
1086441-create-anon-el.diff

>Bug 1086641 - CreateAnonymousElement is used to create anonymous elements in the DOM. The third argument of CreateAnonymousElement adds an attribute to to the created element: "_moz_anonclass". Editor styles that currently use "_moz_anonclass" need to be converted to use :-moz-native-anonymous. To help with this, CreateAnonymousElement is modified so that the third argument add an attribute "class" to the element. nsIHTMLEditor.idl's CreateAnonymousElement is also modified in this patch to reflect the change to the third argument of CreateAnonymousElement.

You probably want a somewhat shorter commit message here; no need to repeat what's obvious from the patch.  For example:

Bug 1086641 - Change CreateAnonymousElement so that it puts the element-specific name in the class attribute instead of the _moz_anonclass attribute, so that we can switch to using selectors with :-moz-native-anonymous.

Note that writing the patch this way means it needs to be landed at the same time as patch 2, but that's ok.

>+    classAttrVal.AppendLiteral("hidden");

Maybe just put a trailing space here: "hidden ", and then:

>+    classAttrVal.AppendLiteral(" ");

...delete this line below.


r=dbaron with that
Attachment #8532361 - Flags: review?(dbaron) → review+
Comment on attachment 8532362 [details] [diff] [review]
1086441-moz-anonclass.diff

So, one thought on how you might (1) find the regression and (2) make the code a bit easier to maintain:

Instead of using SetAttribute() calls with multiple classes in them, on an Element*, you might be able to use element->ClassList()->Add() or element->ClassList()->Remove().

This would let you avoid things like:
>-                                      NS_LITERAL_STRING("hidden"));
>+                                      NS_LITERAL_STRING("moz-table-remove-column hidden"));
which seem like they could be problematic in terms of causing regressions.

(That is, if the intent is "add a class" or "remove a class", you should probably do that explicitly.)


I'd also note that there are places where the current code does things like:
>     res = mRemoveRowButton->HasAttribute(classStr, &hasClass);
>     if (NS_SUCCEEDED(res) && hasClass)
>       mRemoveRowButton->RemoveAttribute(classStr);
which you presumably need to change to remove the "hidden" class, but not remove all the classes.

Otherwise I think this seems reasonable.  Hopefully using clearer addition/removal rather than setting will fix the regressions.
Attachment #8532362 - Flags: feedback?(dbaron) → feedback+
Though, actually, I realize you're dealing with nsIDOMElement*
rather than Element* (except inside CreateAnonymousElement).  It
might make sense to just use the nsIDOMElement variant of getting a
class list (which requires reference-counting it).

In the long term, though, I expect we want to switch those member
variables from being nsIDOMElement* to being Element*; switching
looks like it might be a bit of a pain since Element doesn't inherit from
nsIDOMElement (nsGenericHTMLElement and nsXMLElement do) -- though
things might work out such that it's easy to switch.  (If you do
switch them, it should be a separate patch.)
Excellent. Appreciate the feedback and suggestions.

One thing I realized:

> When setting an element to be absolutely positioned, it correctly get's a "grabber" style 
> applied, but it doesn't respond to mouse events.

This occurs on a clean build (pulled a fresh copy of mozilla-central and then ran ./mach build && ./mach run). I tested by applying "position:absolute" to a <p> and <img> (I applied the style through a stylesheet and separately as an value of the attribute of "style"). Also, when applying position absolute to a <p> it shows both the "grabber" and "resizers" (neither respond to click events. [1] returns false when <img> or <p> is absolutely positioned and true when an element is not absolutely positioned).

I'm not sure if this has always been the case but it seems like it might be a bug? I'll look around and see if there's a ticket already opened for this issue. 

[1] http://hg.mozilla.org/mozilla-central/file/f14dcd1c8c0b/editor/libeditor/nsHTMLEditorEventListener.cpp#l82
For a frame of reference, the current behavior I experience on Firefox 34.0.5 on Mac OS 10.9.5:

When applying position absolute to a <img>, I am able to drag the <img> around the page by the "grabber" icon. I am also able to resize the <img> by clicking on a "resizer" and dragging.

When applying position absolute to a <p>, I am able to drag the <p> around the page by the "grabber" icon. I am also able to resize the <p> by clicking on a "resizer" and dragging.
Attached patch 1086441-create-anon-el-2.diff (obsolete) — — Splinter Review 
I've updated nsHTMLEditor::CreateAnonymousElement to add the following snippet:
> nsISupports* domTokenList;
> nsRefPtr<nsDOMTokenList> classes;
> newElement->GetClassList(&domTokenList);
> classes = static_cast<nsDOMTokenList*>(domTokenList);
> ErrorResult error;

This is similar to the updates to the patch 1086441-moz-anonclass.diff.

Two things I wasn't sure about:
1)
> It might make sense to just use the nsIDOMElement variant of getting 
> a class list (which requires reference-counting it).

I first tried the following:

> nsCOMPtr<nsISupports> domTokenList;
> nsDOMTokenList* classes;
> newElement->GetClassList(getter_AddRefs(domTokenList));
> classes = do_QueryInterface(domTokenList);

but it failed to compile. So I opted for using nsRefPtr for |nsDOMTokenList| instead. However, I'm unsure if this is the correct way to reference count it.

2)
> classes->Add(aClass, error);

I'm unsure what needs to be done with |error|. Is there a preferred way of checking if the resulting call to |Add| had an error?
Attachment #8532361 - Attachment is obsolete: true
Attachment #8556544 - Flags: review?(dbaron)
The changes to this patch are similar to changes in [1].

I replaced calls to modify the class names of elements with something similar to:
> nsISupports* domTokenList;
> nsRefPtr<nsDOMTokenList> classes;
> newElement->GetClassList(&domTokenList);
> classes = static_cast<nsDOMTokenList*>(domTokenList);
> ErrorResult error;

Note: The same concerns from [2] over the usage of nsRefPtr and ErrorResult apply to this patch as well

Note: this does not fix the regression [3]. The regression exists in FF 35.0.1. So it doesn't appear the changes in this patch affect it.

I did some digging on the regression and made the following observations:
1) When an element (ex: div) with contenteditable="true" also has the style "position:relative" than the grabber and resizers of an absolutely positioned element (ex: img) inside that element (div) work as expected
2) When an element (ex: div) with contetneditable="true" does not have the style "position:relative" than the grabber and resizers of an absolutely positioned element (Ex: img) inside that element (div) do not work as expected (they do not respond to mouse clicks)
3) If the <html> element has contenteditable="true", the behavior in (1) is exhibited.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8556544&action=edit
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1086641#c20
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1086641#c14
Attachment #8532362 - Attachment is obsolete: true
Attachment #8556551 - Flags: review?(dbaron)
(In reply to Connor [:cojo] from comment #20)
> Two things I wasn't sure about:
> 1)
> > It might make sense to just use the nsIDOMElement variant of getting 
> > a class list (which requires reference-counting it).
> 
> I first tried the following:
> 
> > nsCOMPtr<nsISupports> domTokenList;
> > nsDOMTokenList* classes;
> > newElement->GetClassList(getter_AddRefs(domTokenList));
> > classes = do_QueryInterface(domTokenList);
> 
> but it failed to compile. So I opted for using nsRefPtr for |nsDOMTokenList|
> instead. However, I'm unsure if this is the correct way to reference count
> it.

Yes.

> 2)
> > classes->Add(aClass, error);
> 
> I'm unsure what needs to be done with |error|. Is there a preferred way of
> checking if the resulting call to |Add| had an error?

error.Failed(), which is defined at:
https://hg.mozilla.org/mozilla-central/file/d7e156a7a0a6/dom/bindings/ErrorResult.h#l134

(In reply to Connor [:cojo] from comment #21)
> Note: this does not fix the regression [3]. The regression exists in FF
> 35.0.1. So it doesn't appear the changes in this patch affect it.

I'm confused as to why you're calling it a regression (which is something that a patch breaks).  What does this bug have to do with it?
Comment on attachment 8556544 [details] [diff] [review]
1086441-create-anon-el-2.diff

>Bug 1086641 - CreateAnonymousElement is used to create anonymous elements in the DOM. The third argument of CreateAnonymousElement adds an attribute to to the created element: "_moz_anonclass". Editor styles that currently use "_moz_anonclass" need to be converted to use :-moz-native-anonymous. To help with this, CreateAnonymousElement is modified so that the third argument add an attribute "class" to the element. nsIHTMLEditor.idl's CreateAnonymousElement is also modified in this patch to reflect the change to the third argument of CreateAnonymousElement.

A good commit message should be a summary of what the patch is changing.  You don't need a detailed list of every change.

Furthermore, there's a semantic difference between the first line and later lines, since many tools show only the first line.  So you want the summary in the first line (without wrapping), and then further details in later lines, wrapped to a reasonable width, as in https://hg.mozilla.org/mozilla-central/rev/830111e10951

In this case, I'd suggest:

Bug 1086641 - Make nsIHTMLEditor::CreateAnonymousElement set the class provided on the class attribute instead of the _moz_anonclass attribute.

The following patch will change the styles using _moz_anonclass to use
:-moz-native-anonymous combined with class selectors.

>diff --git a/editor/libeditor/nsHTMLAnonymousUtils.cpp b/editor/libeditor/nsHTMLAnonymousUtils.cpp

> // Returns in *aReturn an anonymous nsDOMElement of type aTag,
> // child of aParentNode. If aIsCreatedHidden is true, the class
>-// "hidden" is added to the created element. If aAnonClass is not
>-// the empty string, it becomes the value of the attribute "_moz_anonclass"
>+// "hidden" is added to the created element. If aClass is not
>+// the empty string, it becomes the value of the attribute "class"

probably change "the value" to "a value", and probably also (since the wording that's there is a little odd) 'the attribute "class"' to 'the "class" attribute'.

>   nsCOMPtr<nsIDOMElement> newElement = do_QueryInterface(newContent);
>   NS_ENSURE_TRUE(newElement, NS_ERROR_FAILURE);
> 
>+  nsISupports* domTokenList;
>+  nsRefPtr<nsDOMTokenList> classes;
>+  newElement->GetClassList(&domTokenList);
>+  classes = static_cast<nsDOMTokenList*>(domTokenList);

Er, actually, this isn't really what you want; it will leak because Element::GetClassList(nsISupports**) transfers an owning reference to the out-parameter, so the caller has to release the reference.

Also, this code has been converted to use dom::Element now, so what you really want is just (instead of the 4 lines above):

  nsDOMTokenList* classes = newContent->ClassList();


>+  ErrorResult error;
>+
>   // add the "hidden" class if needed
>-  nsresult res;
>   if (aIsCreatedHidden) {
>-    res = newElement->SetAttribute(NS_LITERAL_STRING("class"),
>-                                   NS_LITERAL_STRING("hidden"));
>-    NS_ENSURE_SUCCESS(res, res);
>+    classes->Add(NS_LITERAL_STRING("hidden"), error);

You probably want NS_ENSURE_SUCCESS(rv.ErrorCode(), rv.ErrorCode()); here, after the Add call, to match the old code.

> 
>-  // add an _moz_anonclass attribute if needed
>-  if (!aAnonClass.IsEmpty()) {
>-    res = newElement->SetAttribute(NS_LITERAL_STRING("_moz_anonclass"),
>-                                   aAnonClass);
>-    NS_ENSURE_SUCCESS(res, res);
>+  // add arbitrary class(es) if needed 

This comment isn't right; the Add method only takes a single class, and throws NS_ERROR_DOM_INVALID_CHARACTER_ERR if there's whitespace.  so drop the "(es)" at least.

>+  if (!aClass.IsEmpty()) {
>+    classes->Add(aClass, error);
>   }

Same NS_ENSURE_SUCCESS() here.

>diff --git a/editor/nsIHTMLEditor.idl b/editor/nsIHTMLEditor.idl
>    * "hidden" is added to the created element. If aAnonClass is not
>-   * the empty string, it becomes the value of the attribute "_moz_anonclass"
>+   * the empty string, it becomes the value of the attribute "class"

probably "an item in the value of" instead of just "the value of".  (And rewrap
appropriately.)

>    * @return a DOM Element
>    * @param aTag             [IN] a string representing the desired type of
>    *                              the element to create
>    * @param aParentNode      [IN] the parent node of the created anonymous
>    *                              element
>-   * @param aAnonClass       [IN] contents of the _moz_anonclass attribute
>+   * @param aClass           [IN] contents of the class attribute

Probably "value for" rather than "contents of"


r=dbaron with those changes
Attachment #8556544 - Flags: review?(dbaron) → review+
Comment on attachment 8556551 [details] [diff] [review]
1086441-moz-anonclass-2.diff

Could you fix the manner of getting a token list in this patch, per my comments on the previous patch, and then I'll review it carefully?

(You may need to do something like:

  nsCOMPtr<Element> _variablename_ = do_QueryInterface(_membername_);

to get a mozilla::dom::Element.)
Attachment #8556551 - Flags: review?(dbaron) → review-
> You probably want a somewhat shorter commit message here;
> no need to repeat what's obvious from the patch. 

Commit message should be shorter now. Makes sense to keep the first line short and to the point.

> Also, this code has been converted to use dom::Element now,
> so what you really want is just (instead of the 4 lines above):

I moved a couple lines around to take advantage of dom::Element.

> You probably want NS_ENSURE_SUCCESS(rv.ErrorCode(), rv.ErrorCode());
> here, after the Add call, to match the old code.

I put back the original calls to NS_ENSURE_SUCCESS, now using error.errorCode()

I've also modified the comments accordingly for CreateAnonymousElement in nsHTMLAnonymousUtils.cpp and nsIHTMLEditor.idl
Attachment #8556544 - Attachment is obsolete: true
Attachment #8565453 - Flags: review+
Were you planning to post a revised version of the other patch per comment 24, or should somebody else do that?
Flags: needinfo?(cojojennings)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: