Bug 48112 – Make editing/deleting/5390681.html and editing/deleting/delete-mixed-editable-content-001.html dump text instead of render tree

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Benjamin (Ben) Kalman]

Make editing/deleting/5390681.html and editing/deleting/delete-mixed-editable-content-001.html dump text instead of render tree

[Benjamin (Ben) Kalman]

Created attachment 71529 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 71529 [details]
Patch

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

Thanks for doing this!

> LayoutTests/editing/deleting/5390681-expected.txt:4
> +
> +
> +bar

This contains too little information.  I can't tell what kind of DOM we're generating.  When you're converting a test like this, you should be careful not to lose any information necessary to prevent regressions.  See the changeset that added this test (http://trac.webkit.org/changeset/24945/trunk/LayoutTests/editing/deleting/5390681.html in this case) and make sure the expected result encompasses necessary information to prevent any regression.  In this case particular, we want to avoid hitting an assert but we probably also want to know that we're not adding or removing inappropriate nodes.  So it's good to dump the DOM.  Including dump-as-markup.js and doing Markup.dump('div') will be a good way to achieve this.  http://trac.webkit.org/changeset/70031 is a good example of this kind of test conversion.

> LayoutTests/editing/deleting/delete-mixed-editable-content-001-expected.txt:2
> +12345 
> +

This contains too little information too.  We at least want to see the final DOM.  Please include dump-as-markup.js and dump the contents of the div.

> LayoutTests/editing/deleting/delete-mixed-editable-content-001.html:5
> -	selectAllCommand();
> -	deleteCommand();
> +    selectAllCommand();
> +    deleteCommand();

Great that you got rid of these tabs.

[Benjamin (Ben) Kalman]

Created attachment 71702 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 71702 [details]
Patch

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

> LayoutTests/editing/deleting/5390681.html:3
>  <p>This tests for a bug where expansion for smart delete would not consider editable boundaries.  Only 'foo' should be deleted.  You should see ' bar'. <b>There is a bug: while the non-editable space isn't deleted, deletion inserts a placeholder when it shouldn't.</b></p>

Please move this into Markup.description

> LayoutTests/editing/deleting/5390681.html:8
> +if (window.layoutTestController)
> +    layoutTestController.dumpAsText();

No need to manually call dumpAsText() since dump-as-markup.js automatically calls it.

> LayoutTests/editing/deleting/5390681.html:14
> +Markup.dump(div, "div");

I don't think "div" is descriptive.  You can either omit the second argument or replace it by something more descriptive.

> LayoutTests/editing/deleting/delete-mixed-editable-content-001.html:9
> -<div style="border-style:solid; border-width:2px; border-color:red; width:200px;">
> +<div id="testContent" style="border-style:solid; border-width:2px; border-color:red; width:200px;">

You don't need have to add id here.  You can just do Markup.dump(document.getElementsByTagName('div')[0]).

> LayoutTests/editing/deleting/delete-mixed-editable-content-001.html:18
> +Markup.dump(document.getElementById("testContent"), "testContent");

If you're adding the id, you should just do Markup.dump('testContent') because dump automatically calls getElementById when the first argument is string.  Again, please remove the second argument as labeling the result by "testContent" doesn't add any useful information.

[Benjamin (Ben) Kalman]

Created attachment 71704 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #5)
> Created an attachment (id=71704) [details]
> Patch

LGTM.  +tony, tkent,& ojan for the formal review.

[Benjamin (Ben) Kalman]

Thanks!

[Kent Tamura]

Comment on attachment 71704 [details]
Patch

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

> LayoutTests/editing/deleting/delete-mixed-editable-content-001-expected.txt:1
> +| "

Could you add Markup.description() to show what is this test and how the resultant markup should be, please?

[Kent Tamura]

(In reply to comment #8)
> Could you add Markup.description() to show what is this test and how the resultant markup should be, please?

how -> what

[Ryosuke Niwa]

(In reply to comment #9)
> (In reply to comment #8)
> > Could you add Markup.description() to show what is this test and how the resultant markup should be, please?
> 
> how -> what

I looked up the original changeset (http://trac.webkit.org/browser/trunk/LayoutTests/editing/deleting/delete-mixed-editable-content-001.html) that added this test but it was not clear to me what the test was testing.  We could guess that the test was only testing Mail should not crash but it maybe also testing the resultant DOM.

[Benjamin (Ben) Kalman]

I don't know what the test is for; it was added 4 years ago filed in rdar.  I could speculate something like

'Tests that SelectAll respects editability boundaries. Only "hello" should be selected and deleted; "12345" should remain.'

but that isn't really saying much more than can be concluded from the html + expected.txt.

[Kent Tamura]

ok, I think "Confirm no crash by ...." or something like that is enough.

[Benjamin (Ben) Kalman]

Created attachment 71727 [details]
Patch

[Benjamin (Ben) Kalman]

ok, thanks.

[Kent Tamura]

Comment on attachment 71727 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 71727 [details]
Patch

Clearing flags on attachment: 71727

Committed r70448: <http://trac.webkit.org/changeset/70448>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/70448 might have broken Leopard Intel Debug (Tests)
The following tests are not passing:
editing/spelling/context-menu-suggestions.html
editing/spelling/spellcheck-attribute.html
editing/spelling/spelling-backspace-between-lines.html
editing/spelling/spelling-contenteditable.html
editing/spelling/spelling-linebreak.html
editing/spelling/spelling-textarea.html
platform/mac/accessibility/attributed-string-includes-misspelled-with-selection.html
platform/mac/accessibility/misspelled-attributed-string.html