Bug 45889

Summary: Style visibility: hidden on <br/> tags causes input fields to lose focus after deleting all content
Product: WebKit Reporter: mrandall
Component: FormsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, adele, ap, commit-queue, darin, hyatt, mitz, rniwa, srikumar.b, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 54179    
Bug Blocks:    
Attachments:
Description Flags
proposed changes to fix the bug
darin: review-
proposed changes with layout test to validate the fix
rniwa: review-
latest patch for proposed changes incorporating review comment from adam barth
rniwa: review-, rniwa: commit-queue-
revised patch by making changes with respect to comments from Ryosuke Niwa
none
issue test page
none
revised patch with respect to comments from Ryosuke Niwa
none
revised patch with respect to comments from Ryosuke Niwa
none
revised patch with respect to comments from Ryosuke Niwa
none
revised patch with respect to comments from Ryosuke Niwa
none
revised patch with respect to comments from Ryosuke Niwa
none
updated patch with re-baselined failing testcase
none
adds a regression test tony: review+

Description mrandall 2010-09-16 07:23:45 PDT
Steps to replicate:

-Create an HTML with style "br {visibility: hidden}" (sample included below)
 -Include an input field on the page
-Load the HTML file
 -Type text in the input field
 -Use the backspace key to delete the text.  Once all text is deleted, form field loses focus

Sample HTML to replicate:

