Slow reflow when selecting a large amount of text, on the logged-out view of GitHub's code viewer
Categories
(Core :: Layout: Text and Fonts, defect)
Tracking
()
Performance Impact | medium |
People
(Reporter: gregtatum, Unassigned)
Details
(Keywords: perf:responsiveness)
Profile: https://share.firefox.dev/3qLQguz
Platform: macOS
Steps to reproduce:
- Open https://github.com/unicode-org/cldr-json/blob/master/cldr-json/cldr-dates-full/main/en/timeZoneNames.json
- cmd + a to select all text
- Resize the browser (in my case, I opened devtools)
Expected behavior:
- The browser should behave react quickly to the changes
Actual behavior:
- The browser becomes very sluggish taking ~7 seconds to respond to a resize event.
Reporter | ||
Updated•4 years ago
|
Updated•4 years ago
|
Comment 1•4 years ago
|
||
We do have several issues around selection. WebRender made some of them worse, but this one seems to be about layout.
Comment 2•4 years ago
|
||
From the inverted call stack, I see we spend a lot time in nsINode::ComputeIndexOf()
. Set S2 because of very long reflow time per layout triage guideline.
Comment 3•3 years ago
|
||
I think bug 1679645 and co should've improved things.
Comment 4•3 years ago
|
||
Nope, this is still slow. Profile with Nightly 95.0a1 (2021-10-09): https://share.firefox.dev/3oQTIG1
Updated•3 years ago
|
Comment 5•2 years ago
|
||
Still slow. Profile with 104.0a1 (2022-06-29): https://share.firefox.dev/3y390ub
Comment 6•2 years ago
|
||
This might have gotten better? The STR don't seem anywhere near as bad now. Slightly jankiness but not like >1s hangs like in my comment 5 profile.
Comment 7•2 years ago
|
||
https://share.firefox.dev/3W9cNkD is my profile fwiw. Doesn't show selection code during reflow.
Comment 8•2 years ago
|
||
Ah, I do actually still see the same substanial jank.
When I tested and saw not-bad-results in comment 6, I was logged in to GitHub -- and this bug seems not-as-bad when you're logged in (because the site is presumably completely different). But if I'm not logged in (fresh profile, or private browsing window), then I still see badness like in the profile in comment 5.
Here's a profile I captured in today's Nightly 2022-12-19, with 1.3-second-long reflows, just like in comment 5:
https://share.firefox.dev/3FJaON3
(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
my profile [...] Doesn't show selection code during reflow.
My profile (linked in this comment) shows most of our reflow time spent in this backtrace, inside of ReflowText which calls into selection code via UnionAdditionalOverflow which calls CombineSelectionUnderlineRect:
mozilla::dom::Selection::LookUpSelection(nsIContent*, unsigned int, unsigned int, mozilla::UniquePtr<SelectionDetails, mozilla::DefaultDelete<SelectionDetails> >, mozilla::SelectionType, bool)
nsFrameSelection::LookUpSelection(nsIContent*, int, int, bool) const
nsTextFrame::GetSelectionDetails()
nsTextFrame::CombineSelectionUnderlineRect(nsPresContext*, nsRect&)
nsTextFrame::UnionAdditionalOverflow(nsPresContext*, nsIFrame*, nsTextFrame::PropertyProvider&, nsRect*, bool, bool)
nsTextFrame::ReflowText(nsLineLayout&, int, mozilla::gfx::DrawTarget*, mozilla::ReflowOutput&, nsReflowStatus&)
Comment 9•2 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #8)
When I tested and saw not-bad-results in comment 6, I was logged in to GitHub -- and this bug seems not-as-bad when you're logged in (because the site is presumably completely different).
One example of the substantial difference here: the code viewer uses a table
in the slow/janky logged-out view, vs. a flexbox-based layout in the just-fine logged-in view. Here's the markup for the code viewer, just showing up through the first line of GitHub's "Code viewer" (with the line-number/code 1 {
), in both designs:
Logged out table-table-based design (janky):
<table data-hpc="" class="highlight tab-size js-file-line-container js-code-nav-container js-tagsearch-file" data-tab-size="8" data-paste-markdown-skip="" data-tagsearch-lang="JSON" data-tagsearch-path="cldr-json/cldr-dates-full/main/en/timeZoneNames.json">
<tbody>
<tr>
<td id="L1" class="blob-num js-line-number js-code-nav-line-number js-blob-rnum" data-line-number="1"></td>
<td id="LC1" class="blob-code blob-code-inner js-file-line">{</td>
</tr>
Logged in flex-based design (not janky):
<div class="Box-sc-1gh2r6s-0 bjcubc react-code-file-contents" data-tab-size="8" data-paste-markdown-skip="true" style="height: 47200px;" data-hpc="true">
<div class="react-line-numbers" style="height: 47200px;">
<div data-line-number="1" class="react-line-number react-code-text" style="padding-right: 16px;">1</div>
[...all the other line numbers here...]
</div>
<div class="react-code-lines" style="height: 47200px;">
<div data-key="0" class="react-code-text react-code-line-contents">
<div id="LC1" class="react-file-line html-div" [...]>{</div>
</div>
(Here, .react-line-numbers
and .react-code-lines
are each vertical flex container with all of the numbers/lines.)
So: the table-flavored layout here is the problematic one. (I haven't analyzed to the point of knowing why or if the badness is related to tables at all, or if it's just the codepath that GitHub uses with that particular design vs. the other one.)
Updated•2 years ago
|
Comment 10•2 years ago
|
||
It is related to tables. Selecting a table creates a one selection range per cell (instead of just one for the whole document or so, in the case of flexbox)
Comment 11•2 years ago
|
||
Interestingly, I only see janky resizes when my window is sufficiently small (not super small, maybe 1000px or so)
Comment 12•2 years ago
|
||
Downgrading to reflect reality, since it's a bit special-casey, and we're not aware of this coming up super often. And to the extent that we understand the issue, it's not trivial to fix (based on how our selection code works for tables; see comment 10).
Comment 13•2 years ago
|
||
Thanks to jfkthame for the pointer to this bug.
FWIW, the current selection behaviour in tables as described in comment 10 (also when selecting contiguous cells rather than the whole document) is causing perf problems for a11y as well. There are improvements I can make in a11y which will hopefully help address this, but I thought I'd flag this here in case anyone working on this is considering changing the table cell selection behaviour as a possible solution.
Description
•