796839 - CK Editor video adding option on vB 4.2 not working since FF15

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Ahmad Saleem]

Here are steps to reproduce the isue:

1. Open this forum: http://www.friendskorner.com/forum/
2. Make an account (on request I can mail on test account although free to make)
3. Open any forum, click on "New thread"
4. Now first post any youtube link through CK Editor "video embedding option button"
5. Wait till Auto Save option banner appear
6. Now on same way try to add second link (different from above)

Actual Result:
Dialog box "OK" button does not work at all.

Expected Result:
Link should be embedded perfectly

FF14 version pass this while FF15 show this issue.

This is my regression range found through mozregression tool:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20fc34efd733&tochange=9453cf029b72

Comment 1

[Ahmad Saleem]

Confirmed happening on another forum for me with same core "vbulletin", here is the url: http://forums.iamextreme.net/forum.php

Comment 2

[Ahmad Saleem]

As per Alice, the suspected bug: https://bugzilla.mozilla.org/show_bug.cgi?id=772733

From forum: http://forums.mozillazine.org/viewtopic.php?p=12339095#p12339095

Comment 3

[Alice0775 White]

I tried to reproduce in Firefox15.0.1. but I cannot.
  I cannot find "video embedding option button". Where is the button?

Comment 4

[Ahmad Saleem]

Attachment: Video embed option button

Alice this yellow highlighted button is video embed option one.

Second row, fourth division (last button).

Comment 5

[Alice0775 White]

I tried the following step.
1.  Open http://www.friendskorner.com/forum/
2. Enter "User Name" and "Password", and Click "Log in" button, Go http://www.friendskorner.com/forum/ again
3. Click "Announcements" blue link, Click "+ Post New Thread" blue button
4. Click "Insert Video"Button,  Enter http://www.youtube.com/watch?v=55s3T7VRQSc in  "Enter your video clip URL below." field, Click OK
5. Wait till "Auto-Saved" red  option banner appear
6. Click "Insert Video"Button,  Enter http://www.youtube.com/watch?v=TKLSmhEFoDY in  "Enter your video clip URL below." field, Click OK

It seems to work properly in Firefox15.0.1 and Nightly18.0a1 as well.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0.1 ID:20120905151427
and
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20121001030603

Comment 6

[Ahmad Saleem]

Should I share screencast of this bug reproduction?
I am able to reproduce it. For me second time Click "OK" does not work.

Comment 7

[Ahmad Saleem]

Not the only one facing this issue:
Some bugs reported on vBulletin JIRA, you people should co-ordinate with them to fix it from any side where it is easier.
http://tracker.vbulletin.com/browse/VBIV-15459
http://tracker.vbulletin.com/browse/VBIV-15462
http://tracker.vbulletin.com/browse/VBIV-15470

Some threads on forum
https://www.vbulletin.com/forum/showthread.php/408557-Posting-inline-images-certain-ajax-functionality-in-Firefox-is-partially-broken
https://www.vbulletin.com/forum/showthread.php/407772-Inserting-multiple-images-(ckeditor)

So Hope for the best.....

Comment 8

[Alice0775 White]

Steps to reproduce:
1. Go https://www.vbulletin.com/forum/forum.php
2. Log In (Enter "User Name" and "Password", and Click "Log in" button)
3. Click  forum "vBulletin Pre-sales Questions" blue link
4. Click "+ Post New Thread" blue button
5. Click "Insert Video" Icon,  Enter http://www.youtube.com/watch?v=55s3T7VRQSc in  "Enter your video clip URL below." field, Click OK
6. Press Enter twice
7. Click "Insert Video" Icon,  Enter http://www.youtube.com/watch?v=TKLSmhEFoDY in  "Enter your video clip URL below." field, Click OK

Actual results:
At step 7, dialog box "OK" button does not work at all.
And an error in Error Console as follows:

Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMHTMLDocument.execCommand]
Source file: https://www.vbulletin.com/forum/clientscript/ckeditor/ckeditor.js?t=A7HG4HT&v=420c
Line: 80

Expected results:
Link should be embedded perfectly