<html>
<head>
<style type="text/css">
br {visibility:hidden}
</style>
</head>
<form>
<fieldset>
<input/>
</fieldset>
</form>
</html>
Comment 1 Srikumar B 2010-09-22 19:29:37 PDT
The issue is because we add child node HTMLBRElement when the characters are deleted from the text field. As br style set to hidden without any class, this is considered as default RenderStyle.
Comment 2 Darin Adler 2010-09-28 12:19:42 PDT
I think the right fix is to make sure style from outside the text field can’t affect the elements used inside the field.
Comment 3 Darin Adler 2010-09-28 12:19:57 PDT
Hyatt, any ideas on the best way to do that?
Comment 4 Ryosuke Niwa 2010-09-28 13:07:44 PDT
(In reply to comment #2)
> I think the right fix is to make sure style from outside the text field can’t affect the elements used inside the field.

I agree with you too that that will be the ultimate fix.  Adding br should be fine in this case because that'll remove all dependencies on br styles.  On the other hand, the same technique won't work for the bug 27683.
Comment 5 Srikumar B 2010-10-24 12:29:08 PDT
Created attachment 71694 [details]
proposed changes to fix the bug

As per the discussion on IRC with editing experts, For text input elements, BreakElement place holder is not needed.
So, i have added a validation before creating BR element because any BR styles should not affect text field properties.
Comment 6 Adam Barth 2011-01-12 17:09:12 PST
@rniwa: Any interest in reviewing this editing patch?
Comment 7 Darin Adler 2011-01-12 17:23:13 PST
Comment on attachment 71694 [details]
proposed changes to fix the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=71694&action=review

> WebCore/ChangeLog:8
> +        Tests: Covered with other input text editing testcases

If this is covered with other test cases, then the patch still needs to contain the change to expected results. Otherwise, the fix is not covered. Please include a test.
Comment 8 Ryosuke Niwa 2011-01-12 17:26:55 PST
(In reply to comment #6)
> @rniwa: Any interest in reviewing this editing patch?

The change looks sane but we definitely need a layout test as darin just pointed out.
Comment 9 Srikumar B 2011-01-27 00:11:21 PST
Created attachment 80297 [details]
proposed changes with layout test to validate the fix

proposed changes with layout test to validate the fix
Comment 10 Srikumar B 2011-01-27 00:16:34 PST
I have written a valid layout test and tested with and without changes to made sure the fix is valid and working. Kindly review and do let me know for any further information
-Sri
Comment 11 Adam Barth 2011-01-27 10:11:04 PST
Comment on attachment 80297 [details]
proposed changes with layout test to validate the fix

View in context: https://bugs.webkit.org/attachment.cgi?id=80297&action=review

> Source/WebCore/editing/DeleteSelectionCommand.cpp:751
>                  && ancestorNode->focused())
> +        {

Nit: These two lines should be combined.
Comment 12 Srikumar B 2011-01-30 13:37:36 PST
Created attachment 80600 [details]
latest patch for proposed changes incorporating review comment from adam barth

latest patch for proposed changes incorporating review comment from adam barth
Comment 13 Ryosuke Niwa 2011-01-30 20:26:15 PST
Comment on attachment 80600 [details]
latest patch for proposed changes incorporating review comment from adam barth

View in context: https://bugs.webkit.org/attachment.cgi?id=80600&action=review

> Source/WebCore/ChangeLog:8
> +

Please explain what caused the bug and how you fixed it.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:800
> +    // For text input elements, BreakElement place holder is not needed.
> +    // Because any BR styles should not affect text field properties. 

This comment is redundant it repeats what code says.

> LayoutTests/ChangeLog:8
> +

Please explain what kind of test you're adding.

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:16
> +<script>
> +if (window.layoutTestController)
> +     layoutTestController.dumpEditingCallbacks();
> +</script>
> +
> +<script>
> +if (window.layoutTestController) {
> +    layoutTestController.waitUntilDone();
> +    layoutTestController.dumpAsText();
> +}
> +</script>

Please combine these two script elements.  And also, I don't think you need to call dumpEditingCallbacks in this test unless there are some delegate callbacks you want to test.

Also, why are you calling waitUntilDone() here?  waitUntilDone() is called when the test needs to continue to run after page load event, and I don't see any reason we want such a behavior in this test.

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:22
> +<div><br></div>

What is this div & br doing here?

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:38
> +if(text === "XYZ")
> +	document.write("<div> test SUCCESS <\div>");
> +else
> +	document.write("<div> test FAILED <\div>");

Better written as:
document.write(text == "XYZ" ? "PASS" : "FAIL");

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:45
> +<script>
> +if (window.layoutTestController) {
> +    layoutTestController.notifyDone()
> +}
> +</script>

If we stop calling waitUntilDone(), then this entire script can go away.
Comment 14 Ryosuke Niwa 2011-01-30 20:27:28 PST
Comment on attachment 80297 [details]
proposed changes with layout test to validate the fix

Same issues.
Comment 15 Srikumar B 2011-01-31 07:24:19 PST
Created attachment 80639 [details]
revised patch by making changes with respect to comments from Ryosuke Niwa

revised patch by making changes with respect to comments from Ryosuke Niwa
Comment 16 Srikumar B 2011-01-31 07:26:41 PST
Hi Ryosuke Niwa,
I modified the patch with respect to all the comments posted in the review.
Kindly review do let me know your comments...
Comment 17 Ryosuke Niwa 2011-01-31 23:08:42 PST
Comment on attachment 80639 [details]
revised patch by making changes with respect to comments from Ryosuke Niwa

View in context: https://bugs.webkit.org/attachment.cgi?id=80639&action=review

> Source/WebCore/ChangeLog:9
> +        After deleting all characters in text input field, cursor 
> +        focus is being lost when style br{visibility:hidden} is set.
> +        Actually, placeholder <BR> element not needed for Input Text Field when content empty.
> +        So, additional validation included to skip adding placeholder when the editing node is Input Text field
> +        https://bugs.webkit.org/show_bug.cgi?id=45889

You shouldn't delete the bug title.  Description should come AFTER the bug title and bug url.  So it should be something like (all indented appropriately):

Style visibility: hidden on <br/> tags causes input fields to lose focus after deleting all content
https://bugs.webkit.org/show_bug.cgi?id=45889

The bug was caused by DeleteSelectionCommand's inserting a placeholder br element into a text field
when the text field becomes empty. Fixed DeleteSelectionCommand to not insert the placeholder
when the command is executed inside a text field.

r- because of the format here

> Source/WebCore/editing/DeleteSelectionCommand.cpp:753
>          Node* ancestorNode = startNode ? startNode->shadowAncestorNode() : 0;
>          if (ancestorNode && ancestorNode->hasTagName(inputTag)
>                  && static_cast<HTMLInputElement*>(ancestorNode)->isTextField()
> -                && ancestorNode->focused())
> +                && ancestorNode->focused()) {
>              document()->frame()->editor()->textWillBeDeletedInTextField(static_cast<Element*>(ancestorNode));
> +            isFocusedNodeTextInputElement = true;
> +        }

Why do we care that the input field is focused?

> LayoutTests/ChangeLog:8
> +
> +        This testcase insert characters "ABC" in input text field where style br{visibility:hidden} is set,
> +        Select all the text and delete selection. Then, insert characters "XYZ". 
> +        With this fix, characters "XYZ" can be inserted as focus should not be lost after deletion.
> +        https://bugs.webkit.org/show_bug.cgi?id=45889

Ditto about the bug title and url appearing first followed by a blank line.

I think the description is a little verbose.  Try something along the line of:

Added a test to make sure deleting text from text field doesn't lose focus even if br's visibility is hidden.

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:1
> +<html>

Missing <!DOCTYPE html>

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:11
> +<script src="../editing.js"></script>

Why are you including this file if you're not calling any functions in editing.js?

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:27
> +document.execCommand("InsertText",false,'XYZ');
> +document.write(document.getElementById("t").value == "XYZ" ? "PASS" : "FAIL");

Wait, this doesn't test what the test claims to test.  Shouldn't we be testing that t is still focused?
Comment 18 Ryosuke Niwa 2011-02-04 21:00:22 PST
Any update on this?
Comment 19 Srikumar B 2011-02-05 20:06:18 PST
Comment on attachment 80639 [details]
revised patch by making changes with respect to comments from Ryosuke Niwa

View in context: https://bugs.webkit.org/attachment.cgi?id=80639&action=review

>> Source/WebCore/ChangeLog:9
>> +        https://bugs.webkit.org/show_bug.cgi?id=45889
> 
> You shouldn't delete the bug title.  Description should come AFTER the bug title and bug url.  So it should be something like (all indented appropriately):
> 
> Style visibility: hidden on <br/> tags causes input fields to lose focus after deleting all content
> https://bugs.webkit.org/show_bug.cgi?id=45889
> 
> The bug was caused by DeleteSelectionCommand's inserting a placeholder br element into a text field
> when the text field becomes empty. Fixed DeleteSelectionCommand to not insert the placeholder
> when the command is executed inside a text field.
> 
> r- because of the format here

I will make the changes as suggested in the new patch

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:753
>> +        }
> 
> Why do we care that the input field is focused?

