Closed Bug 612128 Opened 14 years ago Closed 12 years ago

execCommand can inject HTML into non-editable parts of a document if a text control is focused

Categories

(Core :: DOM: Editor, defect)

x86
Windows XP
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
blocking2.0 --- -

People

(Reporter: j.ripalda, Assigned: ehsan.akhgari)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 14 obsolete files)

301 bytes, text/html
Details
1.91 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.18 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
21.59 KB, patch
Details | Diff | Splinter Review
2.14 KB, patch
Details | Diff | Splinter Review
1.13 KB, patch
Details | Diff | Splinter Review
4.18 KB, patch
Details | Diff | Splinter Review
2.50 KB, patch
roc
: review+
Details | Diff | Splinter Review
831 bytes, patch
roc
: review+
Details | Diff | Splinter Review
30.16 KB, patch
Details | Diff | Splinter Review
1.39 KB, patch
roc
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/534.7 (KHTML, like Gecko) Chrome/7.0.517.44 Safari/534.7
Build Identifier: Mozilla/5.0 (Windows; Windows NT 5.1; rv:2.0b2) Gecko/20100720 Firefox/4.0b2

We had to implement an ugly browser detect targeting Firefox because of the following behaviour of execCommand not observed with IE or Google Chrome. An example can be seen in the main js file at notepub.com searching for the scrt, lnk, and styl functions.

Some execCommands require user input (a link or sometimes an inserthtml). If a custom html dialog is used rather than the built in one (and there is all sorts of reason that might push you to do so), the enter key pressed at the dialog somehow disrupts the following execCommand. 

We do not think this is our mistake because the key event is captured and its propagation stopped and the selection is saved inmediatelly before the dialog and restored after it, and the whole thing works fine with all other browsers we have tested. The resulting error is:

uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.execCommand]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: http://notepub.com/beta/notepub/js65.js :: <TOP_LEVEL> :: line 1523" data: no]

Reproducible: Always

Steps to Reproduce:
1. create editable div and custom dialog to get the html to insert.
2. before execCommand save selection, capture key events, and stop propagation to protect selection range.
3. execCommand fails
Actual Results:  
NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.execCommand]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: http://notepub.com/beta/notepub/js65.js :: <TOP_LEVEL> :: line 1523" data: no]

Expected Results:  
execCommand execution

Tested with:
Mozilla/5.0 (Windows; Windows NT 5.1; rv:2.0b2) Gecko/20100720 Firefox/4.0b2
AND
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.11) Gecko/20101012 Firefox/3.6.11

Googling nsIDOMNSHTMLDocument.execCommand shows that a lot of people is having trouble with execCommand in Firefox.
Component: General → Editor
Product: Firefox → Core
QA Contact: general → editor
Can you point to an HTML page that shows the problem, with steps to reproduce on that page (what specific things to click, etc)?
I add here some HTML showing some weird behavior (not exactly the bug as reported as that was in much more complex page that we had to take down, sorry about that).

Google Chrome does not show the same bug.

	<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
	<html>
	<head>
		<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
		<title>Bug: contenteditable div</title>
		<link rel="shortcut icon" href="favicon.ico" type="image/vnd.microsoft.icon">
	</head>
	<body>
	<h1>execCommand inserts in the non editable part of the doc</h1>
	To reproduce type in the textarea, HIT ENTER and then click on Insert.<br>
	Oversized cursor upon first loading.<br>
	<input id="dlg" type="textarea"><input type="button" value="Insert" onclick="document.execCommand('inserthtml',null,document.getElementById('dlg').value);">
	<input type="button" value="Set focus" onclick="document.getElementById('edit').focus();">
	<div id="edit" style="width:400px;height:200px;border-style:dashed;" contenteditable="true"></div>
	</body>
	</html>
The previous example also shows another weirdness:
If after load you attempt to inserthtml before setting focus, nothing happens, so far so go good. If you then set focus and then insert, everything seems to be fine, but if you keep inserting, after a random number of inserts, typically 2,3, or 4, inserthtml stops working (still returns true as if successful, and no error is returned, but nothing is inserted).

In more complex pages things get more random, the div sometimes becoming not editable, and the cursor blinking outside of contenteditable. I am admittedly a very poor JS programmer, but I suspect these things should never happen, and they do not seem to happen in IE or Google Chrome.
I don't see any problems with the testcase from comment 2 in a current trunk build....
Attached file Test case
The attached test case reproduces the problem on load.  WebKit is also affected by a similar bug where "foo" gets injected inside the input element.  I couldn't reproduce the issue in comment 3, though.

