Closed Bug 1395729 Opened 7 years ago Closed 7 years ago

text-layers: dragging some text trips assertion

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: Gankra, Assigned: Gankra)

References

Details

Attachments

(2 files)

Attached file text.html
While using layers-free webrendest:

If you select the whole line of attached text, and then try to drag the selection it crashes with:

Assertion failure: mMergedFrames.IsEmpty(), at /Users/ABeingessner/dev/gecko/layout/generic/nsTextFrame.cpp:5211

Seems we're still a bit inconsistent in our checking here.
Related: https://bugzilla.mozilla.org/show_bug.cgi?id=1392171 and https://bugzilla.mozilla.org/show_bug.cgi?id=1391996

cc :ethlin, in case you know why dragged selections might be special here.
So I discussed this with :mstange a bit and this is a bit of a mess. 

TL;DR: I'm going to just disable text-frame merging, because it doesn't work and might not be worth the trouble to support.

Basically, if you drag text, the way that's implemented is the relevant text-frames are drawn to a different DrawTarget with the pre-advanced-layers nsDisplayText::Paint path. This path doesn't support frame merging, and so it throws an assertion.

I don't believe the frame can strictly tell if it's going to be used in this manner, and so I don't think it can actually block merging for this case reliably. Maybe we could check if there's a selection...? I don't know if that's sufficient.

There's also just the matter that it's unclear how valuable text-frame merging is with webrender. We don't create a bunch of layers for each piece of text, just some extra DisplayItems. Computing a merge is potentially more work than it even saves.
Hey matt, just checking in that my analysis above seems reasonable to you, since you seemed to write this code originally.
Flags: needinfo?(matt.woodrow)
Comment on attachment 8905150 [details]
Bug 1395729 - Disable frame merging for nsTextFrame.

https://reviewboard.mozilla.org/r/176946/#review182014

Can you just remove the frame merging code entirely?

We're not using it for anything (nor do we plan to), so we might as well be rid of it.

r=me with TryMerge and mMergedFrames gone entirely.
Attachment #8905150 - Flags: review?(matt.woodrow) → review+
Assignee: nobody → a.beingessner
Review comments addressed!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/74131a35a149
Disable frame merging for nsTextFrame. r=mattwoodrow
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/74131a35a149
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Part 5 in bug 1359584 was also going to get rid of nsDisplayText merging.
Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.