Closed Bug 1443902 Opened 6 years ago Closed 6 years ago

Cursor disappears on GitHub when using formatting buttons

Categories

(Core :: DOM: Editor, defect, P3)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- verified
firefox63 --- verified

People

(Reporter: armenzg, Assigned: m_kato)

References

Details

Attachments

(3 files)

test case
232 bytes, text/html
Details
Bug 1443902 - Reinitilize selection after destroying nsIEditingSession.
59 bytes, text/x-review-board-request
masayuki
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1443902 - Part 2. Update spellcheck status on focused element after turning off contenteditable.
59 bytes, text/x-review-board-request
masayuki
: review+
lizzard
: approval-mozilla-beta+
Details 
STR:
* Visit any GH issue
* Type a comment
* Click the "Insert a quote" button (or any other formatting button)

Expected results:
* A quote is inserted and you can still see the cursor blink

Actual results:
* A quote is inserted, however, you can't see the cursos anymore
  * Clicking anywhere else on the text does not bring the cursor back

Workaround:
* Swith to another window or tab and then come back
  * After using the workaround you won't be able to reproduce the issue
  * If you reload the GH issue the bug will be reproducible again

This happens on Firefox Nightly and Firefox Release on my Mac.
This does not happen on Chrome.
Component: General → DOM
Product: Firefox → Core
Component: DOM → Editor
Priority: -- → P3
Priority: P3 → --
This depends on contenteditable.

github's issue calls the following.

1. contenteditable=true for textarea?
2. execCommand
3. contenteditable=false for textarea?

Since setting contenteditable=false, caret will be hidden by EditorBase::FinalizeSelection
status-firefox62: --- → affected
Priority: -- → P3
Attached file test case — 

    
    

    
  
Maybe, github wants to call execCommand('insertText'), but we don't support it on form control due to bug 1220696.  So they change contenteditable.

    
  
See Also:  → 1408913
Summary: Cursor dissapears on GitHub when using formatting buttons → Cursor disappears on GitHub when using formatting buttons

    
  
Assignee: nobody → m_kato

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8990669 [details]
Bug 1443902 - Reinitilize selection after destroying nsIEditingSession.

https://reviewboard.mozilla.org/r/255744/#review262468

I think that GitHub is important website for web developers who are important users of us, so, it might be better to request to uplift this to Beta.

::: dom/html/nsHTMLDocument.cpp:2349
(Diff revision 1)
> +      TextEditor* textEditor = txtCtrl->GetTextEditor();
> +      if (textEditor) {
> +        textEditor->ReinitializeSelection(*element);

It seems that calling EditorBase::OnFocus() is not safe. Please use RefPtr<TextEditor> here rather than raw pointer.

::: editor/libeditor/TextEditor.h:145
(Diff revision 1)
>     *
>     * @ param aString   the string to be set
>     */
>    nsresult SetText(const nsAString& aString);
>  
> +  void ReinitializeSelection(Element& aElement);

Please explain why we need to have this method and what this will do.

Additionally, it might be better to create this method into EditorBase since InitializeSelection() and FinalizeSelection() are implemented in EditorBase. So, for consistency, *I* think that it's better place to add this method. But up to you.

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

    
    

    
      
  

    
    

    
  
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/4c31558d3481
Reinitilize selection after destroying nsIEditingSession. r=masayuki

    
    

    
  
Backout by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/120a610ee8ba
Backed out 1 changesets for reftest failures reftest/tests/editor/reftests/1443902-2.html CLOSED TREE

    
    

    
  
Hmm, async spellchecking causes the orange.

    
    

    
  
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #11)
> Hmm, async spellchecking causes the orange.

I file as Bug 1474508.

    
    

    
  
Ah, if merging this to beta or ESR, I should handle bug 1474508 on this bug.  I will add new fix as part2.

    
    

    
      
  


  

    
    

    
      
  

    
    

    
  
Comment on attachment 8990917 [details]
Bug 1443902 - Part 2. Update spellcheck status on focused element after turning off contenteditable.

https://reviewboard.mozilla.org/r/255924/#review262786

::: commit-message-9d90c:3
(Diff revision 1)
> +It is depends on editing host whether spellcheck is turned on.  Input element's
> +default is off, but contenteditable's default is on.

How about:

Spellchecker of <input> element is off by default, however, if it's in a contenteditable element, spellchecker is on by default.

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

    
    

    
      
  

    
    

    
  
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/0cda6f01b04a
Reinitilize selection after destroying nsIEditingSession. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/476d6df50999
Part 2. Update spellcheck status on focused element after turning off contenteditable. r=masayuki

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/0cda6f01b04a
https://hg.mozilla.org/mozilla-central/rev/476d6df50999
Status: NEW → RESOLVED
Closed: 6 years ago

          status-firefox63:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

    
    

    
  
Comment on attachment 8990669 [details]
Bug 1443902 - Reinitilize selection after destroying nsIEditingSession.

Approval Request Comment
[Feature/Bug causing the regression]:
No

[User impact if declined]:
When writing a comment to github's issue, clicking any formant button such as bold causes caret disappears.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
N/A

[Is the change risky?]:
Low.

[Why is the change risky/not risky?]:
Initilize selection information into editor and caret visibility for current focused input/textarea element again when contenteditable attribute sets false.

[String changes made/needed]:
No.

        Attachment #8990669 -
        Flags: approval-mozilla-beta?

    
  

        Attachment #8990917 -
        Flags: approval-mozilla-beta?

    
    

    
  
FYI: The important thing of this bug is, even though this is not a recent regression, but this is annoying bug on GitHub. GitHub is loved by a lot of web designers, web application developers, etc. So, this fix improves UX of them a lot. So, I recommend release drivers to allow to uplift this into Beta.

    
    

    
  
Sounds like a good idea, thank you!  I agree, we want Firefox to behave correctly with github's editor. And, thanks for the new tests!

When we say "verified" usually this means, at least someone who is not the developer or reviewer has tested the fix in nightly or with a try build.
Flags: qe-verify+

    
    

    
  
Managed to reproduce the issue on Windows 10 x64, Mac OS X 10.12 and Ubuntu 16.04 with Nightly 60.0a1(2018-03-10) following the STR from description.
 
The issue is verified fixed on Windows 10 x64, Mac OS X 10.12 and Ubuntu 16.04 with the latest Nightly 63.0a1(2018-07-20).
Status: RESOLVED → VERIFIED

          status-firefox63:
          fixed → verified
Flags: qe-verify+

    
    

    
  
Comment on attachment 8990669 [details]
Bug 1443902 - Reinitilize selection after destroying nsIEditingSession.

Fix for an issue which became apparent recently with changes on github. 
Verified in nightly, let's uplift for beta 11.

        Attachment #8990669 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

        Attachment #8990917 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
The issue is verified fixed on Windows 10 x64, Mac OS X 10.12 and Ubuntu 16.04 x64 with the latest Beta 62.0b11.

          status-firefox62:
          fixed → verified

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: