1360154 - nsIPlaintextEditor might have to have hasText property for UpdateOverlayTextVisibility

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Makoto Kato [:m_kato]]

Now, nsTextEditState::GetValue uses TextEditor::OutputToString to get current value when editor has focus.

nsTextEditState::UpdateOverlayTextVisibility uses this to check whether current value is empty.

I think that OutputToString is expensive whether value is empty or not.  So nsIPlainEditor might have to have hasText property for this.

Comment 1

[Makoto Kato [:m_kato]]

HTMLInputElement::IsValueEmpty might require it since element::UpdateState uses nsTextEditState::GetValue to check empty string.

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1360154 - Part 1. DocumentIsBody should return bool, not nsresult.

nsIEditRules::GetDocumentIsEmpty doesn't return error without null parameter, so we should use bool as return value instead.

Review commit: https://reviewboard.mozilla.org/r/138238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138238/

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Bug 1360154 - Part 2. Add fast path to check whether valus is emtpy.

Actually, we use GetValue to check whether value is empty or not for placeholder.  But since GetValue uses TextEditor::OutputToString when on editor, it is expensive.  Since editor has DocumentIsEmpty method, we should use it for this case.

Review commit: https://reviewboard.mozilla.org/r/138240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138240/

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8866635 [details]
Bug 1360154 - Part 1. DocumentIsBody should return bool, not nsresult.

https://reviewboard.mozilla.org/r/138238/#review141436

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8866636 [details]
Bug 1360154 - Part 2. Add fast path to check whether valus is emtpy.

https://reviewboard.mozilla.org/r/138240/#review141438

::: dom/html/nsTextEditorState.h:184
(Diff revision 1)
>      // not changed the cursor won't move.
>      eSetValue_MoveCursorToEndIfValueChanged = 1 << 3,
>    };
>    MOZ_MUST_USE bool SetValue(const nsAString& aValue, uint32_t aFlags);
>    void GetValue(nsAString& aValue, bool aIgnoreWrap) const;
> +  bool HasValue();

HasValue() is unclear to me. I think that HasNonEmptyValue(), IsEmpty() or something is better.

Comment 6

[Makoto Kato [:m_kato]]

Comment on attachment 8866636 [details]
Bug 1360154 - Part 2. Add fast path to check whether valus is emtpy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138240/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b508ea403285
Part 1. DocumentIsBody should return bool, not nsresult. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/fab114e7628c
Part 2. Add fast path to check whether valus is emtpy. r=masayuki

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b508ea403285
https://hg.mozilla.org/mozilla-central/rev/fab114e7628c