Last Comment Bug 612128 - execCommand can inject HTML into non-editable parts of a document if a text control is focused
: execCommand can inject HTML into non-editable parts of a document if a text c...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: unspecified
: x86 Windows XP
: -- major (vote)
: mozilla15
Assigned To: :Ehsan Akhgari (out sick)
:
Mentors:
Depends on: 618006 688423 688438 696798 751513 796839
Blocks: 312971 616250 543979 617926 618355 618357 618732 695364 737889
  Show dependency treegraph
 
Reported: 2010-11-14 08:15 PST by j.ripalda
Modified: 2015-08-27 07:54 PDT (History)
13 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Test case (301 bytes, text/html)
2010-11-15 14:20 PST, :Ehsan Akhgari (out sick)
no flags Details
Patch (v1) (13.38 KB, patch)
2010-11-18 21:07 PST, :Ehsan Akhgari (out sick)
roc: review+
Details | Diff | Review
Part 2: mark the entire subtree as editable (2.10 KB, patch)
2010-11-22 00:21 PST, :Ehsan Akhgari (out sick)
roc: review+
Details | Diff | Review
Part 3: Add some debugging checks (1.91 KB, patch)
2010-12-02 11:00 PST, :Ehsan Akhgari (out sick)
roc: review+
Details | Diff | Review
Part 4: Use the read-write state to find the selection root of the editor (1.09 KB, patch)
2010-12-06 09:52 PST, :Ehsan Akhgari (out sick)
roc: review+
Details | Diff | Review
Part 1 (16.80 KB, patch)
2010-12-06 22:32 PST, :Ehsan Akhgari (out sick)
bzbarsky: review-
Details | Diff | Review
Part 5: Remove UpdateEditableFormControlState (4.01 KB, patch)
2010-12-07 11:23 PST, :Ehsan Akhgari (out sick)
bzbarsky: review+
Details | Diff | Review
Part 1 (21.39 KB, patch)
2010-12-07 23:41 PST, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Part 6: remove unneeded UpdateEditableState calls (2.07 KB, patch)
2010-12-07 23:47 PST, :Ehsan Akhgari (out sick)
bzbarsky: review-
Details | Diff | Review
Part 1 (21.33 KB, patch)
2010-12-08 10:32 PST, :Ehsan Akhgari (out sick)
bzbarsky: review+
Details | Diff | Review
Part 6: remove unneeded UpdateEditableState calls (2.18 KB, patch)
2010-12-08 10:34 PST, :Ehsan Akhgari (out sick)
bzbarsky: review+
Details | Diff | Review
Part 1 (21.60 KB, patch)
2010-12-08 12:27 PST, :Ehsan Akhgari (out sick)
bzbarsky: review+
Details | Diff | Review
Folded patch (28.97 KB, patch)
2010-12-08 18:43 PST, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Part 1 (21.59 KB, patch)
2011-07-25 11:46 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Part 2: mark the entire subtree as editable (2.14 KB, patch)
2011-07-25 11:46 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Part 4: Use the read-write state to find the selection root of the editor (1.13 KB, patch)
2011-07-25 11:48 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Part 5: Remove UpdateEditableFormControlState (4.18 KB, patch)
2011-07-25 11:49 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Folded patch (28.80 KB, patch)
2011-07-25 11:53 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Folded patch (28.76 KB, patch)
2011-09-13 14:56 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Part 7: Always enable selectAll (1.27 KB, patch)
2011-09-14 16:20 PDT, :Ehsan Akhgari (out sick)
roc: review+
Details | Diff | Review
Part 7: Always enable selectAll (2.50 KB, patch)
2011-09-15 08:44 PDT, :Ehsan Akhgari (out sick)
roc: review+
Details | Diff | Review
Part 8: Select the editable div when running the test (188 bytes, patch)
2012-03-26 17:54 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Part 8: Select the editable div when running the test (831 bytes, patch)
2012-03-26 17:59 PDT, :Ehsan Akhgari (out sick)
roc: review+
Details | Diff | Review
Updated folded patch (30.16 KB, patch)
2012-05-08 13:31 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Part 9: Choose the parent when initiating a drag to see if we're in a textbox or not (1.39 KB, patch)
2012-05-08 14:44 PDT, :Ehsan Akhgari (out sick)
roc: review+
Details | Diff | Review

