1256706 - On Twitter, don't recreate the accessible for the tweet that just lost focus

Status: RESOLVED FIXED

(Web Compatibility :: Site Reports, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. With NVDA and Firefox under Windows, go to https://twitter.com and log in.
2. Press NVDA-key+SpaceBar to turn browse mode *off*. This is needed for the following steps.
3. Use the j and k shortcut keys to navigate among tweets.
4. On any particular tweet, press NVDA-key+7 (on the alphanumeric keyboard). This will decouple the so-called NVDA object navigator from the system focus, in effect locking the navigator in onto the currently focused element.
5. Press j or k once more to shift focus to the below or above tweet.
6. Now, press NVDA-key+NumberPad 5 (when in desktop layout) or NVDA+Shift+O (when in laptop layout) to read the information about the current NVDA object navigator object.

Expected: NVDA should read information about the previously focused tweet (before step 5).

Actual: An indicator is given that the accessible the navigator pointed to is now defunct. The accessible is no longer there.

In its place, a new accessible has been created in the tree that represents the same tweet. So when focus was shifted to the other tweet in step 5, the list item did not only lose focus, which would cause a state change to occur, but something in the CSS or other processing that is also going on causes us to think we need to recreate the accessible. Normal focus changes do not instigate a need to recreate the accessible that just lost focus.

As a side effect, this destruction and recreation of an accessible causes NVDA to rerender the whole list of tweets. The bigger this and its sub lists get, the more delay there is. That's why, Jamie and I think, reading tweets is so slow.

Performing the above steps in Chrome for Windows yields the result that the accessible stays firmly put where it is, and does not get recreated. NVDA does not need to recreate the whole sub tree, and thus reading tweets in Chrome is so much faster.

I've asked Todd from the accessibility team at Twitter for information on what they're doing, but wanted to put this here as one of the symptoms that cause us to recreate accessibles when we don't need to.

dholbert, if you have any ideas how we can distinguish layout changes that should instigate a recreation of an accessible from those instances that don't, those ideas are very much welcome. We're losing ground on perceived reliability and quality with these changes going on more often nowadays and web apps feeling sluggish because we might be doing too much processing, and Chrome taking the lead in these areas.

Comment 1

[Daniel Holbert [:dholbert]]

(caveat: I don't know much about the layout <--> accessibility interactions.)

My first guess here would be that we're doing frame reconstruction -- that seems like the most likely thing to cause teardown & recreation of an associated "accessible" object.

There are a number of CSS changes that cause this:
 - All the cases where we add nsChangeHint_ReconstructFrame in nsStyleStruct.cpp CalcDifference methods (e.g. "display" property changing)
 - Various cases in nsCSSFrameConstructor::MaybeRecreateContainerFor[Whatever] (e.g. removing the only frame that separates two anonymous flex items).

If this is indeed what's happening (frame reconstruction), we should figure out what Twitter's doing to cause this, and then see if we can get them to avoid doing that and/or optimize their use-case to not require frame reconstruction.

Comment 2

[Daniel Holbert [:dholbert]]

[Hmm, just noticed that you marked this as depending on bug 686400.  Seems like you agree that reframing is part of the problem here.]

I don't know enough about accessible objects & their ownership model (& any risks associated with them outliving their frame) to be able to add further information here at this point.

It sounds like bug 686400 is the high-level bug here, anyway (though Twitter is an important use-case).

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Daniel Holbert [:dholbert] from comment #2)

> I don't know enough about accessible objects & their ownership model (& any
> risks associated with them outliving their frame) to be able to add further
> information here at this point.

frame defines an accessible type, see nsIFrame::AccessibleType(), so technically we don't have to recreate an accessible object if frame is reconstructed but a frame type is not changed.

Comment 4

[Marco Zehe (:MarcoZ)]

This is the information I got from my contact at Twitter:

--- snip ---

When the user presses j or k we do the following in the following order:
1. Check if there was a previously focused Tweet, if so:
• Remove the tabIndex from the <li> of the previously focused Tweet
• Remove the classes from the <li> of the previously focused Tweet
2. Add a class to the <li> to be focused
3. Apply the aria-labelledby attribute to the <li> to be focused
4. Apply a tabIndex of -1 to the <li> to be focused
5. Focus the <li>
6. Remove the aria-labelledby attribute from the previously focused <li> 
As for styles, here's the classes and styles applied/removed as the focus changes:
.selected-stream-item:focus {
  box-shadow: 0 0 0 3px rgba(104, 43, 18, 0.5);
}
.selected-stream-item:focus {
  outline: 0;
  z-index: 3;
  position: relative;
  border: 1px solid #fff;
  margin: -1px 0;
  box-shadow: inset 0 1px 3px rgba(0,0,0,.05),0 0 8px rgba(82,168,236,.6);
}
[tabindex="-1"]:focus {
  outline: none!important;
}

--- snip ---

dholbert, ring any bells?

Comment 5

[Daniel Holbert [:dholbert]]

Yup -- the "position: relative" change there will trigger frame reconstruction. (None of the other style changes seem to -- i.e. no frame-reconstruction happens if I apply all of the other styles besides "position:relative" to an element at once.)

I suspect this frame-reconstruction is necessary because it makes a huge difference for any abspos descendants (if there are any) -- those descendants will now be positioned with respect to this element, as opposed to its ancestor. This involves shuffling frames around between frame lists, so (unless this sort of change were frequent) it's easiest just to throw up our hands and do frame reconstruction.

If it's possible for twitter to just remove the "position:relative" declaration from this style rule, that'd be the easiest fix here.  Alternately, if they really need it & could just apply it up-front (so that they're not having to dynamically add it & trigger frame-reconstruction when focus changes), that'd work too.  [I'm not sure offhand if there's a cost associated with promoting a bunch of elements to be position:relative... I don't expect there'd be much of one, though]

Comment 6

[Daniel Holbert [:dholbert]]

(Alternately, we could probably add an optimization (a style hint) to let us skip frame reconstruction when something switches between "position:static" and "position:relative", *if* it doesn't have any descendants that would be abspos with respect to it.  [There might be some other conditions to consider, too, that I'm not thinking of.]  That would be a little complex to get right, but it seems like a possibility.)

Comment 7

[alexander :surkov (:asurkov)]

Also we could try to make the accessible tree updates smarter, like if a frame type of a DOM node wasn't changed during frames reconstruction then no accessible update should be required. This check shouldn't be very expensive.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> If it's possible for twitter to just remove the "position:relative"
> declaration from this style rule, that'd be the easiest fix here.

(Tested this locally -- this slightly breaks things -- the bottom of the box-shadow hidden behind the next tweet, because "z-index:3" from comment 4 will no longer have any effect.)

> Alternately, if they really need it & could just apply it up-front (so that
> they're not having to dynamically add it & trigger frame-reconstruction when
> focus changes), that'd work too.

(Tested this locally too -- this seems to work, but it feels a bit hacky / like overkill.)

Comment 9

[Daniel Holbert [:dholbert]]

I filed bug 1261484 on the optimization that I suggested in comment 6, BTW.

Comment 10

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #7)
> Also we could try to make the accessible tree updates smarter, like if a
> frame type of a DOM node wasn't changed during frames reconstruction then no
> accessible update should be required. This check shouldn't be very expensive.
Where would that live? Looking at this again since apparently bug 1261484 might take a while until someone can get to it.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #10)
> (In reply to alexander :surkov from comment #7)
> > Also we could try to make the accessible tree updates smarter, like if a
> > frame type of a DOM node wasn't changed during frames reconstruction then no
> > accessible update should be required. This check shouldn't be very expensive.
> Where would that live? Looking at this again since apparently bug 1261484
> might take a while until someone can get to it.

part of the accessibility engine, I do have a prototype, but was never about to finish it (make mochitest passing) and measure it. Hopefully I'll get back to it one day.

Comment 12

[Todd Kloots]

Marco, Alexander, Daniel — In an effort to help out NVDA users I've got a branch in the works that removes the dynamic application of position:relative. However, in my branch I'm still changing the z-index of the focused Tweet. Will the z-index change alone trigger frame reconstruction?

Comment 13

[Marco Zehe (:MarcoZ)]

(In reply to Todd Kloots from comment #12)
> Marco, Alexander, Daniel — In an effort to help out NVDA users I've got a
> branch in the works that removes the dynamic application of
> position:relative. However, in my branch I'm still changing the z-index of
> the focused Tweet. Will the z-index change alone trigger frame
> reconstruction?
I don't know, so forwarding to dholbert. :-) The best indicator is if you still hear a lag when changing focus with j and k. If it is significantly faster, and doesn't degrade the more tweets you're dealing with, then no. Otherwise, possibly yes.

dholbert?

Comment 14

[Marco Zehe (:MarcoZ)]

(In reply to Todd Kloots from comment #12)
> Marco, Alexander, Daniel — In an effort to help out NVDA users I've got a
> branch in the works that removes the dynamic application of
> position:relative. However, in my branch I'm still changing the z-index of
> the focused Tweet. Will the z-index change alone trigger frame
> reconstruction?
Does the analysis in comment #5 help or maybe even answer the question?

Comment 15

[Todd Kloots]

Yep, the analysis in comment #5 helps. Thanks.

Comment 16

[Todd Kloots]

Marco, wanted to let you know the fix for this bug is now live on twitter.com.

Comment 17

[Daniel Holbert [:dholbert]]

Sorry for the delayed response -- I'm recovering from bugzilla/email backlog after returning from vacation.

(In reply to Todd Kloots from comment #12)
> Marco, Alexander, Daniel — [...]
> in my branch I'm still changing the z-index of
> the focused Tweet. Will the z-index change alone trigger frame
> reconstruction?

Nope, that should only trigger a repaint (nsChangeHint_RepaintFrame).  For reference, we decide that here:
 http://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#1491

In contrast, changes to "position" do trigger a frame reconstruct (nsChangeHint_ReconstructFrame), which we decide here:
 http://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#2958

(In reply to Todd Kloots from comment #16)
> Marco, wanted to let you know the fix for this bug is now live on
> twitter.com.

That's great news! Thanks, Todd!

Comment 18

[Daniel Holbert [:dholbert]]

Marco, can you verify that this is fixed on your end?

Comment 19

[Marco Zehe (:MarcoZ)]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> Marco, can you verify that this is fixed on your end?
I so can verify this! Just spent 45 minutes reading Twitter in the web client, and don't think I've ever managed to last 10 before. This is really really a big big difference, and there is also no slowdown the longer the list of tweets grows.

I think this result gives us more reason to see if we can fix bug 1261484, since this might also improve the web site performance for many other web sites which we cannot all advocate for changing their CSS. :-)

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Marco Zehe (:MarcoZ) from comment #19)
> > Marco, can you verify that this is fixed on your end?
> I so can verify this!

Great - perhaps we should convert this to Tech Evangelism and resolve it as FIXED then?  (As-filed, it's no longer really a valid bug.)

Alternately/also, perhaps someone should spin off a helper bug with a testcase representing what Twitter used to be doing here, so we can change this behavior on our end ("don't recreate the accessible" after a reframe), if we like.

> I think this result gives us more reason to see if we can fix bug 1261484,
> since this might also improve the web site performance for many other web
> sites which we cannot all advocate for changing their CSS. :-)

I don't think we can say that, without more data / examples.  *If we knew* that a lot of sites dynamically restyled elements back and forth between "position:static" and "position:relative", I'd agree.  But right now (in the absence of that data), I'm not convinced that's a common thing at all, so I'm not sure that optimizing that case would actually be super-useful right now.  (It's a nontrivial amount of implementation work, with a pile of edge-cases to consider/handle carefully/test, and possibly some unforseen edge cases that we could mess up. So, nonzero risk/downside, with unclear upside in the absence of sites that we know would benefit.)

Comment 21

[Marco Zehe (:MarcoZ)]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> (In reply to Marco Zehe (:MarcoZ) from comment #19)
> > > Marco, can you verify that this is fixed on your end?
> > I so can verify this!
> 
> Great - perhaps we should convert this to Tech Evangelism and resolve it as
> FIXED then?  (As-filed, it's no longer really a valid bug.)
Done. Also, I trust your assessment of the layout portion of this, so will leave bug 1261484 in your capable hands. :-)

I will also file a spin-off bug to not create accessibles for reframing that doesn't change the accessible type, if we don't even already have that filed.

Comment 22

[Marco Zehe (:MarcoZ)]

Filed bug 1285862 for the change on the accessible side.