The text is being deleted in this validation.
So i have added the flag isFocusedNodeTextInputElement required to be set only when the input text field is focused and characters in the input field are being deleted which can be used in the below condition to make sure placeholder is needed or not. Hence we will call createBreakElement().

Please share me if you have any further comments on this.

>> LayoutTests/ChangeLog:8
>> +        https://bugs.webkit.org/show_bug.cgi?id=45889
> 
> Ditto about the bug title and url appearing first followed by a blank line.
> 
> I think the description is a little verbose.  Try something along the line of:
> 
> Added a test to make sure deleting text from text field doesn't lose focus even if br's visibility is hidden.

Sure. I will make the changes as per your suggestion in the next patch

>> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:1
>> +<html>
> 
> Missing <!DOCTYPE html>

I have added this in the latest patch

>> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:11
>> +<script src="../editing.js"></script>
> 
> Why are you including this file if you're not calling any functions in editing.js?

I thought editing.js needed to call execute commands "InsertText", "SelectAll", "Delete" etc using document.execCommand() but i am able to call those APIs without including this. So i will remove this in latest patch

>> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:27
>> +document.write(document.getElementById("t").value == "XYZ" ? "PASS" : "FAIL");
> 
> Wait, this doesn't test what the test claims to test.  Shouldn't we be testing that t is still focused?