Description j.ripalda 2010-11-14 08:15:03 PST
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.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-11-14 19:38:41 PST
Can you point to an HTML page that shows the problem, with steps to reproduce on that page (what specific things to click, etc)?
Comment 2 j.ripalda 2010-11-15 04:40:52 PST
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>
Comment 3 j.ripalda 2010-11-15 10:36:29 PST
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.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-11-15 13:09:57 PST
I don't see any problems with the testcase from comment 2 in a current trunk build....
Comment 5 :Ehsan Akhgari (out sick) 2010-11-15 14:20:13 PST
Created attachment 490705 [details]
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.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-11-15 14:27:38 PST
I don't think we should block on this, assuming it's not a regression, but we'll definitely take a patch.
Comment 7 :Ehsan Akhgari (out sick) 2010-11-15 14:41:54 PST
(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.
Comment 8 j.ripalda 2010-11-15 14:53:21 PST
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????
Comment 9 :Ehsan Akhgari (out sick) 2010-11-15 15:48:34 PST
(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/?
Comment 10 j.ripalda 2010-11-17 13:34:18 PST
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.
Comment 11 j.ripalda 2010-11-17 13:45:28 PST
Screenshot of the previous bug here:
http://notepub.com/?fb=&note=86003&dk=e509257136e3692d342be4cbf9b79f8b
Comment 12 :Ehsan Akhgari (out sick) 2010-11-17 17:55:07 PST
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.
Comment 13 :Ehsan Akhgari (out sick) 2010-11-18 21:07:23 PST
Created attachment 491771 [details] [diff] [review]
Patch (v1)

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.
Comment 14 :Ehsan Akhgari (out sick) 2010-11-18 21:09:35 PST
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.
Comment 15 Jesse Ruderman 2010-11-18 21:24:18 PST
Don't worry about me, I wrap everything in try/catch.
Comment 16 :Ehsan Akhgari (out sick) 2010-11-19 16:40:01 PST
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.
Comment 17 alexander :surkov 2010-11-21 20:51:12 PST
(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()).
Comment 18 :Ehsan Akhgari (out sick) 2010-11-22 00:21:43 PST
Created attachment 492269 [details] [diff] [review]
Part 2: mark the entire subtree as editable

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.
Comment 19 :Ehsan Akhgari (out sick) 2010-11-22 00:28:09 PST
(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.
Comment 20 alexander :surkov 2010-11-22 02:38:12 PST
(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.
Comment 21 :Ehsan Akhgari (out sick) 2010-11-22 08:14:37 PST
How do you determine if a textnode, for example, is in an editable session (possibly inside a contenteditable section)?
Comment 22 alexander :surkov 2010-11-22 08:22:26 PST
(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.
Comment 23 David Bolter [:davidb] 2010-11-22 12:42:04 PST
Ehsan, can you describe here the intended distinction in meaning between NODE_IS_EDITABLE and NS_EVENT_STATE_MOZ_READWRITE?
Comment 24 David Bolter [:davidb] 2010-11-23 07:59:11 PST
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)
Comment 25 :Ehsan Akhgari (out sick) 2010-11-23 08:03:26 PST
(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.
Comment 26 :Ehsan Akhgari (out sick) 2010-11-23 09:15:06 PST
(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?
Comment 27 :Ehsan Akhgari (out sick) 2010-11-23 09:15:46 PST
(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!
Comment 28 alexander :surkov 2010-11-23 09:36:28 PST
(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 29 Timothy Nikkel (:tnikkel) 2010-11-30 21:25:22 PST
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?
Comment 30 :Ehsan Akhgari (out sick) 2010-12-01 10:36:15 PST
(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.
Comment 31 Timothy Nikkel (:tnikkel) 2010-12-01 14:40:24 PST
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.
Comment 32 :Ehsan Akhgari (out sick) 2010-12-01 17:22:59 PST
(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?
Comment 33 Timothy Nikkel (:tnikkel) 2010-12-01 22:05:00 PST
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?
Comment 34 :Ehsan Akhgari (out sick) 2010-12-02 11:00:42 PST
Created attachment 494754 [details] [diff] [review]
Part 3: Add some debugging checks

(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.
Comment 35 :Ehsan Akhgari (out sick) 2010-12-06 09:52:14 PST
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.
Comment 36 David Bolter [:davidb] 2010-12-06 09:57:49 PST
(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?
Comment 37 :Ehsan Akhgari (out sick) 2010-12-06 12:12:26 PST
(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.
Comment 38 :Ehsan Akhgari (out sick) 2010-12-06 22:32:41 PST
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.
Comment 39 :Ehsan Akhgari (out sick) 2010-12-06 22:33:55 PST
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?
Comment 40 :Ehsan Akhgari (out sick) 2010-12-07 11:23:09 PST
Created attachment 495898 [details] [diff] [review]
Part 5: Remove UpdateEditableFormControlState
Comment 41 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-12-07 19:44:53 PST
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.
Comment 42 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-12-07 19:45:23 PST
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?
Comment 43 :Ehsan Akhgari (out sick) 2010-12-07 20:24:13 PST
(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?
Comment 44 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-12-07 21:10:17 PST
nsGenericElement::SetAttrAndNotify handles this for states that change with the attribute value.
Comment 45 :Ehsan Akhgari (out sick) 2010-12-07 23:15:55 PST
(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.
Comment 46 :Ehsan Akhgari (out sick) 2010-12-07 23:41:26 PST
Created attachment 496086 [details] [diff] [review]
Part 1

With review comments addressed.
Comment 47 :Ehsan Akhgari (out sick) 2010-12-07 23:47:08 PST
Created attachment 496088 [details] [diff] [review]
Part 6: remove unneeded UpdateEditableState calls
Comment 48 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-12-08 08:25:46 PST
> 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 49 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-12-08 08:27:31 PST
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.
Comment 50 :Ehsan Akhgari (out sick) 2010-12-08 10:32:06 PST
Created attachment 496186 [details] [diff] [review]
Part 1

(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.
Comment 51 :Ehsan Akhgari (out sick) 2010-12-08 10:34:05 PST
Created attachment 496191 [details] [diff] [review]
Part 6: remove unneeded UpdateEditableState calls
Comment 52 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-12-08 10:45:03 PST
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.
Comment 53 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-12-08 10:45:34 PST
Comment on attachment 496191 [details] [diff] [review]
Part 6: remove unneeded UpdateEditableState calls

r=me
Comment 54 :Ehsan Akhgari (out sick) 2010-12-08 12:27:49 PST
Created attachment 496231 [details] [diff] [review]
Part 1

A saner IsEditable this time!
Comment 55 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-12-08 12:44:27 PST
Comment on attachment 496231 [details] [diff] [review]
Part 1

r=me
Comment 56 :Ehsan Akhgari (out sick) 2010-12-08 18:43:37 PST
Created attachment 496394 [details] [diff] [review]
Folded patch
Comment 57 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-12-08 18:51:58 PST
Comment on attachment 496394 [details] [diff] [review]
Folded patch

a=me

Now you're going to work on blockers, right?  ;)
Comment 58 :Ehsan Akhgari (out sick) 2010-12-08 20:09:42 PST
(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
Comment 60 :Ehsan Akhgari (out sick) 2010-12-10 18:23:39 PST
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
Comment 61 Mounir Lamouri (:mounir) 2010-12-14 14:59:35 PST
(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
Comment 62 :Ehsan Akhgari (out sick) 2010-12-14 16:50:54 PST
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.
Comment 63 Mounir Lamouri (:mounir) 2010-12-14 16:59:09 PST
(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.
Comment 64 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-12-14 21:36:54 PST
> 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"?
Comment 65 Mounir Lamouri (:mounir) 2010-12-14 22:39:54 PST
(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.
Comment 66 :Ehsan Akhgari (out sick) 2010-12-14 23:51:52 PST
(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?
Comment 67 Mounir Lamouri (:mounir) 2010-12-14 23:58:25 PST
(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
Comment 68 :Ehsan Akhgari (out sick) 2010-12-15 00:03:25 PST
(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?
Comment 69 Mounir Lamouri (:mounir) 2010-12-15 00:05:29 PST
(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 70 :Ehsan Akhgari (out sick) 2010-12-15 00:38:10 PST
Filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=11554.
Comment 71 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-23 17:41:50 PST
Comment on attachment 496394 [details] [diff] [review]
Folded patch

(as per ehsan's comment - bookkeeping)
Comment 72 :Ehsan Akhgari (out sick) 2011-07-21 16:01:12 PDT
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?
Comment 73 Matt Brubeck (:mbrubeck) 2011-07-21 16:04:38 PDT
Sure - should we just build with this patch and test for any regressions like bug 618357?
Comment 74 :Ehsan Akhgari (out sick) 2011-07-22 15:35:07 PDT
(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.
Comment 75 :Ehsan Akhgari (out sick) 2011-07-25 11:46:06 PDT
Created attachment 548233 [details] [diff] [review]
Part 1

Unbitrotted patch.
Comment 76 :Ehsan Akhgari (out sick) 2011-07-25 11:46:42 PDT
Created attachment 548234 [details] [diff] [review]
Part 2: mark the entire subtree as editable

Unbitrotted patch.
Comment 77 :Ehsan Akhgari (out sick) 2011-07-25 11:48:49 PDT
Created attachment 548235 [details] [diff] [review]
Part 4: Use the read-write state to find the selection root of the editor

Unbitrotted patch.
Comment 78 :Ehsan Akhgari (out sick) 2011-07-25 11:49:20 PDT
Created attachment 548236 [details] [diff] [review]
Part 5: Remove UpdateEditableFormControlState

Unbitrotted patch.
Comment 79 :Ehsan Akhgari (out sick) 2011-07-25 11:53:19 PDT
Created attachment 548239 [details] [diff] [review]
Folded patch

Unbitrotted folded patch.
Comment 80 Matt Brubeck (:mbrubeck) 2011-07-25 15:41:02 PDT
In an Android build of m-c with the latest patches here, I have not seen bug 618357 or any other obvious regressions.
Comment 81 :Ehsan Akhgari (out sick) 2011-07-26 13:27:32 PDT
Great!  I've submitted a try server job, and if it goes green, we can land this.  :-)  Thanks Matt for the testing.
Comment 82 Matt Brubeck (:mbrubeck) 2011-07-26 14:54:41 PDT
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.
Comment 83 :Ehsan Akhgari (out sick) 2011-07-26 15:11:28 PDT
Great, please keep me updated.  Thanks!
Comment 84 Matt Brubeck (:mbrubeck) 2011-07-26 16:36:30 PDT
(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.
Comment 85 Mozilla RelEng Bot 2011-07-26 21:40:19 PDT
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
Comment 86 Mozilla RelEng Bot 2011-07-28 22:50:19 PDT
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
Comment 87 :Ehsan Akhgari (out sick) 2011-09-13 14:56:03 PDT
Created attachment 560042 [details] [diff] [review]
Folded patch

Updated folded patch
Comment 88 Mozilla RelEng Bot 2011-09-14 11:51:06 PDT
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
Comment 89 :Ehsan Akhgari (out sick) 2011-09-14 16:20:52 PDT
Created attachment 560271 [details] [diff] [review]
Part 7: Always enable selectAll

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.
Comment 90 :Ehsan Akhgari (out sick) 2011-09-14 16:22:57 PDT
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?
Comment 91 Mozilla RelEng Bot 2011-09-15 03:10:52 PDT
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
Comment 92 Matt Brubeck (:mbrubeck) 2011-09-15 08:01:55 PDT
(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.
Comment 93 :Ehsan Akhgari (out sick) 2011-09-15 08:44:54 PDT
Created attachment 560350 [details] [diff] [review]
Part 7: Always enable selectAll

Ah, we have this test which thinks that the selectAll command should be disabled outside of editable regions.  :(  This patch fixes it.
Comment 94 :Ehsan Akhgari (out sick) 2011-09-15 12:33:22 PDT
(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.  :-)
Comment 95 Matt Brubeck (:mbrubeck) 2011-09-20 09:43:18 PDT
(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.
Comment 96 :Ehsan Akhgari (out sick) 2011-09-20 16:08:59 PDT
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.
Comment 97 Marco Bonardo [::mak] 2011-09-21 03:01:12 PDT
https://hg.mozilla.org/mozilla-central/rev/a72195ce0eaa
Comment 98 :Ehsan Akhgari (out sick) 2011-09-22 15:14:47 PDT
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
Comment 99 Ed Morley [:emorley] 2011-09-23 04:51:42 PDT
(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
Comment 101 :Ehsan Akhgari (out sick) 2011-10-20 11:04:32 PDT
Followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d5ca366ef1
Comment 104 Mozilla RelEng Bot 2012-03-23 15:46:52 PDT
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
Comment 105 :Ehsan Akhgari (out sick) 2012-03-26 17:54:29 PDT
Created attachment 609563 [details] [diff] [review]
Part 8: Select the editable div when running the test

Without this test fix, the document would be the selected node, and the inserthtml command fails because the selected node is not editable.
Comment 106 :Ehsan Akhgari (out sick) 2012-03-26 17:59:58 PDT
Created attachment 609565 [details] [diff] [review]
Part 8: Select the editable div when running the test

I'm sure roc would rather review a non-empty patch!
Comment 107 :Ehsan Akhgari (out sick) 2012-03-26 22:34:59 PDT
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/)
Comment 108 :Ehsan Akhgari (out sick) 2012-05-08 13:31:09 PDT
Created attachment 622121 [details] [diff] [review]
Updated folded patch
Comment 109 :Ehsan Akhgari (out sick) 2012-05-08 14:44:34 PDT
Created attachment 622150 [details] [diff] [review]
Part 9: Choose the parent when initiating a drag to see if we're in a textbox or not

This should hopefully fix the last test failure as a result of this patch.
Comment 110 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-08 14:52:41 PDT
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.
Comment 111 :Ehsan Akhgari (out sick) 2012-05-08 14:54:28 PDT
OK, I'll do that if the try server job turns out green.
Comment 112 Mozilla RelEng Bot 2012-05-08 15:45:39 PDT
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
Comment 113 Mozilla RelEng Bot 2012-05-09 03:15:20 PDT
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.
Comment 115 Ed Morley [:emorley] 2012-05-10 07:39:20 PDT
https://hg.mozilla.org/mozilla-central/rev/838fb33405ba

Note You need to log in before you can comment on or make changes to this bug.