Closed Bug 1348073 Opened 7 years ago Closed 7 years ago

Consider enabling lazy frame construction for editable regions

Categories

(Core :: Layout, enhancement, P3)

Product:
Core
Component:
Layout
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: m_kato)

References

Details

Attachments

(4 files)

Bug 1348073 - Part 1. MoveCaret should move caret when focus node is whitespace only even if it is no frame.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1348073 - Part 2. Enable lazy frame construction for editable regions.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1348073 - Part 3. Adjust offset value in accessible/tests/mochitest/textcaret/test_general.html.
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1348073 - Part 4. Unnecessary VK_RIGHT to move caret on non-visual frame that is whitespace only node.
59 bytes, text/x-review-board-request
masayuki
: review+
Details 
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.
(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: QuantumFlow
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)
Flags: needinfo?(bbirtles)
Kato-san, are you able to look at this next week?
Flags: needinfo?(bbirtles) → needinfo?(m_kato)
Since I will change the editor transaction of input.value setter that doesn't remove text node, this won't help bug 1346723...
Kato-san: Please let us know if you're able to get to this one after bug 1346723.
Assignee: nobody → m_kato
(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.
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)
editing/run/backcolor.html

nsFrameSelection::GetCellLayout returns nullptr, so this tests are failure.  How do we create frame from cell?
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.
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)
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)
Thanks, Alexander.
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
Depends on: 1373476
Depends on: 699703
Depends on: 1373477
Depends on: 1381710
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.
(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)
No longer depends on: 1373477
Comment on attachment 8899712 [details]
Bug 1348073 - Part 1. MoveCaret should move caret when focus node is whitespace only even if it is no frame.

https://reviewboard.mozilla.org/r/171038/#review176230

LGTM.

::: dom/base/Selection.h:149
(Diff revision 1)
>    NS_IMETHOD   GetPrimaryFrameForAnchorNode(nsIFrame **aResultFrame);
> -  NS_IMETHOD   GetPrimaryFrameForFocusNode(nsIFrame **aResultFrame, int32_t *aOffset, bool aVisual);
> +  nsresult GetPrimaryFrameForFocusNode(nsIFrame** aResultFrame,
> +                                       int32_t* aOffset, bool aVisual);

You're making GetPrimaryFrameForFocusNode non-virtual here, which looks fine although it's not strictly needed for this bug; but while you're here, why not do the same to the similar GetPrimaryFrameForAnchorNode method immediately above? That would keep the parallel between these two declarations, and neither of them needs to be virtual AFAICS.

Also while you're touching this, the declarations use the name `aResultFrame`, but the definitions in the .cpp file call this param `aReturnFrame` (which is also what you've used for the helper below). It would be nice to make this consistent, one way or the other.

And for a bonus, maybe also delete the commented-out GetPrimaryFrameForRangeEndpoint above, which has pointlessly languished there for years?
Attachment #8899712 - Flags: review?(jfkthame) → review+
Comment on attachment 8899715 [details]
Bug 1348073 - Part 4. Unnecessary VK_RIGHT to move caret on non-visual frame that is whitespace only node.

https://reviewboard.mozilla.org/r/171044/#review176236

Does this change mean that actual behavior for users is changed? If not so, i.e., it's caused by synthesizing keyboard events synchronously immediately after setting focus to the editor, I think that this should be rewritten with async/await and SimpleTest.executeSoon().
Comment on attachment 8899714 [details]
Bug 1348073 - Part 3. Adjust offset value in accessible/tests/mochitest/textcaret/test_general.html.

https://reviewboard.mozilla.org/r/171042/#review176378
Attachment #8899714 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8899713 [details]
Bug 1348073 - Part 2. Enable lazy frame construction for editable regions.

https://reviewboard.mozilla.org/r/171040/#review176440

Hurray! \o/  Thank you so much.
Attachment #8899713 - Flags: review?(ehsan) → review+
Comment on attachment 8899715 [details]
Bug 1348073 - Part 4. Unnecessary VK_RIGHT to move caret on non-visual frame that is whitespace only node.

https://reviewboard.mozilla.org/r/171044/#review176236

Yes, but old behaviour is strange.  After landing this bug fixes, this behaviour becomes same as Edge and Blink.

Example, (https://jsfiddle.net/zvvbquL1/1/)

HTML

<div contenteditable="true" id="edit"> <span contenteditable="false">A</span> ; <span contenteditable="false">B</span> ; <span contenteditable="false">C</span> </div>

JavaScript

var edit = document.getElementById("edit");
edit.focus();
window.getSelection().collapse(edit.firstChild, 0);

When running this, content will be rendered as

[caret]A ; B ; C

In this situation, when user types [VK_RIGHT] key, Edge and Blink can move caret to after 'A', but Gecko still keeps same caret position.  Because there is empty frame that is whitepsace only node before 'A', so first [VK_RIGHT] moves to empty frame.  By part 1 and part 2's fix, we don't create empty frame that is whitespace only.  So it is unnecessary to type [VK_RIGHT] twice.
Comment on attachment 8899715 [details]
Bug 1348073 - Part 4. Unnecessary VK_RIGHT to move caret on non-visual frame that is whitespace only node.

https://reviewboard.mozilla.org/r/171044/#review176612

Sounds great! Thank you for the explanation.
Attachment #8899715 - Flags: review?(masayuki) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/118150d80e15
Part 1. MoveCaret should move caret when focus node is whitespace only even if it is no frame. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/6b27a9f18281
Part 2. Enable lazy frame construction for editable regions. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/81e9aa4a4fbc
Part 3. Adjust offset value in accessible/tests/mochitest/textcaret/test_general.html. r=surkov
https://hg.mozilla.org/integration/autoland/rev/ee7f89afcbff
Part 4. Unnecessary VK_RIGHT to move caret on non-visual frame that is whitespace only node. r=masayuki
Depends on: 1393171
ehsan, do you recall if there was any particular benchmark / site that we anticipated would get a speedup from this?  (Was this something that speedometer depended on?)

(If so, I'd be interested in capturing [or having QA/somebody capture] profiles before+after to see the impact here.)
Flags: needinfo?(ehsan)
Yes, this showed up under the profiles of bug 1346723 (for example, see http://bit.ly/2wsQBt5 which is the same profile that I posted in bug 1346723 comment 44).  Also I have seen this in various profiles of sites that use contenteditable like Gmail in the mail compose UI.
Flags: needinfo?(ehsan)
simple benchmark is https://jsfiddle.net/g5cdfk7n/. After landing this, this benchmark is more than twice faster than original.
Thanks! That's benchmark's results is a good rough validation of the speedup here -- that's mostly what I was looking for.

Hooray!
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: