Closed Bug 1443902 Opened 2 years ago Closed Last year

Cursor disappears on GitHub when using formatting buttons

Categories

(Core :: Editor, defect, P3)

defect

Tracking

()

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

People

(Reporter: armenzg, Assigned: m_kato)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

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
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
Duplicate of this bug: 1442590
Summary: Cursor dissapears on GitHub when using formatting buttons → Cursor disappears on GitHub when using formatting buttons
Depends on: 1220696
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
(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: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Duplicate of this bug: 1321933
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
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.
You need to log in before you can comment on or make changes to this bug.