Without these code changes, after "Delete", i am unable to insert any characters even though focus ring is still on text field as hidden style of BR does not let enter data.
hence the test is failing because document.execCommand("InsertText",false,'XYZ') statement fails.

With this fix, the cursor still remain on input text field and document.execCommand("InsertText",false,'XYZ') succeed.

Do let me know your comments on it.
Comment 20 Srikumar B 2011-02-05 20:10:33 PST
Comment on attachment 80639 [details]
revised patch by making changes with respect to comments from Ryosuke Niwa

View in context: https://bugs.webkit.org/attachment.cgi?id=80639&action=review

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:20
> +</form>

removed <form> & </form> tags in the latest patch as it does not have any specific use with this testcase

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:29
> +<body>

changed to </body> on latest patch
Comment 21 Ryosuke Niwa 2011-02-06 01:57:47 PST
Comment on attachment 80639 [details]
revised patch by making changes with respect to comments from Ryosuke Niwa

View in context: https://bugs.webkit.org/attachment.cgi?id=80639&action=review

>>> Source/WebCore/editing/DeleteSelectionCommand.cpp:753
>>> +        }
>> 
>> Why do we care that the input field is focused?
> 
> The text is being deleted in this validation.
> So i have added the flag isFocusedNodeTextInputElement required to be set only when the input text field is focused and characters in the input field are being deleted which can be used in the below condition to make sure placeholder is needed or not. Hence we will call createBreakElement().
> 
> Please share me if you have any further comments on this.

So are you saying that we can insert br into a text field if it's not focused?  I'm skeptical about that.  I don't think we should ever insert a br into the shadow DOM of an input element.

>>> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:27
>>> +document.write(document.getElementById("t").value == "XYZ" ? "PASS" : "FAIL");
>> 
>> Wait, this doesn't test what the test claims to test.  Shouldn't we be testing that t is still focused?
> 
> Without these code changes, after "Delete", i am unable to insert any characters even though focus ring is still on text field as hidden style of BR does not let enter data.
> hence the test is failing because document.execCommand("InsertText",false,'XYZ') statement fails.
> 
> With this fix, the cursor still remain on input text field and document.execCommand("InsertText",false,'XYZ') succeed.
> 
> Do let me know your comments on it.

I think you should still check that t has focus because that's the bug is about.  You can check both that has focus and you can successfully insert text.
Comment 22 Srikumar B 2011-02-06 09:50:47 PST
Created attachment 81414 [details]
issue test page

issue test page
Comment 23 Srikumar B 2011-02-06 09:53:35 PST
Comment on attachment 80639 [details]
revised patch by making changes with respect to comments from Ryosuke Niwa

View in context: https://bugs.webkit.org/attachment.cgi?id=80639&action=review

>>>> Source/WebCore/editing/DeleteSelectionCommand.cpp:753
>>>> +        }
>>> 
>>> Why do we care that the input field is focused?
>> 
>> The text is being deleted in this validation.
>> So i have added the flag isFocusedNodeTextInputElement required to be set only when the input text field is focused and characters in the input field are being deleted which can be used in the below condition to make sure placeholder is needed or not. Hence we will call createBreakElement().
>> 
>> Please share me if you have any further comments on this.
> 
> So are you saying that we can insert br into a text field if it's not focused?  I'm skeptical about that.  I don't think we should ever insert a br into the shadow DOM of an input element.

The actual issue is, When you click on text field==>enter text=> delete complete text==>Now, I cannot enter text as i am not able to edit the focusing node==> Even i cannot gain the cursor by clicking on the text field==> Now if i click outside the textfield with mouse and click on text field again, i Can enter the text. So the issue is that, till the focus is on text field and while editing the text only. When the focus is lost and gain back, There is no issue. I have attached the issue test page (45889_issue.htm). Kindly try to reproduce the issue. Do let me know your comment.

