Open Bug 1787062 Opened 2 years ago Updated 2 years ago

Incorrect height measured when "overflow" style is changed in textarea before bug 1787088, because of editor re-initialization

Categories

(Core :: DOM: Editor, defect)

Firefox 104
defect

Tracking

()

Tracking Status
firefox-esr91 --- unaffected
firefox-esr102 --- unaffected
firefox104 --- wontfix
firefox105 --- wontfix
firefox106 --- fix-optional

People

(Reporter: dl1644, Unassigned)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/104.0.0.0 Safari/537.36

Steps to reproduce:

  1. Go to https://codepen.io/dl1644/pen/oNqOOaL?editors=1111
  2. Type into the left most textarea until the text's height exceeds the textarea's height.
  3. Type into the right most textarea until the textarea's begins to grow.

Actual results:

Note that the left most textarea's height doesn't grow to match the text's height but the right most textarea's height does.

Expected results:

Both textareas should grow to match the text's height as you type into them. Before Firefox 104, we were able to change a textarea's overflow style to "hidden" to remove any vertical scrollbars that may be present in the textarea and thus measure the true height of the textarea's text. We would then set the textarea's height to the measured height, resulting in a textarea that would grow/shrink to fit the amount of text within it.

After Firefox 104, the above no longer works as demonstrated by the left most textarea. Substituting the "overflow: hidden" approach with "scrollbar-width: none" doesn't work either. Note that getting rid of the "target.style.overflow = prevOverflow;" line fixes the height measurement but we want to preserve the previous "overflow" setting. Note that the above codepen works in Chrome and Safari.

Status: UNCONFIRMED → NEW
Has STR: --- → yes
Component: Untriaged → CSS Parsing and Computation
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Regressed by: 1778983

Thanks for the report, will look.

It seems like something is triggering a style change while processing the previous style change, which is obviously unsound, and before my patch was getting wallpapered by an extra style update. Workaround without any negative side effects should be to call target.scrollHeight or anything else that updates layout before updating the computation. e.g:

overflowTextArea.addEventListener("input", (e) => {
  let {target} = e;
  let prevOverflow = target.style.overflow;
  target.style.overflow = 'hidden';
  target.style.height = 'auto';
  target.scrollHeight; // Inserted this line.
  target.style.height = `${target.scrollHeight + (target.offsetHeight - target.clientHeight)}px`;
  target.style.overflow = prevOverflow;
});
Assignee: nobody → emilio
Flags: needinfo?(emilio)

Bug 1787088 adds an optimization that avoids this bug, but there's more to do potentially, since I haven't found the root cause yet.

Ok, so the root cause is that the overflow change causes us to re-initialize the editor because of the re-frame, and thus we end up with a restyle where the editor content is not there yet. The double ProcessPendingRestyles wallpapered over this.

Masayuki, I know it is probably a can of worms, but how hard would it be to make sure that the editor anonymous content is ready when we actually create the anonymous content (so in nsTextControlFrame::CreateAnonymousContent)?

This "need a script runner to go out of scope to create a sane layout" is incompatible with container queries.

Component: CSS Parsing and Computation → DOM: Editor
Flags: needinfo?(emilio) → needinfo?(masayuki)
Summary: Incorrect height measured when "overflow" style is changed in textarea → Incorrect height measured when "overflow" style is changed in textarea before bug 1787088, because of editor re-initialization

Ah, for reference, editor initialization stack:

