Consider enabling lazy frame construction for editable regions

NEW
Assigned to

Status

()

Core
Layout
P3
normal
5 months ago
6 days ago

People

(Reporter: Ehsan, Assigned: m_kato)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qf:p1])

Historically we have disabled lazy frame construction for editable regions.  I don't remember any reasons why this is currently needed, and neither does bz.

needinfo-ing a few other people who may be aware of anything that may depend on eager frame construction for editable regions?  If there is no more dependencies on this, removing this would enable some nice optimizations by removing frame construction from some code paths that JS can trigger, such as bug 1346723.
Flags: needinfo?(mats)
Flags: needinfo?(dholbert)
Flags: needinfo?(dbaron)
(Sorry, I don't have any extra info/context on this.)
Flags: needinfo?(dholbert)
I don't remember anything here.  Perhaps hg annotate / git blame does?
Flags: needinfo?(dbaron)
I disabled it when originally implementing lazy frame construction because of failures in some tests. At the time it did not look easy to fix them, but I didn't spend a lot of time looking into it.
Still tests failing

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c13a8e84208412d7f43d8d0df095db3ad2496472&group_state=expanded
(In reply to Timothy Nikkel (:tnikkel) from comment #3)
> I disabled it when originally implementing lazy frame construction because
> of failures in some tests. At the time it did not look easy to fix them, but
> I didn't spend a lot of time looking into it.

Ah yes, makes sense.  These are places where the editor code needs to be flushing frames, but it isn't.

The good news is that now with rr it should be fairly easy to go back from the failure to the place the frame tree was accessed and that's probably around the place where a flush is needed.  (Maybe GetPrimaryFrame() would be a good place to break on.)

I think this would be a really valuable optimization to do for QF.  Do you have cycles to take it, Timothy?
Blocks: 1337841
Flags: needinfo?(tnikkel)
(Sorry, I don't have any extra info/context on this.)
Flags: needinfo?(mats)
Whiteboard: [qf:p1]
Note that for purposes of bug 1346723 this may not be enough, because we also disable lazy frame construction for native anon stuff, which text editor is.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #7)
> Note that for purposes of bug 1346723 this may not be enough, because we
> also disable lazy frame construction for native anon stuff, which text
> editor is.

Definitely not enough. Enabling lazy frame construction for editable content doesn't change the numbers for the testcase in that bug at all.
Flags: needinfo?(tnikkel)
Right, but I also suspect there is no good reason for that restriction either.

But this bug matters much more for the performance of web mail editors, blog editors and the like.
No longer blocks: 1346723
Could you describe a bit more what needs to be done to track down the test failures?  I understand the problem is some places need to flush frames but don't.  How would I go about finding these places, and what do I do to make them flush frames?  What sort of things would require frames to be flushed?
Flags: needinfo?(ehsan)
Basically you just apply a patch like this

https://hg.mozilla.org/try/rev/5d903da115ab4590ab5c25fa7796a0c957c80954

and track down the root cause of one of these test failures

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c13a8e84208412d7f43d8d0df095db3ad2496472

Flushing frames means calling FlushPendingNotifications on the document or presshell with a FlushType of Frames or greater. This makes sure that the layout model of frames is up to do with the content model of dom nodes. Things that would require a flush would be anything that tries to access a frame, or access any other information that is stored on frames.

Although flushing is one way to fix this, there might be cases where it's better to refactor the editor to not require up-to-date frames. In general we want to avoid flushing to improve performance.
If you add new FlushPendingNotifications calls: please note that arbitrary objects may have
been destroyed, or changed, after that call.  All objects that you touch subsequently needs
to already have a strong ref (on the stack preferably), not only in the method of the call
itself but also in all callers of that method, and callers of those methods etc, out to
the nearest JS API entry point. (or you could use a Weak reference type of some kind and
do a null-check)

FlushPendingNotifications is very dangerous to call, please avoid it if at all possible.
I've been looking at this for a while, but haven't made any actual progress yet.  Lots of trying to figure out how to get debuggers and rr to work, and waiting for compilers and rr to do their job, but not so much actual results.  I'm working only a bit more today and then a bit on Sunday, and after that I'll be working April 18-26 before my next break.  So I'll be available to look at this more on April 18 if no one has taken it before then.

(Maybe it's just me, but I find printf-and-recompile to be more useful than debuggers . . .)
So looking at the tests/test_bug1315065.html failure, the patch causes selCont->CharacterExtendForDelete() to fail in TextEditor::ExtendSelectionForDelete.  I can imagine that this would require accurate frames available.  But putting this at the beginning of the method doesn't help:

    nsCOMPtr<nsIDocument> doc = GetDocument();
    doc->FlushPendingNotifications(FlushType::Frames);

So I don't know what to do.  Any suggestions?
The wpt failure (editing/run/backcolor.html) is because in Selection::getTableCellLocationFromRange, the call to mFrameSelection->GetCellLayout returns null.  Adding

  nsIPresShell* presShell = GetPresShell();
  if (presShell) {
    presShell->FlushPendingNotifications(FlushType::Frames);
  }

beforehand fixes the exception, but the test still fails with

  assert_equals: Wrong result returned (after color normalization) expected "rgb(0, 255, 255)" but got "rgba(0, 0, 0, 0)"

More debugging needed to figure that one out.
(I'm not saying that these FlushPendingNotifications should actually be checked in, I'm just trying to figure out some sort of fix as a starting point.)
Sorry, the fix I gave for the wpt failure actually fixes the unexpected fail entirely.  The tests still fail, but it's expected.  Still to do:

* editor/libeditor/tests/test_bug1315065.html (see comment 14)
* chrome://mochitests/content/a11y/accessible/tests/mochitest/textcaret/test_general.html
* editor/libeditor/tests/test_htmleditor_keyevent_handling.html
* editor/libeditor/tests/test_htmleditor_keyevent_handling.html: To do with events fired on tabbing when tabbing is disabled.  I've never looked at this code before, and there were no warnings printed to the console before the failure.
* accessible/tests/mochitest/textcaret/test_general.html: I don't even know what the features tested here are supposed to do.

I can look more into the test_bug1315065.html failure on Sunday if someone replies to comment 14, and the wpt failure has a fix described in comment 15.  I don't know if I'm a good person to work on the other two failures.
(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #14)
> So looking at the tests/test_bug1315065.html failure, the patch causes
> selCont->CharacterExtendForDelete() to fail in
> TextEditor::ExtendSelectionForDelete.  I can imagine that this would require
> accurate frames available.  But putting this at the beginning of the method
> doesn't help:
> 
>     nsCOMPtr<nsIDocument> doc = GetDocument();
>     doc->FlushPendingNotifications(FlushType::Frames);
> 
> So I don't know what to do.  Any suggestions?

Perhaps something else is failing before this that also needs a flush? You could try flushing layout in the test before the problematic line (getBoundingClientRect will flush layout from js) to see if that fixes the problem. That's not a proper fix but gives helpful information to proceed with, and can be used as a general method to narrow down the problem in any test.
Adding the line

  p.getBoundingClientRect();

before the first failing is() in test_bug1315065.html does not fix the failure.
Looks like Timothy responded to the original needinfo...  Comment 20 basically suggests that the necessary flush needs to happen somewhere inside Gecko which isn't surprising.  From what I can read in the bug my guess is that something in InsertNode (assuming you're here <http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/editor/libeditor/tests/test_bug1315065.html#35> is dirtying layout and then not flushing later on when it implicitly expects the frame tree to be up to date...  Unfortunately it's hard to say something less vague without having debugged this a bit... :/
Flags: needinfo?(ehsan)

Updated

4 months ago
Flags: needinfo?(bbirtles)
Kato-san, are you able to look at this next week?
Flags: needinfo?(bbirtles) → needinfo?(m_kato)
(Assignee)

Comment 23

3 months ago
Since I will change the editor transaction of input.value setter that doesn't remove text node, this won't help bug 1346723...

Comment 24

3 months ago
Kato-san: Please let us know if you're able to get to this one after bug 1346723.
Assignee: nobody → m_kato
(Assignee)

Comment 25

3 months ago
(In reply to Jet Villegas (:jet) from comment #24)
> Kato-san: Please let us know if you're able to get to this one after bug
> 1346723.

Selection methods (ex. CharacterExtendForBackspace) don't seem to flush layout since ContentQuery (ex. eQueryCaretRect and eQueryTextRect) doesn't flush it.  Since editor doesn't know cluster character from Unicode code point, editor uses Selection's method.

OK, I will handle it.
(Assignee)

Comment 26

3 months ago
editor/libeditor/tests/test_bug1315065.html

FlushType::Layout doesn't create focus node's frame because this node has NS_CREATE_FRAME_IF_NON_WHITESPACE.  nsFrameSelection::MoveCaret calls FlushPendingNotification, but it doesn't call nsCSSFrameConstructor::EnsureFrameForTextNode.
Flags: needinfo?(m_kato)
(Assignee)

Comment 27

3 months ago
editing/run/backcolor.html

nsFrameSelection::GetCellLayout returns nullptr, so this tests are failure.  How do we create frame from cell?
(Assignee)

Comment 28

3 months ago
accessible/tests/mochitest/textcaret/test_general.html 

If frame is null, nsAccessibilityService::CreateAccessible doesn't create Accessible object.  So if frame creation is lazily, it doesn't seem to calculate CharacterCount() well.  text node's frame isn't created due to white space only.
(Assignee)

Comment 29

3 months ago
Hi Alexander, 

I am investigating test failure of accessible/tests/mochitest/textcaret/test_general.html when we enable lazy frame construction on editable content.

When enabling it, accessible/tests/mochitest/textcaret/test_general.html is failure (see comment #11 for patch and error details).  Because HyperTextAccessible::CharacterCount returns 15, not 16.

nsAccessibilityService::CreateAccessible creates Accessible object.  But when content is white space only (ex. \r), we might not create primary frame, then a11y doesn't create Accessible object.  Now, we always create primary frame when content node is editable, so it has primary frame.  If lazy frame construction is turned on editable content, it might not have primary frame.

So, for test, when I add a hack that nsCSSFrameConstructor::MaybeConstructLazily always returns false to turn off all lazily frame construction, this test is also failure.  So I think that we should not create Accessible object when content node is white space only (Now, nsAccessibilityService::CreateAccessible only check it into table).  Because nsAccessibilityService::CreateAccessible doesn't check the frame when there is no primary frame that has white space only.

Is is correct?  If not, could you fix this?
Flags: needinfo?(surkov.alexander)

Comment 30

3 months ago
a11y needs an accessible objects for whitespace only frames if they are rendered, if not, then no object is needed. If I read you right then no fix on a11y side is required; we should make sure however, that accessible object is created when its primary frame is created. Otherwise, you could go with just fixing the numbers in the test.
Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 31

3 months ago
Thanks, Alexander.
(Assignee)

Comment 32

3 months ago
editor/libeditor/tests/test_htmleditor_keyevent_handling.html 

HTMLEditor::GetCellAt doesn't returns cell since no flame.  Same root cause of editing/run/backcolor.html
(Assignee)

Updated

2 months ago
Duplicate of this bug: 687981
(Assignee)

Updated

2 months ago
Depends on: 1373476
(Assignee)

Updated

2 months ago
Depends on: 699703
(Assignee)

Updated

2 months ago
Depends on: 1373477
(Assignee)

Updated

a month ago
Depends on: 1381710

Updated

a month ago
Priority: -- → P3
I'm retriaging [qf:p1] bugs that haven't been touched in a little while -- Makoto, is this looking like it'll be doable for 57?  If not, we might want to adjust its priority.
(Assignee)

Comment 35

18 days ago
(In reply to Daniel Holbert [:dholbert] (PTO August 3-7) from comment #34)
> I'm retriaging [qf:p1] bugs that haven't been touched in a little while --
> Makoto, is this looking like it'll be doable for 57?  If not, we might want
> to adjust its priority.

Although I think that we should fix bug 1373476 at first, test_native_key_bindings_mac.html test isn't passed.

Also, I try fixing bug 1381710 to resolve this since DOM's table selection depends on frame.  But bug 1381710 also occurs on non-editable node...  So bug 1381710 is resolved, we can fix this with bug 1373476.  (I already have a WIP)
(Assignee)

Updated

14 days ago
No longer depends on: 1373477
(Assignee)

Comment 36

6 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cfdd626d187c679a5ba1aa7db2596de631737cc
You need to log in before you can comment on or make changes to this bug.