Closed Bug 1250805 Opened 8 years ago Closed 8 years ago

Inserting new lines (and other commands) clears type-in state

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Version:
44 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: shodh131, Assigned: ayg)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: btpp-followup-2016-03-03)

Attachments

(3 files, 4 obsolete files)

test case with iframe
1.61 KB, text/html
Details
test case with contenteditable containing paragraph
786 bytes, text/html
Details
0006-Bug-1250805-Respect-type-in-state-when-caching-inlin.patch
5.60 KB, patch
masayuki
: review+
Details | Diff | Splinter Review 
In content editable div as well as iframe design mode 'on' to develop rich text editor has some bugs in FF 44.0.2 as follows.

When execCommand('bold') is called it works very fine, but when 'bold' is deselected and return key is pressed to go to next line, 'bold' is still active. We have to deselect it again. It is same for 'italics', 'underline' etc..
Forgot to mention. The same code works fine in google chrome
Component: General → Editor
Product: Firefox → Core
Are you sure the source code of your testcase is complete? jquery lib is missing, no?
Flags: needinfo?(shodh131)
I have included J query lib in my source code. I just didn't copy in example.txt. And the same code works fine in google chrome.
Flags: needinfo?(shodh131)
Please, could you attach a fully working testcase.

        Attachment #8722876 -
        Attachment is obsolete: true

    
    

    
  
<!DOCTYPE html>
<html>
<head>

<title>Untitled Document</title>
<!-- j query lib -->
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.12.0/jquery.min.js"></script>

<!-- Script for Rich text editor -->
<script>
$(document).ready(function() {

message_editable.document.designMode = 'on';

	$("#bold").click(function() {
		$("#message_editable").focus();
		message_editable.document.execCommand('bold', false, null);
		//$("#message_editable").focus();
		return false;
		});
		
	$("#italic").click(function() {
		message_editable.document.execCommand('italic', false, null);
		$("#message_editable").focus();
		return false;
		});
		
	$("#underline").click(function() {
		message_editable.document.execCommand('underline', false, null);
		$("#message_editable").focus();
		return false;
		});

});
</script>

<!-- Styling for my ref can be ignored -->
<style>
#wysiwyg_div {
	float: left;
	width: 200px;
	height: 30px;
	clear: right;
}
#wysiwyg_div img {
	float: left;
	width: 10px;
}
#message_editable {
	float: left;
	clear: left;
	width: 500px;
}
</style>
</head>

<body>
	<!-- Buttons for bold, italic and underline -->
	<div id = "wysiwyg_div" >
            <button id= "bold" title= "Bold" ><img src= "images/bold_icon.jpg" id= "bold_icon" /></button>
            <button id= "italic" title= "Italic" ><img src= "images/italic_icon.jpg" id= "italic_icon" /></button>
            <button id= "underline" title= "Underline" ><img src= "images/underline_icon.jpg" id= "underline_icon" /></button>
	</div>
    
    <!-- iframe -->
	<iframe name= "message_editable" id= "message_editable"></iframe>
    
    <!-- I have tried content editable div also instead of iframe but the problem is same -->
</body>
</html>

    
    

    
  

      
      
      
      

        Attached file
          
        test case with iframe
        — 
      

    


  
Testcase with modified buttons.

        Attachment #8722945 -
        Attachment is obsolete: true

    
    

    
  
wow, thanks for refining. I think you understood the problem what I am facing.

Type a sentence with bold/italic/underline active. 
At the end of the sentence deactivate bold/italic/underline. Press enter/return key. 
Functions are not deactivated, next line continues to be bold/italic/underline.
But the same code works fine in other browsers.

    
    

    
  
It worked in versions previous 16.

Regression rage:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e794cef56df6&tochange=642d1a36702f
Blocks: 590640
Keywords: regression
Version: 44 Branch → 16 Branch

    
  
Version: 16 Branch → 44 Branch
Whiteboard: btpp-followup-2016-03-03

    
    

    
  
Confirmed.  Jorg has done some editor fixes lately, so I'll CC him on the off chance he's interested in taking a look.
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  
Can be easily reproduced in the compose window in TB.

This sort of thing is handled via the "type-in" state in the editor, right?

In the regression range I can see a whole lot of Aryeh's changes, so perhaps he can take a look when he's landed his editor clean-up (bug 1156062, bug 1190172, bug 1191354, bug 1191356).

    
    

    
  
I might take a look, but who reviews editor changes these days?  This is almost surely a regression from bug 590640.

    
    

    
  
