ContentEditableField RemoveFormatting#getTableAncestor_ fix

ContentEditableField RemoveFormatting#getTableAncestor_ fix

[gboyer, 2012-11-05 21:41:07]

Thanks! Is it practical to catch this with a test case in removeformatting_test?

https://codereview.appspot.com/6814085/diff/1/closure/goog/editor/plugins/rem...
File closure/goog/editor/plugins/removeformatting.js (right):

https://codereview.appspot.com/6814085/diff/1/closure/goog/editor/plugins/rem...
closure/goog/editor/plugins/removeformatting.js:192: if
(!this.fieldObject.usesIframe() &&
I think it makes sense to make the element containment a condition of the loop
rather than going all the way up the DOM, just to avoid unnecessary iteration.
What do you think about this? Only minimally more verbose, but it's necessarily
richer in control flow. And it avoids treating iframe as a special case, since
the field element iself is never a table. (Also, the null check is probably
unnecessary, since you'll always hit the field itself... but a good case to hit
anyway)

var fieldElement = this.getFieldObject().getElement();
while (node && node != fieldElement) {
  if (nodeToCheck.tagName == goog.dom.TagName.TABLE) {
    return node;
  }
}
return null;

[hochhaus, 2012-11-05 22:22:40]

On 2012/11/05 21:41:07, gboyer wrote:
> Thanks! Is it practical to catch this with a test case in
removeformatting_test?
> 
>
https://codereview.appspot.com/6814085/diff/1/closure/goog/editor/plugins/rem...
> File closure/goog/editor/plugins/removeformatting.js (right):
> 
>
https://codereview.appspot.com/6814085/diff/1/closure/goog/editor/plugins/rem...
> closure/goog/editor/plugins/removeformatting.js:192: if
> (!this.fieldObject.usesIframe() &&
> I think it makes sense to make the element containment a condition of the loop
> rather than going all the way up the DOM, just to avoid unnecessary iteration.
> What do you think about this? Only minimally more verbose, but it's
necessarily
> richer in control flow. And it avoids treating iframe as a special case, since
> the field element iself is never a table. (Also, the null check is probably
> unnecessary, since you'll always hit the field itself... but a good case to
hit
> anyway)
> 
> var fieldElement = this.getFieldObject().getElement();
> while (node && node != fieldElement) {
>   if (nodeToCheck.tagName == goog.dom.TagName.TABLE) {
>     return node;
>   }
> }
> return null;

Your comments make perfect sense to me. I think they are addressed in Patch Set
2. Please let me know if they are not what you had in mind or if you want a more
in depth test added.

[gboyer, 2012-11-06 20:06:03]

Looks good to me. Nathan, would you be able to pull it in?

https://codereview.appspot.com/6814085/diff/3001/closure/goog/editor/plugins/...
File closure/goog/editor/plugins/removeformatting_test.html (right):

https://codereview.appspot.com/6814085/diff/3001/closure/goog/editor/plugins/...
closure/goog/editor/plugins/removeformatting_test.html:35: <table>
Add a comment here explaining that the table wrapping is used in a later test to
ensure that it's ignored. Otherwise it looks a bit random :-)

[hochhaus, 2012-11-06 20:51:55]

On 2012/11/06 20:06:03, gboyer wrote:
> Looks good to me. Nathan, would you be able to pull it in?
> 
>
https://codereview.appspot.com/6814085/diff/3001/closure/goog/editor/plugins/...
> File closure/goog/editor/plugins/removeformatting_test.html (right):
> 
>
https://codereview.appspot.com/6814085/diff/3001/closure/goog/editor/plugins/...
> closure/goog/editor/plugins/removeformatting_test.html:35: <table>
> Add a comment here explaining that the table wrapping is used in a later test
to
> ensure that it's ignored. Otherwise it looks a bit random :-)

Comment added in patch set #3.

[nnaze, 2012-11-06 20:55:07]

Will do. Will send you a CL internally.

Nathan

On Tue, Nov 6, 2012 at 12:06 PM,  <gboyer@google.com> wrote:
> Looks good to me. Nathan, would you be able to pull it in?
>
>
>
https://codereview.appspot.com/6814085/diff/3001/closure/goog/editor/plugins/...
> File closure/goog/editor/plugins/removeformatting_test.html (right):
>
>
https://codereview.appspot.com/6814085/diff/3001/closure/goog/editor/plugins/...
> closure/goog/editor/plugins/removeformatting_test.html:35: <table>
> Add a comment here explaining that the table wrapping is used in a later
> test to ensure that it's ignored. Otherwise it looks a bit random :-)
>
> https://codereview.appspot.com/6814085/

[nnaze, 2012-11-08 19:53:22]

Submitted via SVN. Nick will pull it into p4 in when he runs MOE.

Nathan

On Tue, Nov 6, 2012 at 12:54 PM, Nathan Naze <nnaze@google.com> wrote:
> Will do. Will send you a CL internally.
>
> Nathan
>
> On Tue, Nov 6, 2012 at 12:06 PM,  <gboyer@google.com> wrote:
>> Looks good to me. Nathan, would you be able to pull it in?
>>
>>
>>
https://codereview.appspot.com/6814085/diff/3001/closure/goog/editor/plugins/...
>> File closure/goog/editor/plugins/removeformatting_test.html (right):
>>
>>
https://codereview.appspot.com/6814085/diff/3001/closure/goog/editor/plugins/...
>> closure/goog/editor/plugins/removeformatting_test.html:35: <table>
>> Add a comment here explaining that the table wrapping is used in a later
>> test to ensure that it's ignored. Otherwise it looks a bit random :-)
>>
>> https://codereview.appspot.com/6814085/

[nnaze, 2012-11-08 19:56:11]

Also Garry I have added you as a committer.

You can patch by checking out with SVN and then running curl
"patch_url" | patch -p0

Nathan

On Thu, Nov 8, 2012 at 11:53 AM, Nathan Naze <nnaze@google.com> wrote:
> Submitted via SVN. Nick will pull it into p4 in when he runs MOE.
>
> Nathan
>
> On Tue, Nov 6, 2012 at 12:54 PM, Nathan Naze <nnaze@google.com> wrote:
>> Will do. Will send you a CL internally.
>>
>> Nathan
>>
>> On Tue, Nov 6, 2012 at 12:06 PM,  <gboyer@google.com> wrote:
>>> Looks good to me. Nathan, would you be able to pull it in?
>>>
>>>
>>>
https://codereview.appspot.com/6814085/diff/3001/closure/goog/editor/plugins/...
>>> File closure/goog/editor/plugins/removeformatting_test.html (right):
>>>
>>>
https://codereview.appspot.com/6814085/diff/3001/closure/goog/editor/plugins/...
>>> closure/goog/editor/plugins/removeformatting_test.html:35: <table>
>>> Add a comment here explaining that the table wrapping is used in a later
>>> test to ensure that it's ignored. Otherwise it looks a bit random :-)
>>>
>>> https://codereview.appspot.com/6814085/