Regression widnow(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/6fe7dd2f8f57
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120510021321
Bad:
http://hg.mozilla.org/mozilla-central/rev/b7b6565d12a0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120510050721
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6fe7dd2f8f57&tochange=b7b6565d12a0

Regression widnow(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e5e8a0a47dc5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120509073728
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/36c7d7fdc2ea
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120509093527
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e5e8a0a47dc5&tochange=36c7d7fdc2ea

Suspected:
838fb33405ba	Ehsan Akhgari — Bug 612128 - Prevent the editor from modifying nodes which are not under an editing host; r=roc,bzbarsky This patch ensures that the NODE_IS_EDITABLE flag is only set on nodes living under an editing host. Things like text controls which used to have that flag previously will not have it any more. The flag would be set on their anonymous div node instead. Note that if text controls actually fall under an editing host, they will get the NODE_IS_EDITABLE flag. This patch also makes nsHTMLEditor::IsEditable return sane results (text nodes are always considered to be editable).

Comment 9

[(no longer active)]

I'll take a look.

Comment 10

[(no longer active)]

So the cause of the bug is that we can nsHTMLEditor::IsEditable() on a text node with length 0, and that ends up in a call to nsHTMLEditor::IsVisTextNode with aSafeToAskFrames set to false, so we return false from there and assume that the text node is not editable.

Comment 11

[(no longer active)]

So the whole business of calling IsTextInDirtyFrameVisible from nsHTMLEditor::IsEditable() for text nodes is coming from bug 202166.  But that only happened in NS_FRAME_IS_DIRTY was set on the content's frame (since we didn't wanna reflow).  I inadvertently changed that in bug 688789 where I removed that check completely, and kept on calling IsTextInDirtyFrameVisible.

What bug 612128 did was to make these IsEditable calls mean something.  I think there is no good reason for us to assume that empty text nodes are uneditable.

Comment 12

[(no longer active)]

I think I have a fix.

Comment 13

[Ahmad Saleem]

Sorry for wrong regression range and false steps to reproduce... Sorry everyone..
Ehsan best of luck for fixing this.

Comment 14

[(no longer active)]

Just an update: my fix breaks a whole lot of tests.  I fixed some of them today, and will look into fixing more tomorrow.

Comment 15

[(no longer active)]

Attachment: Part 1

Comment 16

[(no longer active)]

Attachment: Part 2

Comment 17

[(no longer active)]

Attachment: Part 3

Comment 18

[(no longer active)]

Attachment: Part 4

Comment 19

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=0e0d8776d844

Comment 20

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5abea9273a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d97ca37eaa2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7297e7f7b748
https://hg.mozilla.org/integration/mozilla-inbound/rev/e92287b62a54

Comment 21

[Mozilla RelEng Bot]

Try run for 0e0d8776d844 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0e0d8776d844
Results (out of 52 total builds):
    exception: 2
    success: 45
    warnings: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0e0d8776d844

Comment 22

[Ahmad Saleem]

Will it be backported to FF17 (current aurora)?

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d5abea9273a9
https://hg.mozilla.org/mozilla-central/rev/d97ca37eaa2d
https://hg.mozilla.org/mozilla-central/rev/7297e7f7b748
https://hg.mozilla.org/mozilla-central/rev/e92287b62a54

Comment 24

[(no longer active)]

(In reply to Zlip792 from comment #22)
> Will it be backported to FF17 (current aurora)?

Yes, I'll nominate the patches right now.

Comment 25

[(no longer active)]

Comment on attachment 668262 [details] [diff] [review]
Part 1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 612128, bug 688789
User impact if declined: editing commands may fail in some cases, visible to users
Testing completed (on m-c, etc.): m-c, test suite
Risk to taking this patch (and alternatives if risky): the patch series is not small, however the changes are well understood and I think they're rather safe.  They also pass our extensive test suite for editing commands, which make me more confident in them.
String or UUID changes made by this patch: none

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 668262 [details] [diff] [review]
Part 1

Approving for uplift. Please get these landed before Monday Oct 8th merge day, thank you.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/56cdb7a21e0d
https://hg.mozilla.org/releases/mozilla-aurora/rev/b92973364f6d
https://hg.mozilla.org/releases/mozilla-aurora/rev/a72fb0917616
https://hg.mozilla.org/releases/mozilla-aurora/rev/cbba57f7fa32

Comment 28

[(no longer active)]

(In reply to comment #27)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/56cdb7a21e0d
> https://hg.mozilla.org/releases/mozilla-aurora/rev/b92973364f6d
> https://hg.mozilla.org/releases/mozilla-aurora/rev/a72fb0917616
> https://hg.mozilla.org/releases/mozilla-aurora/rev/cbba57f7fa32

Thanks for landing the patches, roc!

Comment 29

[Paul Silaghi, QA [:pauly]]

Saw the issue using the STR in comment 8 on Nightly 2012-10-01.
OK button pressed twice when inserting video links works fine now. Verified fixed on FF 17b5 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0