Closed
Bug 1395729
Opened 7 years ago
Closed 7 years ago
text-layers: dragging some text trips assertion
Categories
(Core :: Graphics: WebRender, enhancement)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: Gankra, Assigned: Gankra)
References
Details
Attachments
(2 files)
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.
Assignee | ||
Comment 1•7 years ago
|
||
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.
Updated•7 years ago
|
Blocks: stage-wr-nightly
Assignee | ||
Comment 2•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → a.beingessner
Assignee | ||
Comment 8•7 years ago
|
||
Review comments addressed!
Assignee | ||
Updated•7 years ago
|
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
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/74131a35a149
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 11•7 years ago
|
||
Part 5 in bug 1359584 was also going to get rid of nsDisplayText merging.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(matt.woodrow)
You need to log in
before you can comment on or make changes to this bug.
Description
•