Also, note that this very specific issue (content injected in non-editable parts of the document) doesn't have anything to do with pressing Enter.  If you are facing other issues on that (and/or comment 3), please file more bugs in the Editor component.
Assignee: nobody → ehsan
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
blocking2.0: --- → ?
I don't think we should block on this, assuming it's not a regression, but we'll definitely take a patch.
blocking2.0: ? → -
(In reply to comment #6)
> I don't think we should block on this, assuming it's not a regression, but
> we'll definitely take a patch.

I tested this on 1.9.2, and the same thing happens there.  Nothing in the code ever checks to make sure that the parent element is editable, so I don't think that this is a regression.
In the previous html example:

- Reload. Click on focus, THEN CLICK on the contenteditable, try to type and
then nothing happens, div not editable. Might take a few attempts to reproduce.

This does not happen if the div is preloaded with content, only seems to happen
on empty divs.

Most of these problems do not happen when saving the current selection as the
mouse moves away and restoring focus/selection a few ms after user generated
event AND a few ms before issuing execCommand. I guess preventDefault() also
would help here.

It seems that the focus/selection restoration has to occur a few miliseconds
after user generated event AND a few miliseconds before execCommand, using
setTimeout.

Perhaps these problems are related to the js restoring focus/selection
executing simultaneously with native code that affects the browser state, such
as the code that sets the focus away from the editable div after a user
generated event, and also the code that implements execCommand????
(In reply to comment #8)
> In the previous html example:
> 
> - Reload. Click on focus, THEN CLICK on the contenteditable, try to type and
> then nothing happens, div not editable. Might take a few attempts to reproduce.
> 
> This does not happen if the div is preloaded with content, only seems to happen
> on empty divs.
> 
> Most of these problems do not happen when saving the current selection as the
> mouse moves away and restoring focus/selection a few ms after user generated
> event AND a few ms before issuing execCommand. I guess preventDefault() also
> would help here.
> 
> It seems that the focus/selection restoration has to occur a few miliseconds
> after user generated event AND a few miliseconds before execCommand, using
> setTimeout.
> 
> Perhaps these problems are related to the js restoring focus/selection
> executing simultaneously with native code that affects the browser state, such
> as the code that sets the focus away from the editable div after a user
> generated event, and also the code that implements execCommand????

I could not reproduce this.  Which version of Firefox are you using?  Can you please test it on the most recent nightly build available at http://nightly.mozilla.org/?
I tried the latest nightly build(downloaded 30 min ago), and the previous html code does not reproduce the bug. But I found another related bug (please tell if should be reported separately) 

Latest minefield, go to notepub.com, choose a username, then a password, after login, before doing anything else change the url from 
http://notepub.com/?buttons=hidden&name=___your_user_name_here___ 
to 
http://notepub.com/beta/?buttons=hidden&name=___your_user_name_here___

then type some text in the big contenteditable div, hit shift+enter to submit, at this point the cursor should be blinking halfway out of the div at the bottom of it, which is something that should never happen, I guess. In firefox 3.6 the div is actually not editable at this point.

The workaround is to never let the div be empty, a <br> suffices.

The code at that notepub.com/beta is constantly changing, so the bug might not be there if you check in a few days.
Screenshot of the previous bug here:
http://notepub.com/?fb=&note=86003&dk=e509257136e3692d342be4cbf9b79f8b
Please report the other bug separately.  Although it's very helpful if you can create a reduced test case which shows that problem and attach it to the bug, especially if the notepub.com/beta code is in flux.
Summary: execCommand fails due to enter key pressed (not a problem in Google Chrome or IE) → execCommand can inject HTML into non-editable parts of a document if a text control is focused
Attached patch Patch (v1) (obsolete) — Splinter Review
This is fixing a very ugly problem.  We have an invariant that designMode documents, and nodes under a contenteditable root should have the NODE_IS_EDITABLE flag.  This is mostly true, except for <input type=text> and <textarea>.  This is a very bad inconsistency, since those nodes are not editable in any sense, they just seem to have content which *is* editable.

This patch first cleans up nsGenericHTMLFormElement::UpdateEditableFormControlState to make those elements editable only if they're in an editable subtree.  That would, however, break IntrinsicState() for those controls, since they used to be relying on nsIContent::Intrinsic to check IsEditable() and set them as :moz-read-write if that returns true.  The code in nsGenericHTMLFormElement::IntrinsicState is responsible for maintaining that state.

I then proceed to remove the wizardry added in bug 559970, which basically happened because of this invariant violation.

We then want some magic to make the anonymous div under those controls editable, so that the textnodes and BR nodes under that div would also be editable.  To do that, we first set NODE_IS_EDITABLE on the anonymous DIV created in nsTextEditorState::CreateRootNode.  That flag would be cleared when the div node is bound to the tree, and the magic in nsCSSFrameConstructor::GetAnonymousContent is responsible for maintaining that state.

We then proceed to make IsEditable actually do the right thing for HTML editors.  This consists of first checking the base class version, and otherwise look at the result of nsIContent::IsEditable (the reason that we couldn't do this before was the NODE_IS_EDITABLE madness I explained above).  nsHTMLEditor::IsEditable is the code responsible to make this happen.

Now, with all of that fixed, the actual fix for the bug is a one-liner: check IsEditable in nsHTMLEditor::DoInsertHTMLWithContext.  :-)

Requesting review from bz on the content part, and from roc on the rest.
Attachment #491771 - Flags: review?(roc)
Attachment #491771 - Flags: review?(bzbarsky)
CCing Jesse because this fix might make him change some of his fuzzers, as it used to rely on the fact that it can use inserthtml to inject random HTML in random places in the document (for example, in editor/libeditor/base/crashtests/382527-1.html), but that assumption is no longer true, and it makes document.execCommand throw, which might break his tests.  I think at the minimum, such statements should now be wrapped in a try/catch block.
Don't worry about me, I wrap everything in try/catch.
I'm seeing an accessibility test timeout with this patch here: accessible/events/test_caretmove.html.  Basically what this patch does is remove the NODE_IS_EDITABLE flag from the <input> and <textarea> elements, and adds it to the anonymous div and textnodes under the text control.  I've confirmed that if I manually add the NODE_IS_EDITABLE flag to those two types of element, this test passes, but it seems like without this flag, the accessibility caret move event is not being fired for text controls.

David, Alex, can you please see what's causing this issue?  It might be the case that some code in the accessibility module is incorrectly relying on that flag to fire the caret move notification, and we need to fix that.
Whiteboard: [help needed]
(In reply to comment #16)
> I'm seeing an accessibility test timeout with this patch here:
> accessible/events/test_caretmove.html.  Basically what this patch does is
> remove the NODE_IS_EDITABLE flag from the <input> and <textarea> elements, and
> adds it to the anonymous div and textnodes under the text control.  I've
> confirmed that if I manually add the NODE_IS_EDITABLE flag to those two types
> of element, this test passes, but it seems like without this flag, the
> accessibility caret move event is not being fired for text controls.
> 
> David, Alex, can you please see what's causing this issue?  It might be the
> case that some code in the accessibility module is incorrectly relying on that
> flag to fire the caret move notification, and we need to fix that.

It doesn't sounds like accessibility problem, a11y don't get nsISelectionListener notifications, so it looks like layout needs node_is_editable flag for this.

But you'll get other a11y failures if you pass this one like test_textboxes.html because I afraid we won't expose editable state on inputs (see nsAccessible::GetStateInternal()).
I found a problem while debugging bug 611189 relating to the case where the anonymous div created for a text control has a text node child before returning from CreateAnonymousContent.  In that case, we fail to set the editable flag on that node, which can make things such as spell check not work.  This patch fixes that issue.
Attachment #492269 - Flags: review?(roc)
(In reply to comment #17)
> (In reply to comment #16)
> > I'm seeing an accessibility test timeout with this patch here:
> > accessible/events/test_caretmove.html.  Basically what this patch does is
> > remove the NODE_IS_EDITABLE flag from the <input> and <textarea> elements, and
> > adds it to the anonymous div and textnodes under the text control.  I've
> > confirmed that if I manually add the NODE_IS_EDITABLE flag to those two types
> > of element, this test passes, but it seems like without this flag, the
> > accessibility caret move event is not being fired for text controls.
> > 
> > David, Alex, can you please see what's causing this issue?  It might be the
> > case that some code in the accessibility module is incorrectly relying on that
> > flag to fire the caret move notification, and we need to fix that.
> 
> It doesn't sounds like accessibility problem, a11y don't get
> nsISelectionListener notifications, so it looks like layout needs
> node_is_editable flag for this.

I didn't say that this is an accessibility problem, I just asked for help since I don't have a clear picture of what happens in the a11y internals to make this test pass without this patch, so that I can try to figure out how the change in this patch breaks this test.

> But you'll get other a11y failures if you pass this one like
> test_textboxes.html because I afraid we won't expose editable state on inputs
> (see nsAccessible::GetStateInternal()).

Well, there aren't any other accessibility test failures with this patch, but I took a look at nsAccessible::GetStateInternal and didn't see anything relevant to this patch, but I think we need to add a check for NS_EVENT_STATE_MOZ_READWRITE and set the editable state accordingly.  If you need the accessible onject for inputs and textareas to be marked as editable, and if a11y is relying on the NODE_IS_EDITABLE flag being set on them, this has changed with this patch, and we need to adjust the code responsible for that in a11y to look for non-readonly text controls instead.
(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > I'm seeing an accessibility test timeout with this patch here:
> > > accessible/events/test_caretmove.html.  Basically what this patch does is
> > > remove the NODE_IS_EDITABLE flag from the <input> and <textarea> elements, and
> > > adds it to the anonymous div and textnodes under the text control.  I've
> > > confirmed that if I manually add the NODE_IS_EDITABLE flag to those two types
> > > of element, this test passes, but it seems like without this flag, the
> > > accessibility caret move event is not being fired for text controls.
> > > 
> > > David, Alex, can you please see what's causing this issue?  It might be the
> > > case that some code in the accessibility module is incorrectly relying on that
> > > flag to fire the caret move notification, and we need to fix that.
> > 
> > It doesn't sounds like accessibility problem, a11y don't get
> > nsISelectionListener notifications, so it looks like layout needs
> > node_is_editable flag for this.
> 
> I didn't say that this is an accessibility problem, 

I didn't say that you said, you just guessed and I answered :)

> I just asked for help since
> I don't have a clear picture of what happens in the a11y internals to make this
> test pass without this patch, so that I can try to figure out how the change in
> this patch breaks this test.

sure, I'm always glad to help you.

> > (see nsAccessible::GetStateInternal()).
> 
> Well, there aren't any other accessibility test failures with this patch, but I
> took a look at nsAccessible::GetStateInternal and didn't see anything relevant
> to this patch, 

You're right. That was wrong assumption, sorry.

> but I think we need to add a check for
> NS_EVENT_STATE_MOZ_READWRITE and set the editable state accordingly.

I don't follow you about NS_EVENT_STATE_MOZ_READWRITE

>  If you
> need the accessible onject for inputs and textareas to be marked as editable,
> and if a11y is relying on the NODE_IS_EDITABLE flag being set on them, this has
> changed with this patch, and we need to adjust the code responsible for that in
> a11y to look for non-readonly text controls instead.

Nah, that's ok, for text fields we set a11y editable state based on editor presence, so it sounds we don't need to change anything on a11y side.
How do you determine if a textnode, for example, is in an editable session (possibly inside a contenteditable section)?
(In reply to comment #21)
> How do you determine if a textnode, for example, is in an editable session
> (possibly inside a contenteditable section)?

We use NODE_IS_EDITABLE (see nsHyperTextAccessible::GetAssociatedEditor) but mostly we don't care about whether textnode is editable or not (we should expose editable state on it but it's for old screen readers I think). Why do you ask? I don't get background of question.
Ehsan, can you describe here the intended distinction in meaning between NODE_IS_EDITABLE and NS_EVENT_STATE_MOZ_READWRITE?
So is it the case that with this patch that nsCaretAccessible::NotifySelectionChanged no longer gets called when the text box is focused? Whereas before, we did? (see comment 17 regarding nsISelectionListener)
(In reply to comment #23)
> Ehsan, can you describe here the intended distinction in meaning between
> NODE_IS_EDITABLE and NS_EVENT_STATE_MOZ_READWRITE?

NODE_IS_EDITABLE, if set on a document node, means that the document is in designMode.
NODE_IS_EDITABLE, if set on a content node, means that the content (or one of its parents) is contentEditable (it won't be set on inputs and textareas after my patch, but it will be set on their anonymous value div).
NS_EVENT_STATE_MOZ_READWRITE, if set on a content node, means that the content is editable as far as the user is concerned.  In that sense, this state is set on input and textareas, in addition to contentEditable elements, etc.
(In reply to comment #22)
> (In reply to comment #21)
> > How do you determine if a textnode, for example, is in an editable session
> > (possibly inside a contenteditable section)?
> 
> We use NODE_IS_EDITABLE (see nsHyperTextAccessible::GetAssociatedEditor) but
> mostly we don't care about whether textnode is editable or not (we should
> expose editable state on it but it's for old screen readers I think).

Hmm, that function should work correctly with this change, I think.

> Why do you ask? I don't get background of question.

I'm trying to understand what causes the behavior of the accessibility code to change with this patch.  So far I haven't been able to come up with an answer.

You mentioned in comment 17 that a11y doesn't get nsISelecionListener notifications from layout.  That's not true, because I've caught it under the debugger, and we _are_ passing a caret move event object to FireDelayedAccessibilityEvent.  It seems to get lost after that point somehow...  Can you please elaborate comment 17?  Have you tried to debug the test with my patches in this bug applied?
(In reply to comment #24)
> So is it the case that with this patch that
> nsCaretAccessible::NotifySelectionChanged no longer gets called when the text
> box is focused? Whereas before, we did? (see comment 17 regarding
> nsISelectionListener)

It does, as we just saw under the debugger!
(In reply to comment #26)

> > We use NODE_IS_EDITABLE (see nsHyperTextAccessible::GetAssociatedEditor) but
> > mostly we don't care about whether textnode is editable or not (we should
> > expose editable state on it but it's for old screen readers I think).
> 
> Hmm, that function should work correctly with this change, I think.

right, nothing should be changed on a11y side for that.

> I'm trying to understand what causes the behavior of the accessibility code to
> change with this patch.  So far I haven't been able to come up with an answer.
> 
> You mentioned in comment 17 that a11y doesn't get nsISelecionListener
> notifications from layout.  That's not true, because I've caught it under the
> debugger, and we _are_ passing a caret move event object to
> FireDelayedAccessibilityEvent.  It seems to get lost after that point
> somehow...  Can you please elaborate comment 17?  Have you tried to debug the
> test with my patches in this bug applied?

Hm, I wasn't able to catch it with your patch applied, iirc I set breakpoint at nsCaretAccessible::NormalSelectionChanged and it wasn't triggered. I'll give one more try.
Comment on attachment 492269 [details] [diff] [review]
Part 2: mark the entire subtree as editable

Just a drive-by comment. When recursing we only need to look at the principal childlist and not the XBL childlist? If the subtree we are setting flags on contains native anonymous nodes itself, are we ok?
(In reply to comment #29)
> Comment on attachment 492269 [details] [diff] [review]
> Part 2: mark the entire subtree as editable
> 
> Just a drive-by comment. When recursing we only need to look at the principal
> childlist and not the XBL childlist? If the subtree we are setting flags on
> contains native anonymous nodes itself, are we ok?

In the general case, probably not, but in this case, yes, we're OK, since the subtree that we're iterating on is the anonymous DIV node of the text control frame, and its content nodes should not have anonymous content of their own.
I wonder if we should assert there that there is no XBL involved in case someone else passes in anonymous content with the editable flag.
(In reply to comment #31)
> I wonder if we should assert there that there is no XBL involved in case
> someone else passes in anonymous content with the editable flag.

Good idea.  How can I get the list of XBL children though?
You could either copy the code in ChildIterator here http://mxr.mozilla.org/mozilla-central/source/layout/base/nsChildIterator.cpp#68 where if nodes is non-null then XBL is involved or just instantiate a ChildIterator and call XBLInvolved.

I guess the situation I'm more concerned about is if the editable flag is set on the node in GetAnonymousContent and it is not a text control creating its anonymous div. Can we assert anything to cover that?
(In reply to comment #33)
> You could either copy the code in ChildIterator here
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsChildIterator.cpp#68
> where if nodes is non-null then XBL is involved or just instantiate a
> ChildIterator and call XBLInvolved.

I guess I'll just borrow code, using ChildIterator is overkill here.

> I guess the situation I'm more concerned about is if the editable flag is set
> on the node in GetAnonymousContent and it is not a text control creating its
> anonymous div. Can we assert anything to cover that?

Of course.
Attachment #494754 - Flags: review?(roc)
Finally tracked down the a11y test failure.  With this, the tests pass successfully.
Attachment #495536 - Flags: review?(roc)
Whiteboard: [help needed] → [has patch][needs review bz,roc]
(In reply to comment #35)
> Created attachment 495536 [details] [diff] [review]
> Part 4: Use the read-write state to find the selection root of the editor
> 
> Finally tracked down the a11y test failure.  With this, the tests pass
> successfully.

Thanks. Can you explain what was happening here?
Blocks: 616250
(In reply to comment #36)
> (In reply to comment #35)
> > Created attachment 495536 [details] [diff] [review] [details]
> > Part 4: Use the read-write state to find the selection root of the editor
> > 
> > Finally tracked down the a11y test failure.  With this, the tests pass
> > successfully.
> 
> Thanks. Can you explain what was happening here?

We would be bailing out of nsEditor::InitializeSelection early, which would mean that the current selection would not be cleared, which is what used to notify the accessible caret.
Attached patch Part 1 (obsolete) — Splinter Review
This patch includes the change that Boris asked me to make on IRC to make sure that readonly input elements in contenteditable sections match :-moz-read-write.  I also added a test to make sure that this doesn't regress in the future.
Attachment #491771 - Attachment is obsolete: true
Attachment #495778 - Flags: review?(bzbarsky)
Attachment #491771 - Flags: review?(bzbarsky)
Mounir, Boris asked me to confirm with you that the behavior described in comment 38 is one that we want to support.  He told me that he remembers this issue being discussed somewhere in the past, and you may remember the result of that discussion better.  Could you please confirm it?
Attachment #495898 - Flags: review?(bzbarsky)
Whiteboard: [has patch][needs review bz,roc] → [has patch][needs review bz]
Comment on attachment 495778 [details] [diff] [review]
Part 1

>+    if (roState && !IsEditable()) {
>+      state &= ~NS_EVENT_STATE_MOZ_READWRITE;
>+    } else {
>+      state |= NS_EVENT_STATE_MOZ_READWRITE;
>+    }

That doesn't look right.  There are four cases, right?  For each case, here are the flags this code as written will produce (keeping in mind what nsIContent::IntrinsicState does):

1)  roState && IsEditable(): READWRITE
2)  roState && !IsEditable(): READONLY
3)  !roState && IsEditable(): READWRITE
4)  !roState && !IsEditable(): READONLY | READWRITE

In other words, with this patch <input> in a random webpage will match :-moz-read-only, which is wrong.  I'm sort of surprised we had no tests for this.

I think what you really want here is that if !roState and |state| doesn't already have READWRITE (which is equivalent to !IsEditable()) you remove the READONLY flag from |state| and add the READWRITE flag, right?

>+  nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
>+  if (content->IsElement()) {

Can |content| not be null?  I'd think it can...

You should document the new semantics you're adding to nsIAnonymousContentCreator.
Attachment #495778 - Flags: review?(bzbarsky) → review-
Comment on attachment 495898 [details] [diff] [review]
Part 5: Remove UpdateEditableFormControlState

This is a good start, but the calls to UpdateEditableState in both AfterSetAttr impls can also go, right?
Attachment #495898 - Flags: review?(bzbarsky) → review+
(In reply to comment #42)
> Comment on attachment 495898 [details] [diff] [review]
> Part 5: Remove UpdateEditableFormControlState
> 
> This is a good start, but the calls to UpdateEditableState in both AfterSetAttr
> impls can also go, right?

No, I don't think so, because they're responsible for updating the readwrite state, right?
nsGenericElement::SetAttrAndNotify handles this for states that change with the attribute value.
(In reply to comment #41)
> Comment on attachment 495778 [details] [diff] [review]
> Part 1
> 
> >+    if (roState && !IsEditable()) {
> >+      state &= ~NS_EVENT_STATE_MOZ_READWRITE;
> >+    } else {
> >+      state |= NS_EVENT_STATE_MOZ_READWRITE;
> >+    }
> 
> That doesn't look right.  There are four cases, right?  For each case, here are
> the flags this code as written will produce (keeping in mind what
> nsIContent::IntrinsicState does):
> 
> 1)  roState && IsEditable(): READWRITE

Right.  We want this because we want the editable-ness override the readonly attr, right?

> 2)  roState && !IsEditable(): READONLY

Hmm, no, we only clear the READWRITE flag here (that's actually bad, see below).

> 3)  !roState && IsEditable(): READWRITE
> 4)  !roState && !IsEditable(): READONLY | READWRITE
> 
> In other words, with this patch <input> in a random webpage will match
> :-moz-read-only, which is wrong.  I'm sort of surprised we had no tests for
> this.

I guess I should add tests for this.

> I think what you really want here is that if !roState and |state| doesn't
> already have READWRITE (which is equivalent to !IsEditable()) you remove the
> READONLY flag from |state| and add the READWRITE flag, right?

So, we also need to make sure that readonly and readwrite are mutually exclusive, right?  In that case, I think this is what we want:

if (!state.HasState(NS_EVENT_STATE_MOZ_READWRITE) &&
    IsTextControl(PR_FALSE)) {
  // get roState
  if (roState) {
    state |= NS_EVENT_STATE_MOZ_READONLY;
    state &= ~NS_EVENT_STATE_MOZ_READWRITE;
  } else {
    state &= ~NS_EVENT_STATE_MOZ_READONLY;
  }
}

The first condition we can check there because the editable-ness will override the readonly attr, and state would not be readonly at that point for sure...

> >+  nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
> >+  if (content->IsElement()) {
> 
> Can |content| not be null?  I'd think it can...

Hmm, yes, I guess so.

> You should document the new semantics you're adding to
> nsIAnonymousContentCreator.

Will do.
Attached patch Part 1 (obsolete) — Splinter Review
With review comments addressed.
Attachment #495778 - Attachment is obsolete: true
Attachment #496086 - Flags: review?(bzbarsky)
Attachment #496088 - Flags: review?(bzbarsky)
> So, we also need to make sure that readonly and readwrite are mutually
> exclusive, right?

Yes, and that at least one of them is set.  Which is why I was talking about having only one flag here; followup bug on that, perhaps?

With that in mind, the new code looks correct but overcomplicated.  That is, if !state.HasState(READWRITE) the all we need to do is check for readonly, and if not then make it READWRITE.  So perhaps like so:

  // Make the text controls read-write if they aren't readonly.  If they're
  // readonly, leave whatever state they already have as generic elements.
  if (!state.HasState(NS_EVENT_STATE_MOZ_READWRITE) &&
      IsTextControl(PR_FALSE)) {
    PRBool roState;
    GetBoolAttr(nsGkAtoms::readonly, &roState);

    if (!roState) {
      state |= NS_EVENT_STATE_MOZ_READWRITE;
      state &= ~NS_EVENT_STATE_MOZ_READONLY;
    }
  }
Comment on attachment 496088 [details] [diff] [review]
Part 6: remove unneeded UpdateEditableState calls

The point was that you don't have to add readonly/readwrite to the state word there either, because the notification for those has been done.  So those entire blocks can go, not just the UpdateEditableState calls.
Attachment #496088 - Flags: review?(bzbarsky) → review-
Attached patch Part 1 (obsolete) — Splinter Review
(In reply to comment #48)
> > So, we also need to make sure that readonly and readwrite are mutually
> > exclusive, right?
> 
> Yes, and that at least one of them is set.  Which is why I was talking about
> having only one flag here; followup bug on that, perhaps?

Filed bug 617629.

> With that in mind, the new code looks correct but overcomplicated.  That is, if
> !state.HasState(READWRITE) the all we need to do is check for readonly, and if
> not then make it READWRITE.  So perhaps like so:
> 
>   // Make the text controls read-write if they aren't readonly.  If they're
>   // readonly, leave whatever state they already have as generic elements.
>   if (!state.HasState(NS_EVENT_STATE_MOZ_READWRITE) &&
>       IsTextControl(PR_FALSE)) {
>     PRBool roState;
>     GetBoolAttr(nsGkAtoms::readonly, &roState);
> 
>     if (!roState) {
>       state |= NS_EVENT_STATE_MOZ_READWRITE;
>       state &= ~NS_EVENT_STATE_MOZ_READONLY;
>     }
>   }

You're right, done.
Attachment #496086 - Attachment is obsolete: true
Attachment #496186 - Flags: review?(bzbarsky)
Attachment #496086 - Flags: review?(bzbarsky)
Attachment #496088 - Attachment is obsolete: true
Attachment #496191 - Flags: review?(bzbarsky)
Comment on attachment 496186 [details] [diff] [review]
Part 1

r=me, assuming the PR_TRUE return is there for a reason for non-elements in the editor IsEditable().  If so, please document the reason.
Attachment #496186 - Flags: review?(bzbarsky) → review+
Comment on attachment 496191 [details] [diff] [review]
Part 6: remove unneeded UpdateEditableState calls

r=me
Attachment #496191 - Flags: review?(bzbarsky) → review+
Attached patch Part 1 (obsolete) — Splinter Review
A saner IsEditable this time!
Attachment #496186 - Attachment is obsolete: true
Attachment #496231 - Flags: review?(bzbarsky)
Comment on attachment 496231 [details] [diff] [review]
Part 1

r=me
Attachment #496231 - Flags: review?(bzbarsky) → review+
Attached patch Folded patch (obsolete) — Splinter Review
Attachment #496394 - Flags: approval2.0?
Whiteboard: [has patch][needs review bz] → [has patch][needs approval]
Comment on attachment 496394 [details] [diff] [review]
Folded patch

a=me

Now you're going to work on blockers, right?  ;)
Attachment #496394 - Flags: approval2.0? → approval2.0+
(In reply to comment #57)
> Comment on attachment 496394 [details] [diff] [review]
> Folded patch
> 
> a=me
> 
> Now you're going to work on blockers, right?  ;)

lol, indeed! :D
Whiteboard: [has patch][needs approval] → [needs landing]
Depends on: 618006
Depends on: 618357
I backed out the patch because of bug 618357 which is a fennec blocker, and I didn't get enough time to come up with a fix for that today :(

http://hg.mozilla.org/mozilla-central/rev/580f9d4ebf9f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 617926, 618357
No longer depends on: 618357
(In reply to comment #39)
> Mounir, Boris asked me to confirm with you that the behavior described in
> comment 38 is one that we want to support.  He told me that he remembers this
> issue being discussed somewhere in the past, and you may remember the result of
> that discussion better.  Could you please confirm it?

Sorry for the late reply, I was offline last week.

(In reply to comment #38)
> Created attachment 495778 [details] [diff] [review]
> Part 1
> 
> This patch includes the change that Boris asked me to make on IRC to make sure
> that readonly input elements in contenteditable sections match
> :-moz-read-write.  I also added a test to make sure that this doesn't regress
> in the future.

I do not remember such a conversation so I guess I forgot about it or it didn't came to me (maybe it was before I've been working on forms?).

(In reply to comment #41)
> 1)  roState && IsEditable(): READWRITE

It seems weird but true.

> 2)  roState && !IsEditable(): READONLY
> 3)  !roState && IsEditable(): READWRITE

Ok.

> 4)  !roState && !IsEditable(): READONLY | READWRITE

It seems really weird. How could both states apply?

As I understand the specs [1], an input element is read-write if the readonly attribute apply and is immutable or if the element is editable. Otherwise, it's read-only.
Obviously, we don't really check for immutable but that's not a big issue. 

For the first case, we could consider that one of the conditions fulfilled is enough but that seems weird because that would mean that a contenteditable can't be read-only?
For the fourth case, I really don't follow how we can have both states applying. We shouldn't. It should be READWRITE IMO.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#selector-read-only
So, given comment 61, I think our current behavior is wrong, but I don't think that this bug should change anything.  Maybe this should be fixed in bug 617629?  Anyways, I talked to roc, and I think it's may not be safe to land this before we branch for post-2.0.
Whiteboard: [post-2.0]
(In reply to comment #62)
> So, given comment 61, I think our current behavior is wrong, but I don't think
> that this bug should change anything.  Maybe this should be fixed in bug
> 617629?  Anyways, I talked to roc, and I think it's may not be safe to land
> this before we branch for post-2.0.

Indeed, this bug isn't the place to fix that behavior as long as it doesn't change it.
> It seems really weird. How could both states apply?

They shouldn't.  The fact that they did was a bug in the patch I was reviewing that I was pointing out.

> an input element is read-write if the readonly attribute apply and is
> immutable or if the element is editable.

With s/immutable/mutable/, that's what we currently do, no?  Or do you mean something different than what I'm thinking by "editable"?
(In reply to comment #64)
> > It seems really weird. How could both states apply?
> 
> They shouldn't.  The fact that they did was a bug in the patch I was reviewing
> that I was pointing out.

Oups, sorry.

> > an input element is read-write if the readonly attribute apply and is
> > immutable or if the element is editable.
> 
> With s/immutable/mutable/, that's what we currently do, no?  Or do you mean
> something different than what I'm thinking by "editable"?

I think we don't take into account the mutable/immutable definition of the input element which isn't as simple as readonly attribute being set. An input element not in the document or disabled are also immutable.

And I still think that it is weird to have a readonly input element inside a contenteditable having :-moz-read-write applying even if we can't technically write inside it. I think we should prevent that: :-moz-read-write/:-moz-read-only should reflect the real state seen by the user.
(In reply to comment #65)
> And I still think that it is weird to have a readonly input element inside a
> contenteditable having :-moz-read-write applying even if we can't technically
> write inside it. I think we should prevent that:
> :-moz-read-write/:-moz-read-only should reflect the real state seen by the
> user.

According to the spec, that is not what we should be doing though, right?
(In reply to comment #66)
> (In reply to comment #65)
> > And I still think that it is weird to have a readonly input element inside a
> > contenteditable having :-moz-read-write applying even if we can't technically
> > write inside it. I think we should prevent that:
> > :-moz-read-write/:-moz-read-only should reflect the real state seen by the
> > user.
> 
> According to the spec, that is not what we should be doing though, right?

Quoting the specs:
"""
The :read-write pseudo-class must match the following elements:
 * input elements to which the readonly attribute applies, but that are not immutable (i.e. that do not have the readonly attribute specified and that are not disabled)
 * textarea elements that do not have a readonly attribute, and that are not disabled
 * any element that is editable [1]

The :read-only pseudo-class must match all other HTML elements.
"""

So, input with @readonly set but inside a contenteditable will not fulfill the first requirement but the third. I think the specs aren't really clear here and should be clarified but it might be my english that make me miss something.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#editable
(In reply to comment #67)
> (In reply to comment #66)
> > (In reply to comment #65)
> > > And I still think that it is weird to have a readonly input element inside a
> > > contenteditable having :-moz-read-write applying even if we can't technically
> > > write inside it. I think we should prevent that:
> > > :-moz-read-write/:-moz-read-only should reflect the real state seen by the
> > > user.
> > 
> > According to the spec, that is not what we should be doing though, right?
> 
> Quoting the specs:
> """
> The :read-write pseudo-class must match the following elements:
>  * input elements to which the readonly attribute applies, but that are not
> immutable (i.e. that do not have the readonly attribute specified and that are
> not disabled)
>  * textarea elements that do not have a readonly attribute, and that are not
> disabled
>  * any element that is editable [1]
> 
> The :read-only pseudo-class must match all other HTML elements.
> """
> 
> So, input with @readonly set but inside a contenteditable will not fulfill the
> first requirement but the third. I think the specs aren't really clear here and
> should be clarified but it might be my english that make me miss something.

Yeah, I guess this _could_ be interpreted both ways...  Should I file a bug on the HTML5 spec?
(In reply to comment #68)
> Yeah, I guess this _could_ be interpreted both ways...  Should I file a bug on
> the HTML5 spec?

I was expecting to do that tomorrow but if you feel like it, feel free :)
Comment on attachment 496394 [details] [diff] [review]
Folded patch

(as per ehsan's comment - bookkeeping)
Attachment #496394 - Flags: approval2.0+
Whiteboard: [post-2.0] → [post-2.0][not-ready]
So, I'd really like to push to get this landed again.  Can some of the mobile folks help me with the testing to see if the issues that existed before still happen?
Sure - should we just build with this patch and test for any regressions like bug 618357?
(In reply to comment #73)
> Sure - should we just build with this patch and test for any regressions like
> bug 618357?

Yes, please!  Let me know if you need help.
Attached patch Part 1Splinter Review
Unbitrotted patch.
Attachment #496231 - Attachment is obsolete: true
Unbitrotted patch.
Unbitrotted patch.
Attachment #492269 - Attachment is obsolete: true
Attachment #495536 - Attachment is obsolete: true
Unbitrotted patch.
Attachment #495898 - Attachment is obsolete: true
Attached patch Folded patch (obsolete) — Splinter Review
Unbitrotted folded patch.
Attachment #496394 - Attachment is obsolete: true
In an Android build of m-c with the latest patches here, I have not seen bug 618357 or any other obvious regressions.
Great!  I've submitted a try server job, and if it goes green, we can land this.  :-)  Thanks Matt for the testing.
Hmm, I've just noticed that in my test build, the backspace key is broken with some Android keyboards.  With Swiftkey X Phone, the backspace key works correctly.  With Swype, backspace works to delete text I have just entered but not text that was already in the field before it was focused.  With Samsung Keypad, backspace never works.

Tested in both chrome and content text fields, all on Samsung Galaxy Tab.  The same tests in a nightly build show no problems.  I'm now preparing a new local build from the same changeset and mozconfig but without this patch applied, to confirm that the problem is caused by the patch rather than something else in my environment.
Great, please keep me updated.  Thanks!
(In reply to comment #82)
> Hmm, I've just noticed that in my test build, the backspace key is broken
> with some Android keyboards.

This does seem related to the patch here.  In a build without the patch but otherwise identical, I do not see any backspace problems.
Try run for e11b86d0d869 is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=e11b86d0d869
Results:
    exception: 1
    success: 124
    warnings: 22
    failure: 16
Total buildrequests: 163
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-e11b86d0d869
Try run for 9907a716967a is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=9907a716967a
Results:
    success: 147
    warnings: 14
    failure: 2
Total buildrequests: 163
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-9907a716967a
Attached patch Folded patch (obsolete) — Splinter Review
Updated folded patch
Attachment #548239 - Attachment is obsolete: true
Try run for 0662f22f6192 is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=0662f22f6192
Results (out of 148 total builds):
    exception: 2
    success: 115
    warnings: 16
    failure: 15
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-0662f22f6192
Attached patch Part 7: Always enable selectAll (obsolete) — Splinter Review
So, I was looking at the selectall command test failures in the try run in comment 88 today.  I spent a lot of time trying to see why these tests are failing on Mac.  nsClipboardSelectAllNoneCommands::IsClipboardCommandEnabled returns true unconditionally, which makes sense, since the selectall command should *always* be enabled.  Here's what's happening.

On Mac, updateEditUIVisibility <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3751> is not called at all.  Which means that this line <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3792> is never called.  This is what ends up making the select all command enabled on non-Mac platforms.  On Mac, we go through the usual process of getting the controllers from the window, looking for a cmd_selectAll controller, and calling IsCommandEnabled on that controller to see whether a particular controller is enabled or not.  Now, the devil lies in the detail.  Whenever an HTML editor is attached to a document, it registers its own controller *before* any others on the window: <http://mxr.mozilla.org/mozilla-central/source/editor/composer/src/nsEditingSession.cpp#1243>, so even if you don't right click on a contenteditable region, the controller returned for cmd_selectAll is going to ne nsSelectAllCommand, not nsClipboardSelectAllNoneCommands, as one would hope.  nsSelectAllCommand::IsCommandEnabled <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditorCommands.cpp#658> naively checks to see whether the selection is editable.  See that "some day we may want to change this" comment there?!  That day is today!  The selectall command should simply always be enabled.
Attachment #560271 - Flags: review?(roc)
Attachment #560042 - Attachment is patch: true
Matt, there is a very small chance that my part 7 patch might have fixed the Fennec problems (as Fennec also doesn't use the updateEditUIVisibility work-around as far as I can tell.  Is there any chance that you could take the latest folded patch and part 7 on top of that and see if it makes any difference?
Try run for 9528112e8725 is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=9528112e8725
Results (out of 171 total builds):
    exception: 4
    success: 133
    warnings: 19
    failure: 15
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-9528112e8725
(In reply to Ehsan Akhgari [:ehsan] from comment #90)
> Matt, there is a very small chance that my part 7 patch might have fixed the
> Fennec problems (as Fennec also doesn't use the updateEditUIVisibility
> work-around as far as I can tell.  Is there any chance that you could take
> the latest folded patch and part 7 on top of that and see if it makes any
> difference?

I tested this patch on HTC T-Mobile G2 (stock Android 2.3) with Android Keyboard, SwiftKey X, and Swype.  I did not see any of the problems from bug 618357 or comment 82.  I'd like to also test with the same device I used in comment 82, but I don't have it with me so I won't be able to do that until next week.
Ah, we have this test which thinks that the selectAll command should be disabled outside of editable regions.  :(  This patch fixes it.
Attachment #560271 - Attachment is obsolete: true
Attachment #560350 - Flags: review?(roc)
(In reply to Matt Brubeck (:mbrubeck) from comment #92)
> (In reply to Ehsan Akhgari [:ehsan] from comment #90)
> > Matt, there is a very small chance that my part 7 patch might have fixed the
> > Fennec problems (as Fennec also doesn't use the updateEditUIVisibility
> > work-around as far as I can tell.  Is there any chance that you could take
> > the latest folded patch and part 7 on top of that and see if it makes any
> > difference?
> 
> I tested this patch on HTC T-Mobile G2 (stock Android 2.3) with Android
> Keyboard, SwiftKey X, and Swype.  I did not see any of the problems from bug
> 618357 or comment 82.  I'd like to also test with the same device I used in
> comment 82, but I don't have it with me so I won't be able to do that until
> next week.

This is great to hear!  Please update this bug when you get to do more testing.  :-)
(In reply to Matt Brubeck (:mbrubeck) from comment #92)
> I tested this patch on HTC T-Mobile G2 (stock Android 2.3) with Android
> Keyboard, SwiftKey X, and Swype.  I did not see any of the problems from bug
> 618357 or comment 82.  I'd like to also test with the same device I used in
> comment 82, but I don't have it with me so I won't be able to do that until
> next week.

I've now tested this on Samsung Galaxy Tab 7" (Android 2.2) with Samsung Keypad, Swiftkey X, and Swype.  I did not see any bugs or regressions editing the urlbar.  This is the same device and software with which I saw regressions in previous versions of this patch set.  This patch is good to go on Android, as far as I can tell.
Thanks a lot for testing this Matt!  This is now on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a72195ce0eaa

Timothy, please note that this patch changes the IsActuallyEditable function used to detect whether we should construct frames lazily for editable elements.
Status: REOPENED → ASSIGNED
Whiteboard: [post-2.0][not-ready]
Target Milestone: mozilla2.0b8 → mozilla9
https://hg.mozilla.org/mozilla-central/rev/a72195ce0eaa
Status: ASSIGNED → RESOLVED
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
Depends on: 688438
I've backed this out because of bug 688438, but good thing is that I have a fix this time!  :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/3045a91f856f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Ehsan Akhgari [:ehsan] from comment #98)
> I've backed this out because of bug 688438, but good thing is that I have a
> fix this time!  :-)
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3045a91f856f

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/3045a91f856f
Target Milestone: mozilla9 → ---
Depends on: 688423
https://hg.mozilla.org/integration/mozilla-inbound/rev/be42bc18185a
Status: REOPENED → ASSIGNED
Target Milestone: --- → mozilla10
Blocks: 695364
https://hg.mozilla.org/mozilla-central/rev/be42bc18185a
https://hg.mozilla.org/mozilla-central/rev/a4d5ca366ef1
Status: ASSIGNED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Depends on: 696798
Try run for 11997a523e21 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=11997a523e21
Results (out of 218 total builds):
    success: 160
    warnings: 56
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-11997a523e21
Without this test fix, the document would be the selected node, and the inserthtml command fails because the selected node is not editable.
Attachment #609563 - Flags: review?(roc)
I'm sure roc would rather review a non-empty patch!
Attachment #609563 - Attachment is obsolete: true
Attachment #609565 - Flags: review?(roc)
Attachment #609563 - Flags: review?(roc)
Note to the mobile team: I'm working on the remaining test failures caused by this patch and will hopefully land it in the next few days.  I will then happily work on fixing any mobile specific regressions (this time I do have a build/debugging environment for Fennec! \o/)
Blocks: 543979
Blocks: 737889
Attachment #560042 - Attachment is obsolete: true
This should hopefully fix the last test failure as a result of this patch.
Attachment #622150 - Flags: review?(roc)
Comment on attachment 622150 [details] [diff] [review]
Part 9: Choose the parent when initiating a drag to see if we're in a textbox or not

Review of attachment 622150 [details] [diff] [review]:
-----------------------------------------------------------------

OK, although it seems like it would be cleaner to have a static method in nsITextControlElement, say GetTextControlElementFromEditingHost, that does the GetParent and QI. That would hide the details of how nsITextControlElements manage their DOM subtree.
Attachment #622150 - Flags: review?(roc) → review+
OK, I'll do that if the try server job turns out green.
Try run for 1da0e0757b86 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1da0e0757b86
Results (out of 100 total builds):
    exception: 22
    success: 16
    warnings: 2
    failure: 60
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-1da0e0757b86
Try run for 4c088e860653 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4c088e860653
Results (out of 222 total builds):
    exception: 1
    success: 188
    warnings: 20
    other: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-4c088e860653
 Timed out after 12 hours without completing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/838fb33405ba
Target Milestone: mozilla10 → mozilla15
https://hg.mozilla.org/mozilla-central/rev/838fb33405ba
Status: REOPENED → RESOLVED
Closed: 13 years ago12 years ago
Resolution: --- → FIXED
No longer depends on: 755599
Depends on: 796839
Depends on: 795418
You need to log in before you can comment on or make changes to this bug.