1304132 - fix .localName checks to compare to lowercase

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v1

Before Gecko 2, .localName was uppercase, see https://developer.mozilla.org/en-US/docs/Web/API/Element/localName#Notes

Now it's lowercase, but some checks are still against uppercase.

Jorg, do you know if the failure in nodeIsBreak has been worked around somewhere else?

Comment 1

[Jorg K (CEST = GMT+2)]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] from comment #0)
> Jorg, do you know if the failure in nodeIsBreak has been worked around
> somewhere else?
Not to my knowledge. But then, I didn't know nodeIsBreak() existed ;-)

One question: Is .localName or .nodeName preferable?
https://dxr.mozilla.org/comm-central/rev/6497b3aa36e15daf7bc6828c2e9e421f33e95b6d/mail/components/compose/content/MsgComposeCommands.js#394

Comment 2

[Jorg K (CEST = GMT+2)]

The patch looks good. Do you have a try run for it since you're changing/correcting behaviour in two spots. Someone might rely on the faulty behaviour:
+ return !node || node.localName == 'br' || editor.nodeIsBlock(node);
+ if (start && start.localName == 'br')
{
  editor.deleteNode(start);
  editor.insertNode(start, element, element.childNodes.length);
in InsertElementAroundSelection().

Or did you test that locally? InsertElementAroundSelection() is used and editor.deleteNode(start); followed by editor.insertNode(start, ...) seems odd, doesn't it? But it's used a few lines further up as well. Can you enlighten me.

And you've gotta love this:
// Hack to set the selection just inside the element
editor.insertNode(editor.document.createTextNode(""), element, offset);

Ratty, can you assist us here. Looks like the whole function needs a good clean.

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Jorg K (GMT+2) from comment #1)
> One question: Is .localName or .nodeName preferable?
> https://dxr.mozilla.org/comm-central/rev/
> 6497b3aa36e15daf7bc6828c2e9e421f33e95b6d/mail/components/compose/content/
> MsgComposeCommands.js#394
.localName makes more sense because it has the same behavior for HTML and XHTML.

Pushed it to Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=62bdd09fb25a

Comment 4

[Jorg K (CEST = GMT+2)]

A try push is OK, but we still need see how we can understand/test InsertElementAroundSelection() since its behaviour is changing from partial no-op to doing something perhaps undesired.

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Jorg K (GMT+2) from comment #2)
> The patch looks good. Do you have a try run for it since you're
> changing/correcting behaviour in two spots. Someone might rely on the faulty
> behaviour:
> + return !node || node.localName == 'br' || editor.nodeIsBlock(node);
> + if (start && start.localName == 'br')
> {
>   editor.deleteNode(start);
>   editor.insertNode(start, element, element.childNodes.length);
> in InsertElementAroundSelection().
> 
> Or did you test that locally?
No. All callers are in an if block which can only be reached if something doesn't exist yet and I don't find UI for adding these elements. That code can likely be only reached from editor applications like Kompozer (for HTML).
> InsertElementAroundSelection() is used and
> editor.deleteNode(start); followed by editor.insertNode(start, ...) seems
> odd, doesn't it? But it's used a few lines further up as well. Can you
> enlighten me.
Nowadays <node>.appendChild would be used to move these siblings: https://developer.mozilla.org/en-US/docs/Web/API/Node/appendChild

>        do range.selectNode(range.commonAncestorContainer);
>        while (range.commonAncestorContainer.localName in NotAnInlineParent);
That semicolon in the first line looks misplaced.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] from comment #5)
> > in InsertElementAroundSelection().
> > Or did you test that locally?
> No. All callers are in an if block which can only be reached if something
> doesn't exist yet and I don't find UI for adding these elements.
Well, one caller of InsertElementAroundSelection() is in EdFieldSetProps.js which you've just fixed. Can't that be triggered?

> >        do range.selectNode(range.commonAncestorContainer);
> >        while (range.commonAncestorContainer.localName in NotAnInlineParent);
> That semicolon in the first line looks misplaced.
Great :-(

My suggestion is this: Don't change the "BR" to "br" in InsertElementAroundSelection(), instead put a big comment that this is a no-op. File another bug for InsertElementAroundSelection() to be looked at. It has more than one issue.

I'm not sure whether we should change nodeIsBreak(). But there at least the intention is clear.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8793006 [details] [diff] [review]
patch, v1

Clearing the review for now. See comment #6 for open issues.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v2

(In reply to Jorg K (GMT+2) from comment #6)
> (In reply to Sebastian Hengst [:aryx][:archaeopteryx] from comment #5)
> > > in InsertElementAroundSelection().
> > > Or did you test that locally?
> > No. All callers are in an if block which can only be reached if something
> > doesn't exist yet and I don't find UI for adding these elements.
> Well, one caller of InsertElementAroundSelection() is in EdFieldSetProps.js
> which you've just fixed. Can't that be triggered?
These are all in functions which want to add an element (filedset, button, form), but the UI is missing in Tb.

> > >        do range.selectNode(range.commonAncestorContainer);
> > >        while (range.commonAncestorContainer.localName in NotAnInlineParent);
> > That semicolon in the first line looks misplaced.
> Great :-(
> 
> My suggestion is this: Don't change the "BR" to "br" in
> InsertElementAroundSelection(), instead put a big comment that this is a
> no-op. File another bug for InsertElementAroundSelection() to be looked at.
> It has more than one issue.
File bug 1306060.

> I'm not sure whether we should change nodeIsBreak(). But there at least the
> intention is clear.
Also kept it like it is because that gets called in InsertElementAroundSelection().

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8795795 [details] [diff] [review]
patch, v2

Review of attachment 8795795 [details] [diff] [review]:
-----------------------------------------------------------------

Please add/change comments as indicated. r=jorgk.

::: editor/ui/dialogs/content/EdDialogCommon.js
@@ +692,4 @@
>  
>  function nodeIsBreak(editor, node)
>  {
>    return !node || node.localName == 'BR' || editor.nodeIsBlock(node);

Add comment above this line:
// XXX This doesn't work because .localName is lowercase (see bug 1306060).

@@ +781,5 @@
>        editor.setShouldTxnSetSelection(true);
>      else
>      {
>        // Also move a trailing <br>
> +      // This gets never executed because .localName is lowercase.

Please add XXX and refer to bug 1306060 here:
// XXX This gets never executed because .localName is lowercase (see bug 1306060).
Not nice, but sticks out more.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

http://hg.mozilla.org/comm-central/rev/ee2583308225