>>>> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:27
>>>> +document.write(document.getElementById("t").value == "XYZ" ? "PASS" : "FAIL");
>>> 
>>> Wait, this doesn't test what the test claims to test.  Shouldn't we be testing that t is still focused?
>> 
>> Without these code changes, after "Delete", i am unable to insert any characters even though focus ring is still on text field as hidden style of BR does not let enter data.
>> hence the test is failing because document.execCommand("InsertText",false,'XYZ') statement fails.
>> 
>> With this fix, the cursor still remain on input text field and document.execCommand("InsertText",false,'XYZ') succeed.
>> 
>> Do let me know your comments on it.
> 
> I think you should still check that t has focus because that's the bug is about.  You can check both that has focus and you can successfully insert text.

Sure. I will test both conditions. Could you please let me know how to validate the focus is on text field? Is it document.activeElement or any better way to get the focus node which is editable or not?
Comment 24 Ryosuke Niwa 2011-02-06 13:45:54 PST
(In reply to comment #23)
> (From update of attachment 80639 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80639&action=review
> The actual issue is, When you click on text field==>enter text=> delete complete text==>Now, I cannot enter text as i am not able to edit the focusing node==> Even i cannot gain the cursor by clicking on the text field==> Now if i click outside the textfield with mouse and click on text field again, i Can enter the text. So the issue is that, till the focus is on text field and while editing the text only. When the focus is lost and gain back, There is no issue. I have attached the issue test page (45889_issue.htm). Kindly try to reproduce the issue. Do let me know your comment.

I know what the problem you're trying to solve.  However, my point is that you're only fixing the very special case and not taking care of others.  Namely, we're still inserting BR into non-focused text field and that's just wrong.  We should never be inserting BR into text field.  So your condition for inserting BR should be that it's not inside a multi-line shadow DOM.
Comment 25 Ryosuke Niwa 2011-02-06 21:44:49 PST
I talked with sri on IRC and it seems like we should be turning on the flag regardless of the focus.  However, we can't call textWillBeDeletedInTextField when the text field doesn't have focus due to http://trac.webkit.org/changeset/19672.
Comment 26 Srikumar B 2011-02-06 23:52:36 PST
Created attachment 81454 [details]
revised patch with respect to comments from Ryosuke Niwa

attached the revised patch with respect to comments from Ryosuke Niwa and discussing further on IRC. I have executed the regression on editing text fields with this fix. 
I have manually tested editing the text in input field using javascript and made sure that BR element is not being inserted when text is empty in input field.
Comment 27 WebKit Review Bot 2011-02-06 23:55:03 PST
Attachment 81454 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/editing/DeleteSelectionCommand.cpp:751:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 Srikumar B 2011-02-07 00:03:05 PST
Created attachment 81455 [details]
revised patch with respect to comments from Ryosuke Niwa

corrected styling error
Comment 29 WebKit Review Bot 2011-02-07 01:05:32 PST
Attachment 81455 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
Updating OpenSource
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': timed out waiting for server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 129.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 30 Srikumar B 2011-02-07 11:00:45 PST
Created attachment 81495 [details]
revised patch with respect to comments from Ryosuke Niwa

uploaded the patch again as the styling failed because the style bot had an internal error
Comment 31 Ryosuke Niwa 2011-02-07 20:03:30 PST
Comment on attachment 81495 [details]
revised patch with respect to comments from Ryosuke Niwa

View in context: https://bugs.webkit.org/attachment.cgi?id=81495&action=review

Thanks for fixing this bug!

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:5
> +br{visibility:hidden}

Let's put spaces between words and semi-colon at the end as in br { visibility: hidden; }

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:11
> +<script>
> +if (window.layoutTestController) {
> +    layoutTestController.dumpAsText();
> +}
> +</script>

You should put this in the script element inside body so that you don't have to have a separate script element.
Comment 32 Srikumar B 2011-02-07 20:23:55 PST
Created attachment 81572 [details]
revised patch with respect to comments from Ryosuke Niwa
Comment 33 Srikumar B 2011-02-07 20:47:33 PST
Created attachment 81575 [details]
revised patch with respect to comments from Ryosuke Niwa
Comment 34 WebKit Commit Bot 2011-02-07 23:58:13 PST
Comment on attachment 81575 [details]
revised patch with respect to comments from Ryosuke Niwa

Rejecting attachment 81575 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-..." exit_code: 2

Last 500 characters of output:
............
fast/forms .........................................................................................................................................................................................................................................
fast/forms/input-placeholder-visibility-3.html -> failed

Exiting early after 1 failures. 8370 tests run.
199.19s total testing time

8369 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7820005
Comment 35 Srikumar B 2011-04-01 09:16:57 PDT
Created attachment 87868 [details]
updated patch with re-baselined failing testcase

Hi Ryosuje Niwa,
Here with i attached the updated patch. This does not have any additional code changes. It just include the one failed testcase (platform/mac/fast/forms/input-placeholder-visibility-3-expected.txt) that i re-baselined as per this fix. I made sure no other layout tests are failed with this fix. Kindly review and approve for commit-queue.
Comment 36 WebKit Commit Bot 2011-04-03 00:40:32 PDT
Comment on attachment 87868 [details]
updated patch with re-baselined failing testcase

Rejecting attachment 87868 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'build-..." exit_code: 2

Last 500 characters of output:
.......
fast/forms ..............................................................................................................................................................................................................................................
fast/forms/input-placeholder-visibility-3.html -> failed

Exiting early after 1 failures. 8564 tests run.
205.17s total testing time

8563 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8321659
Comment 37 Ryosuke Niwa 2011-04-05 05:38:44 PDT
I tested your patch and I'm getting a crash on editing/execCommand/indent-node-to-split-to-crash.html with the following stack trace.

SHOULD NEVER BE REACHED
/Volumes/Data/webkit2/Source/WebCore/editing/ApplyBlockElementCommand.cpp(142) : virtual void WebCore::ApplyBlockElementCommand::formatSelection(const WebCore::VisiblePosition&, const WebCore::VisiblePosition&)
1   WebCore::ApplyBlockElementCommand::formatSelection(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&)
2   WebCore::IndentOutdentCommand::formatSelection(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&)
3   WebCore::ApplyBlockElementCommand::doApply()
4   WebCore::EditCommand::apply()
5   WebCore::applyCommand(WTF::PassRefPtr<WebCore::EditCommand>)
6   WebCore::executeIndent(WebCore::Frame*, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)
7   WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
8   WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)

The other test failure was due to
LayoutTests/platform/mac-snowleopard/fast/forms/input-placeholder-visibility-3-expected.txt.  You should just remove that file and rebaseline LayoutTests/platform/mac/fast/forms/input-placeholder-visibility-3-expected.txt since they're identical.
Comment 38 Ryosuke Niwa 2011-04-05 05:39:33 PDT
Comment on attachment 87868 [details]
updated patch with re-baselined failing testcase

r- since a test crash.
Comment 39 Eric Seidel (no email) 2011-04-06 10:45:34 PDT
Comment on attachment 81495 [details]
revised patch with respect to comments from Ryosuke Niwa

Cleared Ryosuke Niwa's review+ from obsolete attachment 81495 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 40 Ryosuke Niwa 2011-05-23 10:49:45 PDT
Created attachment 94442 [details]
adds a regression test
Comment 41 Ryosuke Niwa 2011-05-23 10:57:49 PDT
Thanks for the review, Tony.  Landing it now.
Comment 42 Ryosuke Niwa 2011-05-23 10:59:23 PDT
Committed r87081: <http://trac.webkit.org/changeset/87081>