Bug 56505 – Add a test for r81266 and fix HTML Editing for fallback contents in object element

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

[Ryosuke Niwa]

As a regression fix, http://trac.webkit.org/changeset/81266 partially fixed a bug in canHaveChildrenForEditing that WebKit used to ignore contents inside hr and datagrid elements even if those elements had child nodes.  We should also fix a bug that it returns false for object element even when it uses fallback content.

[Ryosuke Niwa]

Created attachment 86010 [details]
56505

[Ojan Vafai]

Comment on attachment 86010 [details]
56505

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

Looks like only the last case's "world" is bolded. I assuem that's a bug in the test case.

> LayoutTests/editing/editability/ignored-content.html:34
> +for (var i = 0; i < (text.length + 1) * test.childNodes.length - 1; i++)
> +    window.getSelection().modify('extend', 'forward', 'character');

for loops should always have curly braces.

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 86010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86010&action=review
> 
> Looks like only the last case's "world" is bolded. I assuem that's a bug in the test case.

I don't quite get what you mean by only the last case is bolded.  As far as I can see in the following expected result, "world" is a child of b element in each of the tree cases:

> LayoutTests/editing/editability/ignored-content-expected.txt:18
> +| <hr>
> +|   "hello "
> +|   <b>                       <- here
> +|     "world"
> +|   " WebKit"
> +| <datagrid>
> +|   "hello "
> +|   <b>                       <- here
> +|     "world"
> +|   " WebKit"
> +| <object>
> +|   "hello "
> +|   <b>                       <- here
> +|     "<#selection-anchor>world<#selection-focus>"
> +|   " WebKit"

>> LayoutTests/editing/editability/ignored-content.html:34
>> +    window.getSelection().modify('extend', 'forward', 'character');
> 
> for loops should always have curly braces.

I don't think that's right.  See the 3rd example in "Spacing" section of http://www.webkit.org/coding/coding-style.html.

Also see:
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Node.cpp#L827
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Node.cpp#L992
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Node.cpp#L1003

[Ojan Vafai]

Comment on attachment 86010 [details]
56505

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

Sorry, I misread the test case and apparently I'm just wrong about for loops.

> Source/WebCore/editing/htmlediting.cpp:83
> +        && (!node->hasTagName(objectTag) || static_cast<const HTMLObjectElement*>(node)->useFallbackContent())

How can you know that this will be an HTMLObjectElement?

[Ojan Vafai]

(In reply to comment #4)
> (From update of attachment 86010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86010&action=review
> 
> Sorry, I misread the test case and apparently I'm just wrong about for loops.
> 
> > Source/WebCore/editing/htmlediting.cpp:83
> > +        && (!node->hasTagName(objectTag) || static_cast<const HTMLObjectElement*>(node)->useFallbackContent())
> 
> How can you know that this will be an HTMLObjectElement?

Ugh. Ignore me. I can't read today.

[Ryosuke Niwa]

Committed r81537: <http://trac.webkit.org/changeset/81537>

[Ryosuke Niwa]

Thanks for the review, Ojan.