687981 - Investigate enabling lazy frame construction for editable content

Status: RESOLVED DUPLICATE of bug 1348073

(Core :: Layout, defect)

Description

[(no longer active)]

Timothy told me that he's willing to investigate what kinds of stuff fail in the editor world which caused him to disable lazy frame construction in editable content before.

This can help with editing performance in web apps, Orion included.

Comment 1

[Mozilla RelEng Bot]

Try run for 728387aa2de7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=728387aa2de7
Results (out of 32 total builds):
    success: 20
    warnings: 12
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-728387aa2de7

Comment 2

[Timothy Nikkel (:tnikkel)]

So the IsEditable check that we use to disable lazy frame construction is only one check that might force eager frame construction for editable elements. We also disable lazy frame construction for native anonymous content (and in native anonymous subtrees). This is because there is no easy way to traverse native anonymous content when we are traversing the content tree looking for nodes that need frames constructed later.

Comment 3

[Boris Zbarsky [:bzbarsky]]

That's fine for webapps, which are editing non-anonymous DOMs.

Comment 4

[(no longer active)]

This is a *lot* of failures.   :(

Comment 5

[Timothy Nikkel (:tnikkel)]

One or two patches might fix the majority of them. (Just a hunch, based on nothing.)

Comment 6

[(no longer active)]

(In reply to Timothy Nikkel (:tn) from comment #5)
> One or two patches might fix the majority of them. (Just a hunch, based on
> nothing.)

Do you feel like delving into this a bit, and filing some editor bugs for me to take over?

Comment 7

[Timothy Nikkel (:tnikkel)]

I think I'll pick one failure and investigate it more closely. That way if other failures are fixed by the same patch that fixes the first I don't waste time figuring out that they have the same root cause.

Comment 8

[(no longer active)]

That sounds great, thanks!

Comment 9

[Timothy Nikkel (:tnikkel)]

I quickly was reminded of this code in nsEditor::IsEditable which is likely the source of a lot of problems:

    nsIFrame *resultFrame = content->GetPrimaryFrame();
    if (!resultFrame)   // if it has no frame, it is not editable
      return PR_FALSE;

Comment 10

[Timothy Nikkel (:tnikkel)]

The origin of that code is a 1999 commit whose commit message includes "IsEditable is much less hacky."

I think determining what this function is intended to do, and changing it so that it doesn't rely on frames existing is a good first step.

Comment 11

[(no longer active)]

Yeah, to the best of my knowledge, this function is supposed to make sure that the editor doesn't modify display:none elements.  I wrote a patch today which does that without touching the frame tree or flushing.  It is hitting a few failures in the editor/ tests.  I debugged a lot of them, but there are still 10 or so left for me to debug.

I'll file a bug and attach that patch there once I figure out those failures.  That should be a great step here, since that function is called from all around the place.

Comment 12

[Timothy Nikkel (:tnikkel)]

My uneducated wild guess of just checking if the content node IsEditable() if it has no frame and returning true if it is fixes all but 12 of the failures:
https://tbpl.mozilla.org/?tree=Try&rev=affaf8a41fe9

Just a data point, as I'm sure Ehsan's patch is much better.

Comment 13

[(no longer active)]

(In reply to Timothy Nikkel (:tn) from comment #12)
> My uneducated wild guess of just checking if the content node IsEditable()
> if it has no frame and returning true if it is fixes all but 12 of the
> failures:
> https://tbpl.mozilla.org/?tree=Try&rev=affaf8a41fe9
> 
> Just a data point, as I'm sure Ehsan's patch is much better.

OK, this should be fixed by my patch in bug 688789, which I will land soon.  Timothy, can you please use that patch and see what other failures we're running into?

Comment 14

[Mozilla RelEng Bot]

Try run for aac2cb281498 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=aac2cb281498
Results (out of 24 total builds):
    success: 20
    warnings: 2
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-aac2cb281498

Comment 15

[Timothy Nikkel (:tnikkel)]

Only one failure left, pasting it here in case the try logs go away.

13957 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | non-tabbable HTML editor: Shift+Tab after Tab on TD wasn't prevented on bubbling phase - got false, expected true
14000 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | non-tabbable HTML editor: Shift+Tab after Tab on TH wasn't prevented on bubbling phase - got false, expected true

Comment 16

[Timothy Nikkel (:tnikkel)]

With the extra flush I propose in bug 699703 to fix the last failure this goes green on try server.

Comment 17

[(no longer active)]

(In reply to Timothy Nikkel (:tn) from comment #16)
> With the extra flush I propose in bug 699703 to fix the last failure this
> goes green on try server.

Sorry, I had missed the bugmail here.  Would you mind testing again with that proposed flush?  If all is green, I think we should get this landed!

Comment 18

[Makoto Kato [:m_kato]]

dup to new bug