(rr) bt
#0  nsCOMPtr<nsINode>::assign_assuming_AddRef(nsINode*) (this=0x7fa2d4e2d0a0, aNewPtr=0x7fa2d1d0c550) at /home/emilio/src/moz/gecko-2/obj-debug/dist/include/nsCOMPtr.h:423
#1  0x00007fa2e919234e in nsCOMPtr<nsINode>::assign_with_AddRef(nsISupports*) (this=0x7fa2d4e2d0a0, aRawPtr=0x7fa2d1d0c550) at /home/emilio/src/moz/gecko-2/obj-debug/dist/include/nsCOMPtr.h:1179
#2  nsCOMPtr<nsINode>::operator=(nsINode*) (this=0x7fa2d4e2d0a0, aRhs=0x7fa2d1d0c550) at /home/emilio/src/moz/gecko-2/obj-debug/dist/include/nsCOMPtr.h:690
#3  mozilla::dom::Document::SetServoRestyleRoot(nsINode*, unsigned int) (this=0x7fa2d4e2c700, this@entry=0x1, aRoot=aRoot@entry=0x7fa2d1d0c550, aDirtyBits=4096, aDirtyBits@entry=2271844120)
    at /home/emilio/src/moz/gecko-2/obj-debug/dist/include/mozilla/dom/DocumentInlines.h:39
#4  0x00007fa2e916840d in mozilla::dom::NoteDirtyElement(mozilla::dom::Element*, unsigned int) (aElement=0x7fa2d1d0c550, aBits=4107932576, aBits@entry=4096) at /home/emilio/src/moz/gecko-2/dom/base/Element.cpp:4574
#5  0x00007fa2e9168802 in mozilla::dom::Element::NoteDescendantsNeedFramesForServo() (this=0x7fa2d4e2d0a0) at /home/emilio/src/moz/gecko-2/dom/base/Element.cpp:4670
#6  0x00007fa2eb7b9d58 in nsCSSFrameConstructor::ContentAppended(nsIContent*, nsCSSFrameConstructor::InsertionKind) (this=0x7fa2f4da8e80, aFirstNewContent=0x7fa2d1d06800, aInsertionKind=nsCSSFrameConstructor::InsertionKind::Async)
    at /home/emilio/src/moz/gecko-2/layout/base/nsCSSFrameConstructor.cpp:6788
#7  0x00007fa2eb773606 in mozilla::PresShell::ContentAppended(nsIContent*) (this=<optimized out>, aFirstNewContent=0x7fa2d1d06800) at /home/emilio/src/moz/gecko-2/layout/base/PresShell.cpp:4551
#8  0x00007fa2e91bcd29 in mozilla::dom::MutationObservers::NotifyContentAppended(nsIContent*, nsIContent*)::$_10::operator()(mozilla::PresShell*) const (aPresShell=0x7fa2d4e2d0a0, this=<optimized out>)
    at /home/emilio/src/moz/gecko-2/dom/base/MutationObservers.cpp:162
#9  Notify<(IsRemoval)0, (ShouldAssert)1, mozilla::dom::MutationObservers::NotifyContentAppended(nsIContent*, nsIContent*)::$_10, mozilla::dom::MutationObservers::NotifyContentAppended(nsIContent*, nsIContent*)::$_11>(nsINode*, mozilla::dom::MutationObservers::NotifyContentAppended(nsIContent*, nsIContent*)::$_10&, mozilla::dom::MutationObservers::NotifyContentAppended(nsIContent*, nsIContent*)::$_11&)
    (aNode=<optimized out>, aNotifyPresShell=<optimized out>, aNotifyObserver=<optimized out>) at /home/emilio/src/moz/gecko-2/dom/base/MutationObservers.cpp:97
#10 mozilla::dom::MutationObservers::NotifyContentAppended(nsIContent*, nsIContent*) (aContainer=<optimized out>, aFirstNewContent=0x7fa2d1d06800) at /home/emilio/src/moz/gecko-2/dom/base/MutationObservers.cpp:163
#11 0x00007fa2e9282100 in nsINode::InsertChildBefore(nsIContent*, nsIContent*, bool, mozilla::ErrorResult&) (this=0x7fa2d1d0c550, aKid=0x7fa2d1d06800, aBeforeThis=0x0, aNotify=false, aRv=...)
    at /home/emilio/src/moz/gecko-2/dom/base/nsINode.cpp:1601
