1296050 - Typing in a textarea is sluggish when page contains LRM [[/ RLM]] control characters

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[ori.livneh]

Adding text to a <textarea> element is sluggish when the page contains a left-to-right (U+200E) or right-to-left (U+200F) mark. There is a noticeable delay after each keystroke.

To reproduce this issue, try typing a string of characters into the textarea of each of the pages below:

* Page with RLM: https://people.wikimedia.org/~ori/T143182/with-rlm.html
* Same page, no RLM: https://people.wikimedia.org/~ori/T143182/no-lrm.html

The page with the RLM is much slower.

This is affecting certain Wikipedia edit pages. See https://phabricator.wikimedia.org/T143182 for user reports.

Comment 1

[:Gijs (he/him)]

Masayuki-san, any chance this is an editor issue that you'd be able to look at?

Comment 2

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

Although, I'm still busy at least for some weeks.

I have no idea how this occurs. Could also be a performance issue around text frame or line breaker...

Comment 3

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

Odd, I don't see checking if a character is LRM in editor nor layout. So, I'm really not sure how we handle bidi at creating nsTextFrame...

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

This is a profiling report of this issue: https://is.gd/INMy5M

Looks like half of the time is on reflowing text frames, and the other half is painting.

It seems if there is not LRM, the page is only painted very few times rather than every key stroke, and reflow does not show up in profiling at all (which means it's too short to be caught).

So I guess we have some issue that when bidi is activated, we invalidate much more frames than necessary.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

So this is because when the data is changed:
* nsTextFrame::CharacterDataChanged() calls shell->FrameNeedsReflow() with nsIPresShell::eStyleChange; then
* PresShell::FrameNeedsReflow() calls MarkIntrinsicISizesDirty() on each of its ancestors; then
* nsBlockFrame::MarkIntrinsicISizesDirty() sets NS_BLOCK_NEEDS_BIDI_RESOLUTION flag; then
* nsBidiPresUtils::Resolve() is called when reflowing a block frame with that flag

nsBidiPresUtils would eventually mark every line dirty.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

So basically, I think there are two optimizations that we can do:

(1) Try hard to avoid bidi resolution. In majority of cases, we don't really need it at all.

(2) Try to limit bidi resolution to only the dirty paragraphs. Bidi resolution algorithm is paragraph-based, so do it on the whole block isn't necessary.

Neither (1) nor (2) looks like a trivial fix, but I guess (2) is probably less risky.

Comment 7

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

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #6)
> (2) Try to limit bidi resolution to only the dirty paragraphs. Bidi
> resolution algorithm is paragraph-based, so do it on the whole block isn't
> necessary.
> 
> Neither (1) nor (2) looks like a trivial fix, but I guess (2) is probably
> less risky.

I agree, (2) sounds reasonable.

Comment 8

[Jonathan Kew [:jfkthame]]

Looking at the code (and experimenting with the example, to confirm) I note that we activate the bidi-resolution code if any strong right-to-left characters _or_ directional control characters are found in the content. (See nsTextFragment::UpdateBidiFlag, which eventually leads to the nsDocument's mBidiEnabled flag getting set.)

But if there are no actual strong-RTL characters, and _only_ directional control character in the document is an LRM, we don't really need bidi resolution. Similarly if there's a LRE or LRO control: in the context of otherwise-purely-LTR content, these have no effect on the result. And if there is RTL content, such that these controls _do_ need bidi resolution, we can rely on the RTL content setting the bidi flag.

So I think we can make nsTextFragment::UpdateBidiFlag more selective about the directional controls that trigger setting the mIsBidi flag: only those controls that themselves introduce RTL-ness need to set this.

This will avoid the problem here for documents that have LRM characters -- which may have been automatically inserted by an editing tool just to ensure a known initial state in an element -- but do not actually contain any RTL content. It won't help for truly mixed-direction documents, though.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: When checking whether bidi resolution is needed, ignore Unicode directional-control characters that do not actually introduce right-to-left-ness

This avoids the bidi performance hit for documents that contain LRM but no actual bidi text, as noted above.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

But the document in comment 0 has an RLM, no?

It seems to me mBidiEnabled being set for this document is probably inevitable, but the majority of text doesn't really affected by the RLM, because they are in an <textarea>.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase with rlm from comment 0

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8787633 [details] [diff] [review]
When checking whether bidi resolution is needed, ignore Unicode directional-control characters that do not actually introduce right-to-left-ness

So given comment 10 (and tested locally to confirm), this patch doesn't fix this issue.

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10)
> But the document in comment 0 has an RLM, no?

Yes, so this patch won't help _that_ document. But it does help similar documents where the control character present is only LRM, which (IIUC) is the case for some of the wikimedia content being discussed. (See https://phabricator.wikimedia.org/T143182#2561436, and testcase at https://people.wikimedia.org/~ori/T143182/with-lrm.html.)

RLM will still trigger bidi resolution, but there's no need for LRM to do so, and that's the original case that people were encountering.

> 
> It seems to me mBidiEnabled being set for this document is probably
> inevitable, but the majority of text doesn't really affected by the RLM,
> because they are in an <textarea>.

Right, there's clearly a lot more scope for improvement there; but that sounds like it'll be a significantly more complex patch, with the associated risks.... 

OTOH, the proposed patch above is really simple and low-risk, so I think it makes sense to take it even though it doesn't address _all_ the cases where we get sluggish.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8787633 [details] [diff] [review]
When checking whether bidi resolution is needed, ignore Unicode directional-control characters that do not actually introduce right-to-left-ness

Review of attachment 8787633 [details] [diff] [review]:
-----------------------------------------------------------------

Please reconsider the r- here, as I still think this patch is worth having.

Even if/when we manage to do a better job about selectively applying bidi resolution only on parts of the content, it will still be beneficial to avoid setting the bidi flag on text-fragments that don't actually need it. The current behavior, of setting the flag as a result of _any_ directional-control character, even if it's only a left-to-right one, has no benefit and will always risk triggering unnecessary work.

It's true this does not fix all the cases described in this bug -- it doesn't help for content that actually HAS a right-to-left character or control anywhere -- but it does make a big difference for some of the real-world cases mentioned, and it's a cheap fix that we can apply immediately. Is there any reason _not_ to do this?

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8787633 [details] [diff] [review]
When checking whether bidi resolution is needed, ignore Unicode directional-control characters that do not actually introduce right-to-left-ness

Review of attachment 8787633 [details] [diff] [review]:
-----------------------------------------------------------------

OK. I guess this is an optimization worth taking.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

But given it doesn't fix this issue, we would need to keep it open even when we land the patch.

Comment 17

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68bcf4ac2aa2d448178442781dfba971f7ae68ea
Bug 1296050 - When checking whether bidi resolution is needed, ignore Unicode directional-control characters that do not actually introduce right-to-left-ness. r=xidorn

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/68bcf4ac2aa2

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> (2) Try to limit bidi resolution to only the dirty paragraphs. Bidi
> resolution algorithm is paragraph-based, so do it on the whole block isn't
> necessary.

To implement this, I think we can do:
* when nsTextFrame::CharacterDataChanged is called, find the boundary of paragraph this change touches
* record the boundary somewhere
* only resolve the corresponding paragraphs when there are boundaries recorded

For the specific case of typing, we can probably just do this optimization when there is only one paragraph involves, which should improve the input responsibility for majority of the cases.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jfkthame, maybe it's time to close this bug?

Comment 21

[Jonathan Kew [:jfkthame]]

I guess we can close this, as we landed a fix here for at least some of the original testcases. However, the wider issue of excessive bidi resolution in some cases still remains, and we should try to do something more (e.g. along the lines of comment 19) in a followup bug.