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

VERIFIED FIXED in Firefox 17

Status

()

Core
Editor
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Zlip792, Assigned: Ehsan)

Tracking

({regression, verifyme})

15 Branch
mozilla18
x86_64
Windows 7
regression, verifyme
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15 affected, firefox16- wontfix, firefox17+ verified, firefox18+ fixed)

Details

(URL)

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

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

Comment 2

5 years ago
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

5 years ago
I tried to reproduce in Firefox15.0.1. but I cannot.
  I cannot find "video embedding option button". Where is the button?
(Reporter)

Comment 4

5 years ago
Created attachment 666962 [details]
Video embed option button

Alice this yellow highlighted button is video embed option one.

Second row, fourth division (last button).

Comment 5

5 years ago
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
(Reporter)

Comment 6

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

Comment 7

5 years ago
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

5 years ago
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).
Blocks: 612128
Status: UNCONFIRMED → NEW
Component: General → Editor
Ever confirmed: true
Keywords: regression
Product: Firefox → Core

Updated

5 years ago
status-firefox15: --- → affected
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?

Updated

5 years ago
status-firefox16: --- → wontfix
tracking-firefox16: ? → -
tracking-firefox17: ? → +
tracking-firefox18: ? → +

Updated

5 years ago
Assignee: nobody → ehsan
I'll take a look.
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.
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.
Blocks: 688789
I think I have a fix.
(Reporter)

Comment 13

5 years ago
Sorry for wrong regression range and false steps to reproduce... Sorry everyone..
Ehsan best of luck for fixing this.
Just an update: my fix breaks a whole lot of tests.  I fixed some of them today, and will look into fixing more tomorrow.
Created attachment 668262 [details] [diff] [review]
Part 1
Attachment #668262 - Flags: review?(roc)
Created attachment 668264 [details] [diff] [review]
Part 2
Attachment #668264 - Flags: review?(roc)
Created attachment 668265 [details] [diff] [review]
Part 3
Attachment #668265 - Flags: review?(roc)
Created attachment 668266 [details] [diff] [review]
Part 4
Attachment #668266 - Flags: review?(roc)
https://tbpl.mozilla.org/?tree=Try&rev=0e0d8776d844
Attachment #668262 - Flags: review?(roc) → review+
Attachment #668264 - Flags: review?(roc) → review+
Attachment #668265 - Flags: review?(roc) → review+
Attachment #668266 - Flags: review?(roc) → review+
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
status-firefox18: --- → fixed
Target Milestone: --- → mozilla18

Comment 21

5 years ago
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/eakhgari@mozilla.com-0e0d8776d844
(Reporter)

Comment 22

5 years ago
Will it be backported to FF17 (current aurora)?
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(In reply to Zlip792 from comment #22)
> Will it be backported to FF17 (current aurora)?

Yes, I'll nominate the patches right now.
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
Attachment #668262 - Flags: approval-mozilla-aurora?
Attachment #668264 - Flags: approval-mozilla-aurora?
Attachment #668265 - Flags: approval-mozilla-aurora?
Attachment #668266 - Flags: approval-mozilla-aurora?
Comment on attachment 668262 [details] [diff] [review]
Part 1

Approving for uplift. Please get these landed before Monday Oct 8th merge day, thank you.
Attachment #668262 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #668264 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #668265 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #668266 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
status-firefox17: --- → fixed
(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!
Keywords: verifyme
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
Status: RESOLVED → VERIFIED
status-firefox17: fixed → verified
Keywords: verifyme
Keywords: verifyme
Depends on: 848644
You need to log in before you can comment on or make changes to this bug.