Closed
Bug 1306060
Opened 9 years ago
Closed 5 years ago
checking .localName for "BR" which cannot work
Categories
(SeaMonkey :: Composer, defect)
SeaMonkey
Composer
Tracking
(seamonkey2.49esr wontfix, seamonkey2.53+ fixed, seamonkey2.57esr? affected)
RESOLVED
FIXED
seamonkey 2.74
People
(Reporter: aryx, Assigned: frg)
Details
(Whiteboard: SM2.53.3)
Attachments
(2 files)
|
4.18 KB,
patch
|
iannbugzilla
:
review+
iannbugzilla
:
feedback+
iannbugzilla
:
approval-comm-release+
iannbugzilla
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
|
3.89 KB,
patch
|
iannbugzilla
:
review+
|
Details | Diff | Splinter Review |
In bug 1304132, we discovered some issues with InsertElementAroundSelection() in EdFieldSetProps.js
> > do range.selectNode(range.commonAncestorContainer);
> > while (range.commonAncestorContainer.localName in NotAnInlineParent);
> That semicolon in the first line looks misplaced.
> > 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
|
||
Comment 1•9 years ago
|
||
Also wrong:
function nodeIsBreak(editor, node) {
return !node || node.localName == 'BR' || editor.nodeIsBlock(node);
because .localName is lowercase.
Also wrong:
if (start && start.localName == 'BR')
because .localName is lowercase.
This code seems old and messy and we can't trigger it in TB.
Flags: needinfo?(rsx11m.pub)
Flags: needinfo?(philip.chee)
Flags: needinfo?(iann_bugzilla)
These are triggered when dealing with forms, buttons and field sets - not sure whether many emails have those.
I would say these need fixing to be 'br'
Flags: needinfo?(iann_bugzilla)
With it fixed, an excessive <br> is stripped when inserting those elements. For example:
With/without fix and before insert:
<body><br></body>
Without fix and after insert:
<body><button name="a" value="a"></button><br></body>
With fix and after insert:
<body><button name="a" value="a"></button></body>
|
|
||
Comment 4•9 years ago
|
||
Could someone from the SM team please address this and submit a patch. There is also the
do range.selectNode(range.commonAncestorContainer);
while (range.commonAncestorContainer.localName in NotAnInlineParent);
https://dxr.mozilla.org/comm-central/rev/ee25833082257fde66cc37232eca28313a40ec00/editor/ui/dialogs/content/EdDialogCommon.js#736
We'd love to fix it, but we don't really want to build SM to test it and it would be irresponsible to make code changes without testing.
(Sorry about the NI, I don't know how closely you follow bugs.)
Flags: needinfo?(iann_bugzilla)
|
|
||
Comment 5•8 years ago
|
||
> do range.selectNode(range.commonAncestorContainer);
> while (range.commonAncestorContainer.localName in NotAnInlineParent);
This looks like a valid do/while loop:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/do...while
Perhaps this is clearer if rewritten:
do { range.selectNode(range.commonAncestorContainer);
} while (range.commonAncestorContainer.localName in NotAnInlineParent);
|
|
||
Comment 6•8 years ago
|
||
(In reply to Ian Neal from comment #3)
> With it fixed, an excessive <br> is stripped when inserting those elements.
> For example:
> With/without fix and before insert:
> <body><br></body>
> Without fix and after insert:
> <body><button name="a" value="a"></button><br></body>
> With fix and after insert:
> <body><button name="a" value="a"></button></body>
Over IRC Ian said:
> I'm not sure if it is correct behaviour
NI: kaze. Perhaps he has some insight
Flags: needinfo?(fabien)
Flags: needinfo?(daniel)
|
|
||
Comment 7•6 years ago
|
||
The broken code is here. Somehow no one is interested :-(
https://searchfox.org/comm-central/search?q=localName+%3D%3D+%22BR%22&case=false®exp=false&path=EdDialogCommon.js
Component: Composition → General
Flags: needinfo?(rsx11m.pub)
Flags: needinfo?(philip.chee)
Flags: needinfo?(iann_bugzilla)
Product: MailNews Core → SeaMonkey
|
|
||
Updated•6 years ago
|
Summary: sanity check code of InsertElementAroundSelection() in EdFieldSetProps.js → checking .localName for "BR" which cannot work
|
|
Assignee | |
Updated•6 years ago
|
Flags: needinfo?(frgrahl)
|
|
Assignee | |
Comment 8•5 years ago
|
||
Proposed patch before the bug and everybody else dies of old age. While this is clearly wrong I was unable to reproduce the original problem with the latest 2.53.3 builds inserting a button via html. Seems to only affect composer so that is why probably no one complained. Reformatted some of the code around too. Did hurt my eyes :)
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Flags: needinfo?(frgrahl)
Flags: needinfo?(fabien)
Flags: needinfo?(daniel)
Attachment #9138060 -
Flags: feedback?(iann_bugzilla)
|
|
Assignee | |
Updated•5 years ago
|
Component: General → Composer
Comment on attachment 9138060 [details] [diff] [review]
1306060-breaks-2533.patch
Yes, I suspect code changes elsewhere have fixed trailing <br> but best to have this code correct.
Attachment #9138060 -
Flags: review+
Attachment #9138060 -
Flags: feedback?(iann_bugzilla)
Attachment #9138060 -
Flags: feedback+
Attachment #9138060 -
Flags: approval-comm-release+
Attachment #9138060 -
Flags: approval-comm-esr60+
|
|
Assignee | |
Comment 10•5 years ago
|
||
Rebased patch for comm-central only
Attachment #9140030 -
Flags: review?(iann_bugzilla)
|
||
Comment 11•5 years ago
|
||
Comment on attachment 9140030 [details] [diff] [review]
1306060-breaks-cc.patch
Yes, makes sense to me r=me
Attachment #9140030 -
Flags: review?(iann_bugzilla) → review+
|
||
Comment 12•5 years ago
|
||
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/01de8e375632
Fix invalid checks for <br> and clean up formatting in composer code. r=IanN
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
|
|
Assignee | |
Comment 13•5 years ago
|
||
Scheduled for 2.53.3
status-seamonkey2.49esr:
--- → wontfix
status-seamonkey2.53:
--- → affected
status-seamonkey2.57esr:
--- → affected
Target Milestone: --- → seamonkey 2.74
|
|
||
Comment 14•5 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/ad558c51b1f0
folllowup to fix linting. rs=eslint
|
|
Assignee | |
Comment 15•5 years ago
|
||
Target 2.53.3
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/7de2a03afbdca27c1c68216af7385b267bbe9834
Fix invalid checks for <br> and clean up formatting in composer code. r=IanN a=IanN
tracking-seamonkey2.53:
--- → +
tracking-seamonkey2.57esr:
--- → ?
You need to log in
before you can comment on or make changes to this bug.
Description
•