Closed Bug 1611729 Opened 5 years ago Closed 5 years ago

Blame popup is slow on large files.

Categories

(Webtools :: Searchfox, defect)

Product:
Webtools
Component:
Searchfox
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: emilio, Assigned: emilio)

References

Details

On my laptop hovering on the blame on large files takes multiple seconds in multiple browsers.

This is a profile of hovering over the blame sidebar on nsGlobalWindowInner.cpp:

https://perfht.ml/36wCu3Q

It seems we force various big reflows to show the popup which end up doing a lot of work.

Unfortunately I don't know where the time is spent further than that because I don't know where the native stacks went... But seems that we could make the popup abspos or something on the root to avoid all this work.

A profile from Nightly 72, which shows native stacks: https://perfht.ml/36oGmny

A profile from my current nightly, as it seems I'm stupid enough to have the native stacks unchecked in the profiler toolbar icon... https://perfht.ml/37uygv1

The blame popup is "position: absolute", and for accessibility reasons, we want to keep it parented by the blame strip, I think. Or at least we want to maintain the existing experience for NVDA screen-reader users. See bug 1511275 for some context and the current intent behind what's going on. Maybe the display: inline isn't helpful at https://github.com/mozsearch/mozsearch/blob/master/static/css/mozsearch.css#L628 ?

/* ## Blame ## */
/* the role=cell that holds the role=button `.blame-strip`.  We created a class
   for this so we could make it `position: relative` so it could create a new
   containing block for the `.blame-popup` which cannot live beneath the
   role=button `.blame-strip`. */
.blame-container {
  position: relative;
}
.blame-strip {
    display: block;
    width: 20px;
    height: 16px;
    padding: 0;
    margin: 0;
}
/*...*/
.blame-popup {
    display: inline !important;
    /* positioned inside the .blame-container's containing block created by it
       being position: relative */
    position: absolute;
    top: 0;
    left: 20px;
    width: 600px;
    padding: 10px;
    background: #404040;
    box-shadow: 3px 3px 3px grey;
    border-radius: 3px;
    color: white;
    text-align: left;
    z-index: 200;
    cursor: auto;
    -moz-user-select: text;
    -o-user-select: text;
    -khtml-user-select: text;
    -webkit-user-select: text;
    -ms-user-select: text;
    user-select: text;
}
See Also: → 1511275

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #3)

The blame popup is "position: absolute", and for accessibility reasons, we want to keep it parented by the blame strip, I think. Or at least we want to maintain the existing experience for NVDA screen-reader users.

I don't quite follow everything here, but:

  1. Both the blame strip (role="button") and the blame popup need to be a child of a role="cell" (currently .blame-container) for a11y.
  2. Child doesn't necessarily mean DOM child. You could use aria-owns to make something a child for a11y. However, that would require that the target has an id.

display: inline is useless because absolutely-positioned stuff gets blockified.

https://github.com/mozsearch/mozsearch/pull/303 does the aria-owns shenanigans :)

Jamie, is it fine if the role=button element owns the popup? Or do I need to move the aria-owns to the parent role=cell?

Assignee: nobody → emilio
Flags: needinfo?(jteh)

A button shouldn't have any children, so please move aria-owns to the parent.

Flags: needinfo?(jteh)

I tried using NVDA against the dev run Emilio stood up at https://dev.searchfox.org/ specifically WorkerPrivate.cpp. While I could get the blame popup to show/hide as expected, I couldn't figure out how to get NVDA to read the contents of the blame popup. Specifically, I think I experienced what :Jamie was saying in the paragraph of https://bugzilla.mozilla.org/show_bug.cgi?id=1511275#c10 that I excerpt immediately below here, where pushing the down arrow would not cause the blame text to be read.

There's a bigger problem here, though. Even though the pop-up won't get read automatically, the user should still be able to cursor down to it once it appears. That no longer works because the pop-up is contained inside the .blame-strip "button". In the production instance, .blame-strip and .blame-popup are siblings, rather than .blame-popup being a descendant. .blame-popup can't be a descendant of .blame-strip for this to b accessible.

:Jamie, is there an alternate NVDA flow that I should be trying / does it work for you?

Note that my choice of WorkerPrivate.cpp may have been a bad one, I experienced issues where Firefox release seemed to lock up and become unresponsive for 5-10 or more seconds on the dev site, but that may have also involved my poor NVDA skills and the size of the DOM for a 5000-line source file.