(In reply to :Aryeh Gregor (working until May 8) from comment #12)
> I might take a look, but who reviews editor changes these days?
Hard to say. Ehsan knocked a few back (bug 1250010 comment #16 and bug 1257363 comment #6). In one case Robert O'Callahan (not longer with Mozilla) did the review, in the other case Masayuki.

Then Ehsan surprisingly did a review here: bug 233705, comment #56. So you never know ;-)

I think where there is a solution, there will be a reviewer ;-)

    
    

    
  
Masayuki, are you willing to review this?  If so, I might look at it.
Flags: needinfo?(masayuki)
Blocks: 1248971

    
    

    
  
The cached styles should probably be reset at the end of
nsHTMLEditRules::SplitParagraph(). I guess a simple

  ClearCachedStyles();

before the final return will do the trick.

    
    

    
  
(In reply to :Aryeh Gregor (away until August 15) from comment #14)
> Masayuki, are you willing to review this?  If so, I might look at it.

Yeah, I'll try to do it.
Flags: needinfo?(masayuki)

    
    

    
  
(In reply to Daniel Glazman (:glazou) from comment #15)
> The cached styles should probably be reset at the end of
> nsHTMLEditRules::SplitParagraph(). I guess a simple
>   ClearCachedStyles();
> before the final return will do the trick.
I tried it and it doesn't. There are also no paragraphs involved in this test case. Am I missing something?

    
    

    
  
Loic, could you please attach an example with contenteditable.
Flags: needinfo?(epinal99-bugzilla2)

    
    

    
  
There is an example here:
http://starkravingfinkle.org/blog/wp-content/uploads/2007/07/contenteditable.htm

Same issue with a contenteditable div.
Flags: needinfo?(epinal99-bugzilla2)

    
    

    
  


  
Thanks, Loïc. I wanted the contentedible case so I could insert a paragraph to try David's suggestion from comment #15 on a paragraph. I am happy to report that in this case it works.

So we have half the fix ;-) Most likely another ClearCachedStyles(); needs to go where a <br> is inserted.

David, would you like to make another suggestion ;-)
Flags: needinfo?(daniel)

        Attachment #8722949 -
        Attachment description: index.html → test case with iframe
Flags: needinfo?(daniel)

    
    

    
  
That breaks a test:
editor/libeditor/tests/test_bug1250010.html | unexpected HTML - got
"<p><tt>xyz</tt></p><p><tt><img src=...><br></tt></p><p>A<br><tt></tt></p>", expected
"<p><tt>xyz</tt></p><p><tt><img src=...><br></tt></p><p><tt>A</tt><br><tt></tt></p>"

Looks like clearing the style loses the style when a paragraph is split in this case.

    
    

    
  
I don't think clearing the cached styles is correct here.  The problem is that they're being cleared when they shouldn't.  When the cursor is inside a <b> and you run the bold command, it sets type-in state that tells us new text here should not be bold.  This should be recorded in BeforeEdit and restored after the command finishes, due to bug 590640 part 7.  I don't know why it's not working.

If you ClearCachedStyles(), you'll make the opposite (more common) case break, where you hit enter inside some formatted text and then expect new text you type to still be formatted.  In fact, bug 590640 part 7 specifically does not clear cached styles in this case for that reason.

Note that if you put the text in the example inside a <p>, it works as expected.  My guess is the problem here is that when we're just inserting a <br>, the cursor remains within the <b> element, so the style caching gets confused somehow, and thinks the type-in state is wrong and removes it.  But if it's in a <p>, we create a new <p> with no <b> inside it, so it has no reason to get confused.  But this is just speculation.

The code touched by bug 590640 part 7 is definitely the place to look to continue with this.

    
    

    
  
(In reply to :Aryeh Gregor (working until September 2) from comment #25)
> If you ClearCachedStyles(), you'll make the opposite (more common) case
> break, where you hit enter inside some formatted text and then expect new
> text you type to still be formatted.
Indeed. That's what I found trying Daniel's suggestion (comment #15), failure documented in comment #24.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Summary: when calling execCommand('bold'), it works fine but after deselecting bold and press return for a new line, bold is still active. Same goes for italic, underline, etc.. → Inserting new lines (and other commands) clears type-in state

    
    

    
  


  
Previous version that used a mochitest: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b17838cfe1f&exclusion_profile=false

New version uses wpt, based on bug 748308, so I re-ran wpt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=138ce5557474&exclusion_profile=false

This is a good test assuming that insertParagraph really behaves the same as hitting Enter, which I think it does.  Someday wpt is supposed to support the equivalent of synthesizeKey(), and then I intend to add additional wpt tests everywhere that insertParagraph etc. are used that test the equivalent keys as well.

        Attachment #8783917 -
        Flags: review?(masayuki)

        Attachment #8754356 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8783917 [details] [diff] [review]
0006-Bug-1250805-Respect-type-in-state-when-caching-inlin.patch

>+    // If type-in state is set, don't intervene
>+    bool typeInSet, unused;
>+    NS_ENSURE_STATE(mHTMLEditor);
>+    mHTMLEditor->mTypeInState->GetTypingState(typeInSet, unused,
>+      mCachedStyles[j].tag, mCachedStyles[j].attr, nullptr);
>+    if (typeInSet) {
>+      continue;
>+    }

I needed long time to understand what's the type-in state. But I understand now for referring the code of cmd_bold. I hope TypeInState.h explains the detail, though...

But looks like fine to me. If you don't mind, use NS_WARN_IF instead of NS_ENSURE_STATE.

>+    if (node.nodeType == Node.TEXT_NODE && node.nodeValue.indexOf("x") != -1) {
>+      assert_in_array(getComputedStyle(node.parentNode).fontWeight,
>+                    !startBold ? ["700", "bold"] : ["400", "normal"],
>+                    "font-weight");

I worry about this. I'm not familiar with treating bold state, even if the font doesn't have a set of glyph for 700, do browser return 700? It might be safer to use "italic" or something instead.

        Attachment #8783917 -
        Flags: review?(masayuki) → review+

    
    

    
  
Tests need to be run with normal configuration.  If someone configures their browser to use a weird default font with no glyphs for 700, I'm not worried about it if they cause tests to fail.  Fonts don't have to have an italic variant either.

Anyway, I just tested in three browsers and they all return "100" for font-weight if I set it to 100, and it looks like they're using the same font as for 400.

    
    

    
  
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9822a995b93
Respect type-in state when caching inline styles; r=masayuki

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/b9822a995b93
Status: ASSIGNED → RESOLVED
Closed: 8 years ago

          status-firefox51:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

    
    

    
  
Thanks for fixing this, much appreciated!!

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: