1726297 - `ContentEventHandler` shouldn't try to get text frame from white-space only text nodes

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

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

I see a lot of warnings running some tests:

WARNING: NS_ENSURE_TRUE(frame) failed: dom/events/ContentEventHandler.cpp:1316
WARNING: 'NS_FAILED(rv)', dom/events/ContentEventHandler.cpp:1753
WARNING: '!brRectRelativeToLastTextFrame.IsValid()', dom/events/ContentEventHandler.cpp:1983
WARNING: 'queryTextRectsEvent.Failed()', widget/ContentCache.cpp:285
WARNING: '!QueryCharRectArray(aWidget, startOffset, length, rects)', widget/ContentCache.cpp:350

This is caused by that it reaches invisible white-spaces only text node at end of the range, it does not have a frame and caused failing to store the text rect properly. Therefore, if the text node does not have primary frame, it should be fallen back to the guessing part.

Comment 1

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

Setting to S3 because this may cause IME candidate window position unstable.

Comment 2

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

Attachment: Bug 1726297 - part 1: Make GetFrameForTextRect stop warning no primary frame case r=smaug

If a node is invisible (for example, its display property is none, the node
is a text node whose text only white-spaces and around a block boundary, etc),
the node is not associated with a primary frame is not illegal case. Therefore,
it should stop spamming in the terminal and log in automated tests.

Comment 3

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

Attachment: Bug 1726297 - part 2: Make `ContentEventHandler::Get(First|Last)FrameInRangeForTextRect` ignore invisible nodes r=smaug

The callers of them want to return text rect for first visible thing in the
given range to make IME place UI around there. Therefore, it should ignore
invisible text which won't be interactive with IME.

Depends on D160590

Comment 4

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

Attachment: Bug 1726297 - part 3: Make callers of `ContentEventHandler::GuessLineBreakerRectAfter` use with visible text node r=smaug

It guesses caret rect at end of a text node, i.e., it can work as expected
only when the text node is visible. Therefore, if a text node for computing
the rect is invisible, the callers need to use another fallback path.

Depends on D160591

Comment 5

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

Attachment: Bug 1726297 - part 4: Make `ContentEventHandler::OnQueryTextRectArray` fills invisible character rects with caret rect before next visible character r=smaug

This patch does not make ContentEventHandler return consistent rect
for invisible text node, however, it should be okay for now because users
cannot put caret into invisible text node and cannot type text into it.

For avoiding the warning spam of ContentCacheInChild in automated tests,
ContentEventHandler::OnQueryTextRectArray shouldn't give it up correcting
character rects in invisible text nodes. And as mentioned above, using
similar rects to visible character around there is okay. Therefore, this
patch makes OnQueryTextRectArray fills invisible text rects with caret
rect before following visible character if they are followed by visible
characters. Otherwise, i.e., if invisible text rects are the last things
in the range, make it use caret rect after the last visible character.

Note that if the range is completely in invisible nodes, the value will be
computed in the fallback part of the method. It still has issues, but it
does not happen so many times in the automated tests. Therefore, this patch
does not treat the case.

Depends on D160592

Comment 6

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

Attachment: Bug 1726297 - part 5: Make `ContentEventHandler` use common utility methods to compute caret rect from a character rect r=smaug

Unfortunately, this class does not fully support bidi text because desktop
IME are typically created for East-Asian languages. Therefore, the new methods
still do not support RTL text cases.

Depends on D160593

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94ff72f5eb14
part 1: Make GetFrameForTextRect stop warning no primary frame case r=smaug
https://hg.mozilla.org/integration/autoland/rev/d0ff1c0606d6
part 2: Make `ContentEventHandler::Get(First|Last)FrameInRangeForTextRect` ignore invisible nodes r=smaug
https://hg.mozilla.org/integration/autoland/rev/02b97e347cb1
part 3: Make callers of `ContentEventHandler::GuessLineBreakerRectAfter` use with visible text node r=smaug
https://hg.mozilla.org/integration/autoland/rev/865311ab3d6d
part 4: Make `ContentEventHandler::OnQueryTextRectArray` fills invisible character rects with caret rect before next visible character r=smaug
https://hg.mozilla.org/integration/autoland/rev/18db48a49cea
part 5: Make `ContentEventHandler` use common utility methods to compute caret rect from a character rect r=smaug

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/94ff72f5eb14
https://hg.mozilla.org/mozilla-central/rev/d0ff1c0606d6
https://hg.mozilla.org/mozilla-central/rev/02b97e347cb1
https://hg.mozilla.org/mozilla-central/rev/865311ab3d6d
https://hg.mozilla.org/mozilla-central/rev/18db48a49cea

Comment 9

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

Number of lines of the log of mozilla-central:

• Linux: editing/run: 6522 -> 3508 (x 0.58)
• Linux: editing/other: 47096 -> 22926 (x 0.49)
• Windows: editing/run: 6537 -> 3538 (x 0.54)
• Windows: editing/other: 44532 -> 22107 (x 0.50)

I don't see reasonable difference of running time in the treeherder logs and actual running result in Windows. So the outputting cost on Windows may be much less than older Windows or not aborting the computation of invisible text rect may consume the saved output cost.