Flags: needinfo?(jteh)

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #8)

I tried using NVDA against the dev run Emilio stood up at https://dev.searchfox.org/ specifically WorkerPrivate.cpp. While I could get the blame popup to show/hide as expected, I couldn't figure out how to get NVDA to read the contents of the blame popup. Specifically, I think I experienced what :Jamie was saying in the paragraph of https://bugzilla.mozilla.org/show_bug.cgi?id=1511275#c10 that I excerpt immediately below here, where pushing the down arrow would not cause the blame text to be read.

Yes, this is broken. I'd say Emilio hasn't gotten around to implementing my answer in comment 7 yet.

Note that my choice of WorkerPrivate.cpp may have been a bad one, I experienced issues where Firefox release seemed to lock up and become unresponsive for 5-10 or more seconds on the dev site,

Yes, Searchfox + Firefox + NVDA performance is absolutely awful on any file more than a few hundred lines, pretty much unusable with thousands of lines. :( These are Firefox a11y problems which are unfortunately difficult to fix: bug 1591326, bug 1473310, bug 1627073, probably others.

Flags: needinfo?(jteh)

I had implemented comment 7, but it wasn't in the dev run indeed.

(In reply to James Teh [:Jamie] from comment #9)

Yes, Searchfox + Firefox + NVDA performance is absolutely awful on any file more than a few hundred lines, pretty much unusable with thousands of lines. :( These are Firefox a11y problems which are unfortunately difficult to fix: bug 1591326, bug 1473310, bug 1627073, probably others.

I hadn't realized that the ARIA tables were so much slower than the real thing (per bug 1591326). Would it be helpful to add a mode to searchfox that either a) dynamically remixes the ARIA tables into real tables at the cost of the position: sticky headers on the client, or b) dynamically does it on the server, but leveraging our caching so any hit is a one-off, or c) statically does it during the indexing process as a second file so there's never any remix hit at runtime? For option 'c', we probably want to pre-gzip/whatever the HTML files which currently need to be compressed on the fly, which should more than make up for any additional disk usage and be a win for everyone. Option 'c' could also make more radical DOM changes if there are ways to further improve screen reader performance.

I'm going to file a bug on pre-gzip encoding right now, as a separate thing.

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #11)

I hadn't realized that the ARIA tables were so much slower than the real thing (per bug 1591326). Would it be helpful to add a mode to searchfox that either

While I really appreciate your willingness to improve perf for a11y, I think we really just need to fix this issue in Firefox. It's on my near-term list of things I want to tackle, as it is going to impact more and more things going forward.

a) dynamically remixes the ARIA tables into real tables at the cost of the position: sticky headers on the client,

Note that if we did this, we'd want to make sure that the original table wasn't visible before it got remixed; i.e. it should be DOM hidden or display: none until the remix was complete. Otherwise, a screen reader would end up trying to render the first table, then rendering it again after the remix, defeating the point of the change.

https://github.com/mozsearch/mozsearch/pull/303 got merged and fixed this, the blame popup is super fast now. Thanks Emilio!

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

(In reply to James Teh [:Jamie] from comment #12)

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #11)

I hadn't realized that the ARIA tables were so much slower than the real thing (per bug 1591326). Would it be helpful to add a mode to searchfox that either

While I really appreciate your willingness to improve perf for a11y, I think we really just need to fix this issue in Firefox. It's on my near-term list of things I want to tackle, as it is going to impact more and more things going forward.

That's great, just want to make it clear that I'm happy to help if there are short-term fixes possible, especially if they help with dogfooding.

a) dynamically remixes the ARIA tables into real tables at the cost of the position: sticky headers on the client,

Note that if we did this, we'd want to make sure that the original table wasn't visible before it got remixed; i.e. it should be DOM hidden or display: none until the remix was complete. Otherwise, a screen reader would end up trying to render the first table, then rendering it again after the remix, defeating the point of the change.

Understood. By the time I'd finished the comment, I think I'd talked myself out of this "a" strategy on the basis that it would be introducing consistent latency for screen-reader users. I mentioned it primarily as one end of the spectrum of options available. (Option 'b' as posed could introduce latency, but can be mitigated by priming the caches with automated requests before marking the web-server operational. This could even be done on the indexer at a much higher rate before spinning up the web-server because we use the EBS volume for caching. And I think we might want to spin up the web serving processes on the indexer for self-testing in general.)

You need to log in before you can comment on or make changes to this bug.