#12 0x00007fa2eb57aea7 in mozilla::EditorBase::EnsureEmptyTextFirstChild() (this=this@entry=0x7fa2f4df1e00) at /home/emilio/src/moz/gecko-2/editor/libeditor/EditorBase.cpp:371
#13 0x00007fa2eb57b1af in mozilla::EditorBase::InitEditorContentAndSelection() (this=0x7fa2f4df1e00) at /home/emilio/src/moz/gecko-2/editor/libeditor/EditorBase.cpp:383
#14 0x00007fa2eb66d62d in mozilla::TextEditor::Init(mozilla::dom::Document&, mozilla::dom::Element&, nsISelectionController&, unsigned int, mozilla::UniquePtr<mozilla::PasswordMaskData, mozilla::DefaultDelete<mozilla::PasswordMaskData> >&&) (this=0x7fa2f4df1e00, aDocument=..., aAnonymousDivElement=..., aSelectionController=..., aFlags=<optimized out>, aPasswordMaskData=<optimized out>) at /home/emilio/src/moz/gecko-2/editor/libeditor/TextEditor.cpp:139
#15 0x00007fa2ea609e0e in mozilla::TextControlState::PrepareEditor(nsTSubstring<char16_t> const*) (this=0x7fa2d3caea00, aValue=aValue@entry=0x7fa2d3cc5498) at /home/emilio/src/moz/gecko-2/dom/html/TextControlState.cpp:1846
#16 0x00007fa2ea624bf1 in mozilla::PrepareEditorEvent::Run() (this=<optimized out>) at /home/emilio/src/moz/gecko-2/dom/html/TextControlState.cpp:1608
#17 0x00007fa2e901fae7 in nsContentUtils::RemoveScriptBlocker() () at /home/emilio/src/moz/gecko-2/dom/base/nsContentUtils.cpp:5880
#18 0x00007fa2eb771ecf in nsAutoScriptBlocker::~nsAutoScriptBlocker() (this=<optimized out>) at /home/emilio/src/moz/gecko-2/dom/base/nsContentUtils.h:3565
#19 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) (this=0x7fa2d3c83000, aFlush=...) at /home/emilio/src/moz/gecko-2/layout/base/PresShell.cpp:4375
#20 0x00007fa2e9142c5c in mozilla::PresShell::FlushPendingNotifications

Bug 1787088 makes this possibly less pressing. For some reason I thought it'd be trivial to repro switching display rather than overflow, but I couldn't create a test-case... But it's way too late today, so maybe I messed it up.

Assignee: emilio → nobody

If it's fine to set the value at calling TextEditor::Init for layout (i.e., inserting the text node with default/current value), I think that the caller of EnsureEmptyTextFirstChild() should be changed from EditorBase::InitEditorContentAndSelection to TextEditor::Init, and make TextEditor::Init take the initial value. Then, I guess it'll work for the reported case.

However, if the editor is for <textarea> and it ends with empty line, a padding <br> for making the empty last line visible.
https://searchfox.org/mozilla-central/rev/30ac44262374be08ccd4262ee36f0716152868d3/editor/libeditor/EditorBase.cpp#408

So I think that the approach does not work in the case. I guess that we need to put it off to append the anonymous <div> or make TextControlState reuse TextEditor and the anonymous subtree if possible. (E.g., putting off to clean up them in the next event loop.) So I think it requires a big change.

Flags: needinfo?(masayuki)

Ah, but I don't know when nsTextControlFrame::CreateAnonymousContent is called. I assume that it's called before the call of TextEditor::Init.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Thanks for the report, will look.

It seems like something is triggering a style change while processing the previous style change, which is obviously unsound, and before my patch was getting wallpapered by an extra style update. Workaround without any negative side effects should be to call target.scrollHeight or anything else that updates layout before updating the computation. e.g:

overflowTextArea.addEventListener("input", (e) => {
  let {target} = e;
  let prevOverflow = target.style.overflow;
  target.style.overflow = 'hidden';
  target.style.height = 'auto';
  target.scrollHeight; // Inserted this line.
  target.style.height = `${target.scrollHeight + (target.offsetHeight - target.clientHeight)}px`;
  target.style.overflow = prevOverflow;
});

Thanks for the workaround!

Severity: -- → S3
Blocks: 1788981
Duplicate of this bug: 1799404
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: