Closed Bug 570275 Opened 15 years ago Closed 15 years ago

rework accessible tree update code

Categories

(Core :: Disability Access APIs, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: surkov, Assigned: surkov)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: access, perf)

Attachments

(1 file, 9 obsolete files)

Accessibility module is notified by nsIAccessibilityService::invalidateSubtreeFor. It's called by 1) nsCSSFrameConstructor::RecreateFramesForContent() - http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9181 2) nsFrameManager::ReResolveStyleContext - http://mxr.mozilla.org/mozilla-central/source/layout/base/nsFrameManager.cpp#1329 The problem we can't create an accessible at this point because accessible creation is frame based (i.e. frame creates an accessible) and it sounds there are kind of temporary frames since sometimes we create wrong accessible object. That's a reason we don't do this. However we fire delayed accessible events to process the change when we're ok to build accessible tree. invalidateSubtreeFor should be called when frames were constructed so we can avoid excess delayed events processing. Boris, any idea what can I do here?
I'm not sure I follow what the problem is.... Frames are _very_ ephemeral. They can be created and destroyed for all sorts of reasons. Is that what we're talking about here? Or something else?
I think we need to get notification about frame changes when display or visibility style is changed and when frame is created/destroyed for appended/removed DOM node (the last one because of lazy frame construction). Also we'd like to get notification for changed root element only, for example, if display style for the div element containing other elements is changed then we need a notification for div element when frames for its child elements where updated. That allows us to create accessible tree for changes subtree at once and do not coalesce a11y mutation events from subtree. Does nsFrameManager::ReResolveStyleContext (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsFrameManager.cpp#1373) deal with display and visibility style changes only? Can it be changed to notify a11y after child frames were updated? It looks nsCSSFrameConstructor::RecreateFramesForContent (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9175) is also related with display style changes handling, at least changes for a11y were added within nsFrameManager changes in bug 354745. Why is notification from nsFrameManager not enough?
> about frame changes when display or visibility style is changed OK. Those are _very_ different operations from a layout perspective.... > and when frame is created/destroyed for appended/removed DOM node So fundamentally, a display change is converted into a remove/insert pair internally. So as long as you're notified for those, all you need in addition is visibility changes, right? > for example, if display style for the div element containing other elements > is changed then we need a notification for div element when frames for its > child elements where updated. I'm not sure I follow. A display change on a node will reframe all its descendants.... It'll look just like that node being removed, then reinserted. Note that visibility changes do NOT work that way, though. ReResolveStyleContext deals with all style changes. Note that no updates happen _inside_ ReResolveStyleContext: it just makes a list of updates to process, and then the list is processed. RecreateFramesForContent is what's used for display (and various other properties that can cause a framechange) changes, yes. This is also called in cases where styles haven't changed at all but the DOM tree has. > Why is notification from nsFrameManager not enough? That notification only notifies on visibility changes, not on display changes.
(In reply to comment #3) > > about frame changes when display or visibility style is changed > > OK. Those are _very_ different operations from a layout perspective.... > > > and when frame is created/destroyed for appended/removed DOM node > > So fundamentally, a display change is converted into a remove/insert pair > internally. So as long as you're notified for those, all you need in addition > is visibility changes, right? Yes. > > for example, if display style for the div element containing other elements > > is changed then we need a notification for div element when frames for its > > child elements where updated. > > I'm not sure I follow. A display change on a node will reframe all its > descendants.... It'll look just like that node being removed, then reinserted. That works I think. If we don't get notification for children of node that display style was changed then that's what is needed I think. Is that going to be? > Note that visibility changes do NOT work that way, though. We can deal with visibility changes separately. I lean towards we shouldn't change accessible tree for visibility changes at all since node takes a place on the screen but not visible. We could expose these accessible with invisible accessible state. Boris, could you point me a code please where I should add notifications?
> If we don't get notification for children of node that display style was > changed then that's what is needed I think. Is that going to be? I think so, yes. > Boris, could you point me a code please where I should add notifications? For display changes and remove/insert? It really depends on what you want... Right now you have notifications for display changes already in nsCSSFrameConstructor::RecreateFramesForContent. If you're willing to give up on the FRAME_SIGNIFICANT_CHANGE thing and just treat a display change as hide + show, you could just add a notification for hide to the end of nsCSSFrameConstructor::ContentRemoved and add notifications for show in nsCSSFrameConstructor::ContentRangeInserted.
Is nsCSSFrameConstructor::ContentRemoved called after DOM node is removed from the tree or after node is hidden so that we can't get right text and offsets from nsIFrame::GetRenderedText() in the case of text node?
> Is nsCSSFrameConstructor::ContentRemoved called after DOM node is removed from > the tree If the node is being removed to start with, then yes. Note that the ContentRemoved call contains the information needed to determine where it used to be in the tree, in that case.
Alexander do you have a WIP for this bug?
No, I haven't
This bug is crucial for accessibility support since after lazy frame construction implementation we might report wrong information to AT (for example, when DOM node was appended to DOM and frame wasn't created then we don't fire accessible show event however we should).
blocking2.0: --- → ?
(In reply to comment #5) > you could just add a notification for hide to the end of > nsCSSFrameConstructor::ContentRemoved and add notifications for show in > nsCSSFrameConstructor::ContentRangeInserted. to the end of ContentRemoved and ContentRangeInserted? I shouldn't care about early returns, right?
It sounds switching to ContentRemoved and ContentRangeInserted() should make nsIEventStateManager->IsHandlingUserInputExternal() return false always what results "event-from-input" object attribute has "false" value. I wonder if this object attribute is used by AT when they handle show/hide events?
Boris, where should I add notification for a11y to listen visibility style changes? Btw, Marco, what do you think if we fire STATE_INVISIBLE change for visibility style changes (comment #4)?
> I shouldn't care about early returns, right? Without looking at the code just now, I think that's correct. > Boris, where should I add notification for a11y to listen visibility style > changes? That one is tougher. We don't do anything special for visibility other than treating it as just another property that needs a repaint, like color. And in particular, getting at "root" visibility changes... I don't know. roc, any good ideas?
Is STATE_INVISIBLE just about the CSS 'visibility' property or is it about other things that can make content invisible, like clipping, scrolling out of view, etc?
(In reply to comment #15) > Is STATE_INVISIBLE just about the CSS 'visibility' property or is it about > other things that can make content invisible, like clipping, scrolling out of > view, etc? State invisible is used when there's element is accessible but it's not visible. I think this state can be used for clipping if accessible element is not visible. The same time scrolling out view accessible element uses different accessible state. In general we don't create an accessible for not visible element (like display: none) but if there's an accessible for element that not visible then we expose invisible accessible state. This makes me think state invisible should be suitable for visibility property so that we don't remove accessible from the tree but AT can ignore it and look for its children since they can be visible.
Alexander, I'm not confident we would block a release on this bug. I think it is definitely a strong want (especially since our event queue should shrink nicely). I'll deny blocking for now but let's chat if you disagree. We can always consider landing approval once we have a tested fix.
blocking2.0: ? → -
(In reply to comment #17) > Alexander, I'm not confident we would block a release on this bug. I think it > is definitely a strong want (especially since our event queue should shrink > nicely). I'll deny blocking for now but let's chat if you disagree. We can > always consider landing approval once we have a tested fix. The problem is we don't send show events under certain circumstances. That means virtual buffer is out of date. That's definitely huge problem. That's theoretical. I don't have time to look for testcase. But I'm sure it'll be found after firefox release if we don't fix it.
Boris, knowing whether the action was from user input or not (nsIEventStateManager::IsHandlingUserInputExternal()) when we handle mutations is important for a11y. Since ContentRangeInserted and ContentRemoved are processed async then IsHandlingUserInputExternal() returns false always. Is there a way to store the value when mutation happens and pass into these methods (perhaps by nsILayoutHistoryState)?
Boris, Robert, do you have any idea how to workaround the problem described in comment #19? This is blocking thing to continue the work on this bug.
rerequest blocking Firefox 4 due to reasons: 1) it's perf improvement because tree invalidation happens ansync 2) no missed events what leads updated virtual buffer of screen readers because show events can be missed when DOM node is inserted but frame isn't constructed yet 3) it makes code logic cleaner and more robust so that we have unique way to deal with DOM and layout changes 4) it's a base for further improvements so that it allows to make text insert events coalescence and simplify reorder events construction what is perf win.
blocking2.0: - → ?
As side note it might be a fix for bug 580464 which has blocking status already. Any way it's worth to fix a mess with accessible tree invalidation code before we get the right way to fix bug 580464 since that bug is caused by accessible tree invalidation problems.
Approving final+. Based on comment 21 and comment 22 (which I tend to agree with) I think this is our top priority right now.
Assignee: nobody → surkov.alexander
blocking2.0: ? → final+
Priority: -- → P1
> Boris, knowing whether the action was from user input or not > (nsIEventStateManager::IsHandlingUserInputExternal()) when we handle mutations But not style changes (which were already async)? > Is there a way to store the value when mutation happens Not an easy one; for one thing we coalesce user-triggered and non-user-triggered mutations, right? It might be possible to come up with something, obviously, but I'm not sure yet what a good way to do it would be or how much work it would take. Further, the existing setup was broken if the script did the mutation from a user-triggered timeout, no? What is the "user-trigered" information used for?
(In reply to comment #24) > > Boris, knowing whether the action was from user input or not > > (nsIEventStateManager::IsHandlingUserInputExternal()) when we handle mutations > > But not style changes (which were already async)? Yes. That's not right but that's the thing we deal with. There's no point to change this. > > > Is there a way to store the value when mutation happens > > Not an easy one; for one thing we coalesce user-triggered and > non-user-triggered mutations, right? It might be possible to come up with > something, obviously, but I'm not sure yet what a good way to do it would be or > how much work it would take. Ok, I see, this doesn't sound trivial and perhaps it's hard to find right way. > Further, the existing setup was broken if the script did the mutation from a > user-triggered timeout, no? User-triggered timeout is not considered as user-triggered action. At least it sounds everybody is happy with this. > What is the "user-trigered" information used for? Primary when page is loading we need to decide whether we should fire events to AT or not. If mutation is because of page loading then we shouldn't bother an AT and all we need is to update accessible tree. But if the user types into textbox while page is loading then we should fire events so that AT announce this change. I don't have complete information how AT use events for user-triggered and not user-triggered changes but this is the thing I would like to keep unchanged. Ok, that's nearly impossible so if we get something to workaround the problem above then it should be a good way to go for now.
> But if the user types into textbox while page is loading then we should fire > events so that AT announce this change. The mutations here happen sync, though, right?
(In reply to comment #26) > > But if the user types into textbox while page is loading then we should fire > > events so that AT announce this change. > > The mutations here happen sync, though, right? When user types first character into the field then text node is inserted and I expected to get notification from nsCSSFrameConstructor::ContentRangeInserted to fire events. nsCSSFrameConstructor::ContentRangeInserted is not sync or do I miss something?
Hmm. Does that create new text nodes? I thought with Ehsan's patches it just edited the single textnode inline, so the only event involved is CharacterDataChanged. I guess that stuff hasn't landed yet, but it should for 4.0.
Is this valid for content editable areas as well? No?
Hmm. For content editable, probably not. But now that you remind me... we don't do lazy frame construction for editable content. We do eager frame construction. That includes both text inputs and contenteditable. See nsCSSFrameConstructor::MaybeConstructLazily.
Does it mean nsCSSFrameConstructor::ContentRangeInserted isn't called in this case or is it called sync?
It depends. For inserts, it's called and does the work sync. For appends, it's not called in this case.
(In reply to comment #32) > It depends. For inserts, it's called and does the work sync. For appends, > it's not called in this case. So where should I notify a11y to not miss mutation changes and avoid dupes. Should I add code into the end of ContentRangeInserted/ContentAppended/ContentRemvoed?
(In reply to comment #28) > Hmm. Does that create new text nodes? I thought with Ehsan's patches it just > edited the single textnode inline, so the only event involved is > CharacterDataChanged. I guess that stuff hasn't landed yet, but it should for > 4.0. It hasn't landed yet (it's bug 240933) but it will hopefully pretty soon. That patch will *only* affect textarea's though.
> Should I add code into the end of ContentRangeInserted/ContentAppended/ContentRemvoed? That seems reasonable, though note that ContentRemoved doesn't always reach its end (nor does ContentAppended/ContentRangeInserted). In those cases they cause a rebuild of a large chunk of the frame tree containing the removal or insert location.
(In reply to comment #35) > > Should I add code into the end of ContentRangeInserted/ContentAppended/ContentRemvoed? > > In those cases they cause > a rebuild of a large chunk of the frame tree containing the removal or insert > location. Does it mean ContentRangeInserted/ContentRemoved will be called for this large chunk of frame tree in the end?
Yes (possibly async, and _that_ might be async even in the case of editor, iirc).
Though at that point you're getting into edge cases which were already async before the async frame construction stuff.
note to myself: it's needed to check whether the fix affects on bug 334386 and bug 413777.
Depends on: 591163
Depends on: 591199
Boris, I have couple questions. 1. div.style.display="none" results in two notifications: nsCSSFrameConstructor::ContentRemoved with REMOVE_FOR_RECONSTRUCTION flag and then presumably ContentRangeInserted (because it shouldn't be ContentAppneded). Why does this change causes remove_from_reconstruction flag in ContentRemoved and why is ContentRanageInserted is called? How can ignore notification from ContentRangeInserted in this case? 2. div.style.display="inline" and div.style.overflow="scroll" both result in ContentRemoved and ContentRnageInserted calls. It's ok to recreate accessible for the first case but we don't want to do this for the second case. So basically I want to filter ContentRemoved/RangeInserted notifications to achieve this. I think the check can be based on frame type since frame class is responsible for accessible creation. Is there a way to do this?
btw, this bug is a fix for bug 580464 because ContentRemoved/RangeInserted triggers before we fire focus event in testcase of bug 580464. So if we are able to update accessible tree synchronously then that bug is gone. Btw, Boris, div.style.display="block" on hidden div results in ContentRemoved call. How can we filter this case as well?
Alexander, right now layout treats all style changes requiring a frame reconstruct as a removal followed by an insert. > Why does this change causes remove_from_reconstruction flag in ContentRemoved So it can be distinguished from actual removals from the DOM. This is only used for the textframe-suppression code at the moment. > and why is ContentRanageInserted is called? Because we don't special-case "display: none" in the style change handling code. It's just a style change that needs a frame reconstruct. We treat it as a removal followed by insert; the insert just doesn't create any frames. You need to handle this last anyway, since it can happen easily for DOM inserts. > It's ok to recreate accessible for the first case but we don't want to do > this for the second case. OK, then maybe what you really want is to explicitly flag which style changes need to recreate accessibles and only do that for those style changes and actual DOM mutations? But I thought accessibles cached frames and thus had to be recreated whenever the frame is? > I think the check can be based on frame type The frame type changes when the "overflow" value changes from "visible" to "scroll". Or am I misunderstanding the suggestion? > Btw, Boris, div.style.display="block" on hidden div results in ContentRemoved > call. Right, which skips most of the work because there's no existing frame (childFrame variable in ContentRemoved), correct?
(In reply to comment #42) > Alexander, right now layout treats all style changes requiring a frame > reconstruct as a removal followed by an insert. Fair enough. > > It's ok to recreate accessible for the first case but we don't want to do > > this for the second case. > > OK, then maybe what you really want is to explicitly flag which style changes > need to recreate accessibles and only do that for those style changes and > actual DOM mutations? similar things we have now: 1) notifications in RecreateFramesForContent() and ReResolveStyleContext and 2) nsIMutationObserver notifications if you mean this. The problem of mutation observer is frame might be not constructed at this point. And there are problems in layout changes, I'm not sure whether this is valid now but frame for changed node or its descendants might be not constructed as well. If there's a way to workaround this then it's great, though I started to love already the unique interface to handle style and DOM changes :) > But I thought accessibles cached frames and thus had to be recreated whenever > the frame is? No, we don't cache them. But we don't create an accessible if there's no frame and it's considered that the accessible is dead if we can't obtain a frame (usually it's primary frame of content). > > I think the check can be based on frame type > > The frame type changes when the "overflow" value changes from "visible" to > "scroll". Or am I misunderstanding the suggestion? Ok, this doesn't work. I didn't know frame type is changed. The algorithm can be fixed: we could introduce accessible type for frame (for example, replace nsIFrame::GetAccessible() to nsIFrame::GetAccessibleType()). And if the accessible type is the same then we don't need to recreate an accessible. At least currently when page have style "a:focus { overflow: scroll; }" and user navigates through links then nothing special happens visually but we recreate accessibles which makes additional work for us and screen readers. > > Btw, Boris, div.style.display="block" on hidden div results in ContentRemoved > > call. > > Right, which skips most of the work because there's no existing frame > (childFrame variable in ContentRemoved), correct? It sounds I can use childFrame to detect whether a11y should be notified or not. It should be something similar for ContentRangeInserted, right? A side question: if frame is recreated then are ContentRemoved/RangeInserted() called synchronously each after other?
> The problem of mutation observer is frame might be not constructed at > this point. Yes, I understand that. > but frame for changed node or its descendants might be not constructed as well Not sure what you mean. There are definitely all sorts of cases when an element has no frame... You can certainly add arguments as needed to ContentRemoved/ContentRangeInserted/ContentAppended. Would that help your situation? > At least currently when page have style "a:focus { overflow: scroll; }" and > user navigates through links then nothing special happens visually That depends on whether links are styled with display:inline or not.... "overflow" simply doesn't apply to inlines. We could almost certainly change the style system to not reframe for overflow changes to inlines specifically, but there are likely other reframe changes which you don't want to treat as hide/show... > It sounds I can use childFrame to detect whether a11y should be notified or > not. You can use it in ContentRemoved to detect whether there was a frame before the call. For ContentRangeInserted, you can detect whether there is a frame after the call. > A side question: if frame is recreated then are ContentRemoved/RangeInserted() > called synchronously each after other? Yes, but there is no guarantee that RangeInserted won't do its work async in this case. I believe that it doesn't now, and won't in gecko 2.0, but I'd like to make this async if I can, in fact.
Blocks: 580535
(In reply to comment #44) > > but frame for changed node or its descendants might be not constructed as well > > Not sure what you mean. There are definitely all sorts of cases when an > element has no frame... Do you mean an element has no frame in general or its pended for construction? Currently we update the accessible tree asynchronously when style changes occur because (at least previously) the frame or its descedeant frames weren't created yet. If descendants frames might be not constructed when ContentRangeInserted triggers for the frame but they will be and ContentRangeInserted will be called later for them then it still works for us. If it won't be called then it's a problem. > > It sounds I can use childFrame to detect whether a11y should be notified or > > not. > > You can use it in ContentRemoved to detect whether there was a frame before the > call. > > For ContentRangeInserted, you can detect whether there is a frame after the > call. That's great. At least it works for the cases when we must recreate an accessible. So at this point I need to get an idea how to avoid accessible recreation for style changes that we don't want to recreate an accessible for. > > > At least currently when page have style "a:focus { overflow: scroll; }" and > > user navigates through links then nothing special happens visually > > That depends on whether links are styled with display:inline or not.... > "overflow" simply doesn't apply to inlines. We could almost certainly change > the style system to not reframe for overflow changes to inlines specifically, links don't have any additional style so they should have display:inline. I tried the testcase and when I tabbed through links then I get ContentRemoved/ContentRangeInserted pair for blur and focused links. > but there are likely other reframe changes which you don't want to treat as > hide/show... Yes, the idea (as I said previously) would be checking accessible type of the frame (it should be added though) and if it's not changed then do not notify a11y. Though I don't get how to implement this. > You can certainly add arguments as needed to > ContentRemoved/ContentRangeInserted/ContentAppended. Would that help your > situation? I must be missing the idea. Is it about accessible type checks? I wonder if the following scenario is possible. Frame is recreated but it's accessible type is not changed, however the frame descendants are recreated as well but their accessible types are changed and we don't get ContentRangeInserted for them. Though it doesn't sound like that's possible. > > A side question: if frame is recreated then are ContentRemoved/RangeInserted() > > called synchronously each after other? > > Yes, but there is no guarantee that RangeInserted won't do its work async in > this case. I believe that it doesn't now, and won't in gecko 2.0, but I'd like > to make this async if I can, in fact. Ok, then I shouldn't rely on this. Btw, are ContentRangeInserted/ContentAppended are called for initial frame creation when page is loaded?
> Do you mean an element has no frame in general or its pended for construction? I meant the former. > If it won't be called then it's a problem. ContentRangeInserted or ContentAppended will be called on the element or some ancestor for any case when frames are actually constructed. > I tried the testcase and when I tabbed through links then I get > ContentRemoved/ContentRangeInserted pair for blur and focused links. Right. Like I said, we could avoid that for the specific case of overflow on inlines; might be worth filing a bug. But the general problem remains. > I must be missing the idea. Is it about accessible type checks? No... it's about being able to tell apart content insertions, lazy construction, and restyles. Assuming being able to tell them apart is useful, of course. > Frame is recreated but it's accessible type is not changed, however the frame > descendants are recreated as well but their accessible types are changed Definitely possible, if the accessible type depends on the kind of frame... does it? Do blocks and inlines get different accessible types, say? > Btw, are ContentRangeInserted/ContentAppended are called for initial frame > creation when page is loaded? Yes, from PresShell::InitialReflow (see the ContentInserted call it makes) and then from the parser sending mutation notifications as content streams in.
(In reply to comment #46) > > I must be missing the idea. Is it about accessible type checks? > > No... it's about being able to tell apart content insertions, lazy > construction, and restyles. Assuming being able to tell them apart is useful, > of course. Sorry, could you describe these terms so I can be sure I understand them right? content insertion is about DOM insertion or about sync DOM insertions only? lazy construction is about frame construction but not frame reconstruction? restyles is about display and other style changes or about other style changes? > > Frame is recreated but it's accessible type is not changed, however the frame > > descendants are recreated as well but their accessible types are changed > > Definitely possible, if the accessible type depends on the kind of frame... > does it? Do blocks and inlines get different accessible types, say? I think we create an accessible for blocks (for example, for HTML div). Usually we don't create for inlines until it's necessary (for example, we don't create an accessible for span in general, but we do create an accessible for HTML a if it's inline at all). Could you provide an example please?
> Sorry, could you describe these terms so I can be sure I understand them right? * Content insertion: someone modified the DOM * Lazy construction: someone modified the DOM sometime in the past and now we're processing the lazy frame construction bit for it. * Restyle: some style changed so we need to reconstruct frames, and we make that look like a remove+insert. > content insertion is about DOM insertion or about sync DOM insertions only? DOM insertion will always call ContentInserted or ContentAppended. Sometimes they will construct frames immediately; sometimes they'll set some bits and post an event to do it later. > restyles is about display and other style changes or about other style > changes? Any style changes that need a reframe. > Could you provide an example please? Sure, if your description above is correct. Start with this DOM: <div><span></span><div> then in a script set the overflow of the div to "scroll", then set the display of the span to "block", then flush layout. This will reframe both the div and the span by performing a single removed/inserted notification on the div.
Boris, what is the "chrome-flush-skin-caches" notification about and do we need to recreate accessible tree on this if we follow the ContentRangeInserted/Removed/Appended approach?
(In reply to comment #49) > Boris, what is the "chrome-flush-skin-caches" notification about and do we need > to recreate accessible tree on this if we follow the > ContentRangeInserted/Removed/Appended approach? http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7856
That notification happens during theme changes. You can just assume that it happens very rarely and never without the user knowing.
(In reply to comment #51) > That notification happens during theme changes. You can just assume that it > happens very rarely and never without the user knowing. Ok, can theme change result in new elements insertion/deletion or frame changes because of different styling? If yes then will be ContentRemoved/RangeInserted called for them?
Theme change can cause reframing due to different styling, of course. I think that should go through the normal restyle codepath.
Attached patch wip (obsolete) — — Splinter Review
doesn't pass test suite
Status: NEW → ASSIGNED
Blocks: 472810
Attached patch wip2 (obsolete) — — Splinter Review
Boris, when I notify a11y from ContentRemoved (notification in the end of this method) then frame is destroyed at this point and I can't get rendered text for text node and therefore can't fire text change accessible event. Any ideas? Is it possible to move notification higher where frame is not destroyed yet?
Attachment #478155 - Attachment is obsolete: true
Well, you could move it earlier in ContentRemoved....
(In reply to comment #56) > Well, you could move it earlier in ContentRemoved.... Great, do you keep in mind the right place for this?
Well, how about right here: // Examine the containing-block for the removed content and see if // :first-letter style applies. or so? That's after all the early returns, but before we remove anything....
changing summary to reflect better this bug is about
Summary: layout notifies a11y about layout changes too early → rework accessible tree update code
Boris, one more question. We're not able to create accessible for plugin when ContentRangeInserted notifies us because frame->GetRect()->IsEmpty() is true ( http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibilityService.cpp#347). This check was introduced in bug 312788, where they decided to not expose empty rect plugins. It sounds at this point size of plugin isn't calculated yet. Is there another way to know if plugin won't be visible? The stack is: xul.dll!nsDocAccessible::UpdateTree(nsIContent * aContainerNode, nsIContent * aStartNode, nsIContent * aEndNode, int aIsInsert) Line 1361 + 0x22 bytes C++ xul.dll!nsAccessibilityService::ContentRangeInserted(nsIPresShell * aPresShell, nsIContent * aContainer, nsIContent * aStartChild, nsIContent * aEndChild) Line 489 C++ xul.dll!nsCSSFrameConstructor::ContentAppended(nsIContent * aContainer, nsIContent * aFirstNewContent, int aAllowLazyConstruction) Line 6700 C++ xul.dll!nsCSSFrameConstructor::CreateNeededFrames(nsIContent * aContent) Line 6302 C++ xul.dll!nsCSSFrameConstructor::CreateNeededFrames(nsIContent * aContent) Line 6309 C++ xul.dll!nsCSSFrameConstructor::CreateNeededFrames() Line 6324 C++ xul.dll!PresShell::FlushPendingNotifications(mozFlushType aType) Line 4872 C++ xul.dll!nsDocument::FlushPendingNotifications(mozFlushType aType) Line 6536 C++ xul.dll!nsObjectLoadingContent::NotifyStateChanged(nsObjectLoadingContent::ObjectType aOldType, int aOldState, int aSync) Line 1667 C++ xul.dll!AutoNotifier::Notify() Line 354 C++ xul.dll!nsObjectLoadingContent::LoadObject(nsIURI * aURI, int aNotify, const nsCString & aTypeHint, int aForceLoad) Line 1305 C++ xul.dll!nsHTMLSharedObjectElement::StartObjectLoad(int aNotify) Line 453 C++ xul.dll!nsHTMLSharedObjectElement::StartObjectLoad() Line 135 + 0x11 bytes C++ xul.dll!nsRunnableMethodImpl<void (__thiscall nsHTMLSharedObjectElement::*)(void),1>::Run() Line 348 C++ xul.dll!nsContentUtils::RemoveScriptBlocker() Line 4754 C++ xul.dll!nsContentUtils::RemoveRemovableScriptBlocker() Line 1488 C++ xul.dll!nsDocument::EndUpdate(unsigned int aUpdateType) Line 4075 C++ xul.dll!nsHTMLDocument::EndUpdate(unsigned int aUpdateType) Line 2952 C++ xul.dll!nsHtml5TreeOpExecutor::EndDocUpdate() Line 257 C++ xul.dll!nsHtml5TreeOpExecutor::DidBuildModel(int aTerminated) Line 128 C++ xul.dll!nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor * aBuilder, nsIContent * * aScriptElement) Line 700 C++ xul.dll!nsHtml5TreeOpExecutor::RunFlushLoop() Line 492 C++ xul.dll!nsHtml5ExecutorReflusher::Run() Line 91 C++
> It sounds at this point size of plugin isn't calculated yet Right. No layout has happened yet at this point; that was also the case before lazy frame construction. Sounds like you want to add some hooks to nsObjectFrame to notify you when the size changes....
the similar issue happens for <div style="width: 100px; height: 100px; overflow: auto;">hello</div>, this div has nsHTMLScrollFrame which is accessible if it's focusable but it's not focusable when we update the accessible tree because it looks like no layout has happened yet at this point and therefore margin is zero (http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#6435). I'm not sure how can we deal with this. For some cases a11y relies on assumption element's size is calculated to create an accessible.
FYI nsHTMLScrollFrame accessibility was introduced in bug 254966 and they used scrollbar styles (like NS_STYLE_OVERFLOW_SCROLL) instead of margin info to decide whether it's focusable. I keep in mind to approaches 1. force layout to calculate size (that's not good idea from perf point of view I think) 2. create accessible not depending on frame size, this way, create accessible for plugin always (remove frame->GetRect() check) and for HTML scroll frames (remove IsFocusable() check). Though I wonder what affect may HTML scroll frame, that is always accessible, have? What is it used for?
(In reply to comment #63) > this way, create accessible > for HTML scroll frames > (remove IsFocusable() check). Though I wonder what affect may HTML scroll > frame, that is always accessible, have? What is it used for? HTML scroll frame is used for HTML input and making it accessible breaks accessible attribute ranges. That's unwanted. Boris, is there a way to change nsFrame::IsFocusable() for HTML scroll frames to remove dependance from nsIScrollableFrame::GetActualScrollbarSizes() or making this method to return calculated margin when ContentRangeInserted() triggers a11y notfication?
(In reply to comment #63) > create accessible > for plugin always (remove frame->GetRect() check) nsIObjectFrame::GetPluginInstance returns null, so it doesn't help.
Attached patch wip3 (obsolete) — — Splinter Review
passes all mochitests but plugin and div@style="overflow" tests
Attachment #478229 - Attachment is obsolete: true
(In reply to comment #66) > Created attachment 479056 [details] [diff] [review] > wip3 > > passes all mochitests but plugin and div@style="overflow" tests even we have some mochitests failures but I think the patch is suitable for general testing, the build will be available here - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-08f4c6c50dcb
Observing a lot of crashiness with this try-server build: 1. When entering http://www.google.com in the awesome bar, and hitting ENTER. The build crashes: bp-53b530bb-2ca4-45fd-a53d-654112100929 2. When logging into http://neu.de (Surkov has the credentials), after the initial login page, the build crashes: bp-4b4e887e-8c66-4174-b38a-767232100929 3. When on Facebook, on the main page, and switching between Main News and Latest Updates, I get this crash: bp-13da1256-c15b-496b-8e30-2d37a2100929 All of this is with NVDA 2010.2Beta2 running.
Attached patch wip4 (obsolete) — — Splinter Review
* no crashes for me * plugin issue fixed * div@style="overflow: scroll" fails, I need a help here from Boris I think (answer on comment #64 or otherwise ideas when I can invalidate accessible for HTML scroll frame when margin is calculated)
Attachment #479056 - Attachment is obsolete: true
> For some cases a11y relies on assumption element's size is calculated to > create an accessible Then it needs to do that after layout.... Note that whether an overflow:auto element has scrollbars (and thus whether you want to create an accessible?) can change without any style changes on the element itself. > 1. force layout to calculate size (that's not good idea from perf point of > view I think) Also can't be done inside the frame construction code. > Boris, is there a way to change nsFrame::IsFocusable() for HTML scroll frames > to remove dependance from nsIScrollableFrame::GetActualScrollbarSizes() That's a question for Neil Deakin, not me. > or making this method to return calculated margin when ContentRangeInserted() > triggers a11y notfication? This can't be done; whether the scrollbars are visible depends on the sizes of the scrollframe and its kids; see above. How did any of this use to work? Or did it not use to work? ;)
(In reply to comment #71) > > For some cases a11y relies on assumption element's size is calculated to > > create an accessible > > Then it needs to do that after layout.... > > 1. force layout to calculate size (that's not good idea from perf point of > > view I think) > > Also can't be done inside the frame construction code. Ok, that works I think. This could be done in similar way as the patch makes for plugin, i.e. we need to create an accessible after layout. The question is when should html scroll frame notify accessibility about that? > > Note that whether an overflow:auto element has scrollbars (and thus whether you > want to create an accessible?) can change without any style changes on the > element itself. Yes, scrollbar presence makes HTML scroll area focusable, any focusable should be exposed to AT. We need to handle the case of non style changes affecting on scrollbar presence. > > > Boris, is there a way to change nsFrame::IsFocusable() for HTML scroll frames > > to remove dependance from nsIScrollableFrame::GetActualScrollbarSizes() > > That's a question for Neil Deakin, not me. That's preferable way than accessible creation after layout. If that's possible then that's the way we should go.
I ran the latest try-server build with various scenarios. The crashes are gone. I did see an update problem on the neu.de page that I also saw in one of the earlier perf bugs: A section where one of the members is being presented while the rest of the page shows different parts like visitors etc., is again missing all that member info. This was also the case in one of the earlier perf bugs. Alex, after logging into neu.de, click on "Besucher". There should be info for 1 particular member there that is missing with this build. This is between the advertisement for the plus membership and the actual short list of visitors.
Marco, just to make sure, the problem is different than the problem described in bug 566103 comment #15, right? If yes then could you please find what was that perf bug?
Attached patch wip5 (obsolete) — — Splinter Review
Attachment #479410 - Attachment is obsolete: true
(In reply to comment #72) > > > > > Boris, is there a way to change nsFrame::IsFocusable() for HTML scroll frames > > > to remove dependance from nsIScrollableFrame::GetActualScrollbarSizes() > > > > That's a question for Neil Deakin, not me. > > That's preferable way than accessible creation after layout. If that's possible > then that's the way we should go. I talked to Neil and he showed me this culprit: Bug 257938. I tend to agree with Roc in that bug... not sure why it was really necessary.
(In reply to comment #77) > I talked to Neil and he showed me this culprit: Bug 257938. I tend to agree > with Roc in that bug... not sure why it was really necessary. So is it ok to remove this part of code (http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#6435)? Robert?
(In reply to comment #78) > (In reply to comment #77) > > > I talked to Neil and he showed me this culprit: Bug 257938. I tend to agree > > with Roc in that bug... not sure why it was really necessary. > > So is it ok to remove this part of code > (http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#6435)? > Robert? It shouldn't be removed no. The suggestion is to revert the change made in that bug. Currently, the decision whether a scrollable area is focusable is determined by whether scrollbars are visible, which means that the element could return different results if the window is resized. Instead, the old way way to just assume that anything with overflow: auto or overflow: scroll was focusable.
Ok, thanks.
> The question is when should html scroll frame notify accessibility about that? Presumably whenever it shows or hides scrollbars? But are you sure you need accessible objects for these at all? They're focusable when they have scrollbars so you can scroll them with the keyboard; is that something a11y users need to care about?'
(In reply to comment #81) > > The question is when should html scroll frame notify accessibility about that? > > Presumably whenever it shows or hides scrollbars? I think yes. When it's time to call GetActualScrollbarSizes(). > But are you sure you need accessible objects for these at all? They're > focusable when they have scrollbars so you can scroll them with the keyboard; > is that something a11y users need to care about?' Yes, any focusable element must be accessible, otherwise if not accessible element is focused then AT users don't know where's the focus and what they can do. But it sounds Neil suggest to back out the patch of bug 257938, that solves the problem I hope.
(In reply to comment #74) > Marco, just to make sure, the problem is different than the problem described > in bug 566103 comment #15, right? If yes then could you please find what was > that perf bug? Slightly, but the same symptom: The same parts of neu.de don't appear, but this time, a refresh of NVDA's virtual buffer doesn't bring the content back. It stays invisible. I'm currently downloading the latest try-server and will report back.
The latest try-server build still has the same problems. Basically the comment from bug 566103 comment#15 applies fully here as well. There are inconsistencies with trying to pull the mouse to a particular tweet on twitter.com, and the controls for tweets appear somewhere else on a different tweet, not the one I intend to trigger. And on neu.de, the individual "now online" profiles also don't appear yet.
Marco, could you please provide exact steps to reproduce the bug for neu.de for example to make sure it try right things in right way?
Attached patch wip6 (obsolete) — — Splinter Review
Attachment #480086 - Attachment is obsolete: true
1. go to http://neu.de 2. Log in with the credentials I gave you. 3. After log in, click on "Besucher". 4. On the next page, you should see a) a list of visitors in the main area. b) a single member with more info than just the name and city. While a) is there, b) is not in the NVDA virtual buffer.
(In reply to comment #88) > 1. go to http://neu.de > 2. Log in with the credentials I gave you. > 3. After log in, click on "Besucher". > 4. On the next page, you should see > a) a list of visitors in the main area. > b) a single member with more info than just the name and city. > While a) is there, b) is not in the NVDA virtual buffer. While I arrowed down through the page (though members I think) I've heard their offline status and the time when they were last time. That means the problem should be fixed in last build, right?
Comment on attachment 480596 [details] [diff] [review] wip6 I tried this patch in a local build, and it is working less than the last try-server build: 1. On Twitter, I can't see any of the "reply/Retweet" actions on any of the tweets any more. This is exactly as it was in bug 566103 Comment#15 now. 2. On neu.de, after logging in and clicking on "Besucher", the ONLY frame that comes up is on that has an ad in it. The other frames all appear blank. Also, an NVDA refresh doesn't help. So now, the main page content is completely gone. So, turn the screw driver in the other direction and we'll be good <Grin>.
It appears I need __exact__ steps to reproduce because German words in English frustrates me :). Could you please describe steps? For example, "tab - you should hear this but you get that". That should really help me while I'm playing with build trying to figure if this or that change helps.
For twitter: 1. Go to twitter.com and log in. 2. You'll see a list of tweets from people you are following. 3. With NVDA, down arrow to one of the tweets and place the virtual cursor onto the message text. 4. Press NVDA+NUMPAD 1. Expected (and the case with regular nightly): NVDA repeats the text of the tweet. Actual: NVDA says something like "paragraph" or "section". This is already where the problem starts: NVDA does not focus on the actual text the virtual cursor is pointing to. 5. Press NVDA+NUMPAD SLASH. This should route the mouse to the tweet, but in case of the try-server build, will not. Result: With a regular nightly, you should see that visually, options to favorite a tweet, to reply and to retweet will appear. Actual: This will not happen. The mouse will be placed somewhere else. In the case of neu.de, it is already enough to just log in, click "Besucher", and then try to read the next page with the arrow keys. You'll only hear gibberish relating to one of the advertisement banners, no actual page content.
Additionally, the latest WIP regresses the test cases from bug 570500: a) LFC1 and LFC 2: The button text does not get updated after the test finishes: 1. With NVDA running, load each of the two LFC test cases. 2. Press the "Test (can freeze your browser)" button. 3. After the test finishes, press up and down arrow once. With a regular nightly: The button text gets updated to read the actual result. With the local build containing latest WIP: The button text still reads "Test (can freeze your browser)". You *have* to press NVDA+F5 (NUMPAD 0 + F5) to refresh the buffer. The name change of the button is not being picked up automatically by just arrowing up and down. b) The table mutation test never finishes loading. I gave up waiting for it after 5 minutes. With a regular current nightly build, NVDA's speech comes up after at most 30 to 40 seconds and one can actually run the test.
Blocks: a11yperf_fx4
(In reply to comment #93) > Additionally, the latest WIP regresses the test cases from bug 570500: > a) LFC1 and LFC 2: The button text does not get updated after the test > finishes: > 1. With NVDA running, load each of the two LFC test cases. > 2. Press the "Test (can freeze your browser)" button. > 3. After the test finishes, press up and down arrow once. > > With a regular nightly: The button text gets updated to read the actual result. > With the local build containing latest WIP: The button text still reads "Test > (can freeze your browser)". You *have* to press NVDA+F5 (NUMPAD 0 + F5) to > refresh the buffer. The name change of the button is not being picked up > automatically by just arrowing up and down. I can't reproduce with build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-4ee053125573.
Attached patch wip7 (obsolete) — — Splinter Review
wip for latest try server build
Comment on attachment 480900 [details] [diff] [review] wip7 With this latest WIP: 1. LFC tests work again with the name change being reflected by NVDA immediately. Note that this patch causes a 50% speed increase, taking the post processing time down from over 4000 or 50000 MS to something around 2500 MS for the second test. 2. The RemoveChildren test also works. 3. The Table Mutation test also works, but heavily exposes problems described in bug 599814. However, the time and post processing time are cut down to 2031 and 1954 MS respectively. This is fantastic! 4. Twitter still doesn't work for me as described for WIP6. 5. neu.de now shows *some* of the info. It allows to go to "Besucher" again without losing all the content. It shows the info up to "zu meinen Favoriten? Ja | Nächste", but not the details tables below that. AND one has to refresh NVDA's virtual buffer after clicking on "Nächste", for example. It seems a reorder event is missing here or the like that tells NVDA to refresh its view. After clicking "Nächste", the info NVDA sees is still that of the old member, until I refresh with NVDA+F5. There's definitely good progress in this patch in the right direction here!
Attachment #480900 - Attachment is patch: true
Attachment #480900 - Attachment mime type: application/octet-stream → text/plain
(In reply to comment #96) > Comment on attachment 480900 [details] [diff] [review] > wip7 > 5. neu.de now shows *some* of the info. It allows to go to "Besucher" again > without losing all the content. It shows the info up to "zu meinen Favoriten? > Ja | Nächste", but not the details tables below that. AND one has to refresh > NVDA's virtual buffer after clicking on "Nächste", for example. It seems a > reorder event is missing here or the like that tells NVDA to refresh its view. > After clicking "Nächste", the info NVDA sees is still that of the old member, > until I refresh with NVDA+F5. I can reproduce this, the table is skipped when I arrow down through the page. We definitely fire show event for table container and reorder event for container's container and the tree contains this table when reorder event is handled (and of course it's not missed in the end when page was completely loaded). I don't see any other events that may cancel this reorder event affect. Honestly I don't have any ideas. Perhaps NVDA ignores some events at some point. Jamie, can I ask you to look on your side?
(In reply to comment #97) > We definitely fire show event for table container and reorder event for > container's container and the tree contains this table when reorder event is > handled (and of course it's not missed in the end when page was completely > loaded). I don't see any other events that may cancel this reorder event > affect. Honestly I don't have any ideas. Perhaps NVDA ignores some events at > some point. It's worth noting that we don't handle show and hide events for the buffer. We rely on reorder, text inserted and text removed, as well as property change events. Also, we never handle an event immediately. Instead, we queue all events within a 300 ms period and re-render the common ancestor for all of those events. > Jamie, can I ask you to look on your side? I'll try to take a look at this when I get some time. However, have you checked with accProbe to ensure that the events are being fired to the platform API?
(In reply to comment #98) > > Jamie, can I ask you to look on your side? > I'll try to take a look at this when I get some time. Thank you, much. > However, have you checked > with accProbe to ensure that the events are being fired to the platform API? Unfortunately I tested crossplatform part only by DOMi. I've got some problems with accProbe/JAVA on my machine.
Jamie, basing on assumption the tree is correct and you receive right events what kind of info you request from accessible before putting it into virtual buffer? I wonder whether some information is wrong because layout stuffs weren't finished at that point.
(In reply to comment #99) > > However, have you checked > > with accProbe to ensure that the events are being fired to the platform API? > > Unfortunately I tested crossplatform part only by DOMi. I've got some problems > with accProbe/JAVA on my machine. Yes, I see reorder event for HTML div accessible containing that table in accprobe.
Attached patch wip8 (obsolete) — — Splinter Review
Attachment #480596 - Attachment is obsolete: true
Attachment #480900 - Attachment is obsolete: true
Some additional data points from some quick tests with wip8: When I log into twitter (old twitter), I see 6 reorder events via accprobe, including 2 for the "Twitter/Home" document. When I log in using new twitter, I see 12 reorder events via accprobe, including 3 for the "Twitter/Home" document. Looking at the accessible tree to see what the other reorder events were on crashed accprobe unfortunately.
(In reply to comment #97) > Jamie, can I ask you to look on your side? Can I have a try build please? (In reply to comment #100) > Jamie, basing on assumption the tree is correct and you receive right events > what kind of info you request from accessible before putting it into virtual > buffer? Er... quite a lot: pretty much all basic properties (name, value, states, keyboard shortcut, location, etc.), object attributes, accessible text, accessible table info, etc. The only things that should cause the content of a node to be ignored are a child count of 0 and width and height of 0. My explanation above regarding our event handling was slightly incorrect. We queue all events within a 300 ms period. Once that 300 ms period elapses, we re-render any subtrees that changed (only once). For example, if a reorder event was fired on the document, we'll re-render the entire document regardless of other events because the entire document tree has changed. The important thing to note is that if the div containing the table wasn't in the tree when the document was first rendered, it will never get handled.
(In reply to comment #104) > (In reply to comment #97) > > Jamie, can I ask you to look on your side? > Can I have a try build please? You could try build for wip7 (http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-4ee053125573). I'll start build for wip8. I see the same results for these wips.
Jamie, do you ignore reorder/text change events under certain circumstances for example if you receive them before the iframe was loaded?
(In reply to comment #106) > Jamie, do you ignore reorder/text change events under certain circumstances for > example if you receive them before the iframe was loaded? I don't quite understand. We don't create a buffer until the main document (tab document) is not busy. Beyond that, we don't ignore any events. If an iframe changes, it should fire an event, so we then re-render it. If a node fires a reorder but its content isn't ready yet, this may cause problems because we won't be able to render it.
(In reply to comment #107) > (In reply to comment #106) > > Jamie, do you ignore reorder/text change events under certain circumstances for > > example if you receive them before the iframe was loaded? > I don't quite understand. We don't create a buffer until the main document (tab > document) is not busy. Beyond that, we don't ignore any events. Ok, that shouldn't be this case. I think frames are changed, the tab document left unchanged. > If an iframe > changes, it should fire an event, so we then re-render it. We did this prior this patch, but this patch fires events for stuffs that were actually changed. > If a node fires a > reorder but its content isn't ready yet, this may cause problems because we > won't be able to render it. If you get reorder event then some children must be presented, if new children will be appended or grandchildren will be changed then other reorder events should be fired.
Jamie, did you have a chance to look what's wrong with neu.de/twitter and this build on NVDA side?
Depends on: 604293
Depends on: 604296
Attached patch wip9 (obsolete) — — Splinter Review
Attachment #481282 - Attachment is obsolete: true
(In reply to comment #92) > For twitter: > 1. Go to twitter.com and log in. > 2. You'll see a list of tweets from people you are following. > 3. With NVDA, down arrow to one of the tweets and place the virtual cursor onto > the message text. > 4. Press NVDA+NUMPAD 1. > Expected (and the case with regular nightly): NVDA repeats the text of the > tweet. I don't get this even with a regular nightly, nor Firefox 3.6. > Actual: NVDA says something like "paragraph" or "section". This is already > where the problem starts: NVDA does not focus on the actual text the virtual > cursor is pointing to. Try reading with the review cursor. NVDA doesn't announce text in paragraphs when moving by object. > 5. Press NVDA+NUMPAD SLASH. This should route the mouse to the tweet, but in > case of the try-server build, will not. > Result: With a regular nightly, you should see that visually, options to > favorite a tweet, to reply and to retweet will appear. Again, I don't reliably see this with a Minefield nightly, nor Firefox 3.6. There's definitely some weirdness here. It looks like Firefox is returning the wrong location via IAccessibleText in some cases. However, I think we need to disregard this test case for this bug, as it is very unreliable for me even in 3.6, and I'm not seeing different behaviour between the try build and a regular Minefield nightly. > In the case of neu.de, it is already enough to just log in, click "Besucher", > and then try to read the next page with the arrow keys. You'll only hear > gibberish relating to one of the advertisement banners, no actual page content. Last time i logged into neu.de, I didn't even see the Besucher link. Has the site changed somehow? Frustrating...
With the latest try-server build: 1. NOn neu.de, the problem is the same as before. When I logged in, I did see the "Besucher" link. If you don't see it, try finding a link called "Direkt zu neu.de" and click it first. Sometimes they seem to push a different page to the user after logging in. 2. On Twitter, I now see "Favorite", "reply" and "Retweet" for *every* tweet without having to hover the mouse anywhere. It does not appear like this with a regular nightly. Appears as if the visibility of these items is no longer honored or something else is triggering it for every tweet automatically.
(In reply to comment #114) > 2. On Twitter, I now see "Favorite", "reply" and "Retweet" for *every* tweet > without having to hover the mouse anywhere. It does not appear like this with a > regular nightly. Appears as if the visibility of these items is no longer > honored or something else is triggering it for every tweet automatically. This is because this patch reverts part of bug 309329. We started to create an accessible object even if it's invisible letting AT to filter what they don't need. I was forced to do this because when we create an accessible then its visibility may be not calculated yet - I've got this problem in real life, when open CKEditor table dialog.
(In reply to comment #114) > 2. On Twitter, I now see "Favorite", "reply" and "Retweet" for *every* tweet > without having to hover the mouse anywhere. It does not appear like this with a > regular nightly. Ack. Sorry; I forgot to mention this. I got confused after testing this for quite a while. :) The link (and several of its ancestors) also has the invisible state. (In reply to comment #115) > This is because this patch reverts part of bug 309329. We started to create an > accessible object even if it's invisible letting AT to filter what they don't > need. This is bad for us because we ignore the invisible state due to bug 591363.
(In reply to comment #116) > (In reply to comment #115) > > This is because this patch reverts part of bug 309329. We started to create an > > accessible object even if it's invisible letting AT to filter what they don't > > need. > This is bad for us because we ignore the invisible state due to bug 591363. This might be not so bad as it sounds. I should note twitter issue and I think Marco's facebook issue are about using visibility style. We expose accessibiles if their DOM nodes are not visible when visibility style is used, for example, visibility: hidden like in twitter case or I suppose visibility: collapsed in Facebook case. The same time note, we don't expose accessible if their nodes has display:none style which is used to show/hide some things usually. In comment #4 I suggested to drop visibility for now and I didn't get any warnings from you. I think it's correct to expose accessibles with visibility: hidden because they take a place on the screen, they just not rendered. In this case state invisible is right thing to do. It might make sense to do the same for visibility: collapse so that we keep visibility styles in the same way. Taking into account we have a bug 591363, I would call this as a feature, not as a bug. Anyway I suggest to deal with this issue separately. I realize this brings some undesirable issues with current screen readers but I think it's worth to deal with this problem in follow up, at least while the "right" behavior is not clear.
(In reply to comment #117) > > This is bad for us because we ignore the invisible state due to bug 591363. > This might be not so bad as it sounds. I should note twitter issue and I think > Marco's facebook issue are about using visibility style. We expose accessibiles > if their DOM nodes are not visible when visibility style is used, for example, > visibility: hidden like in twitter case or I suppose visibility: collapsed in > Facebook case. The author still intended these to be invisible, so they shouldn't be visible to AT users either. Otherwise, there could be a lot of unnecessary clutter on the page as in the Twitter case. > In comment #4 I suggested to drop visibility for now and I didn't get any > warnings from you. Sorry - my bad. I didn't catch that comment. If I had, I would have given the same warning. > I think it's correct to expose accessibles with visibility: hidden because they > take a place on the screen, they just not rendered. In this case state > invisible is right thing to do. It might make sense to do the same for > visibility: collapse so that we keep visibility styles in the same way. As I understand it, visibility: collapse removes the row and allows its space ot be used by other content. Perhaps I am misunderstanding this, though. > Taking into account we have a bug 591363, I would call this as a feature, not > as a bug. Anyway I suggest to deal with this issue separately. I'll comment on that bug as well and continue further discussion there. > I realize this brings some undesirable issues with current screen readers but I > think it's worth to deal with this problem in follow up, at least while the > "right" behavior is not clear. It's worth noting that this is a big backwards compatibility issue and ATs are going to have to do version checks to know whether it's safe to use the invisible state.
(In reply to comment #118) > The author still intended these to be invisible, so they shouldn't be visible > to AT users either. Otherwise, there could be a lot of unnecessary clutter on > the page as in the Twitter case. Right. But the question is how we should do this. Either we should prune them from accessibility tree or expose them with invisible state. Note, the way we handle visibility style has problems, bug 355521. > > In comment #4 I suggested to drop visibility for now and I didn't get any > > warnings from you. > Sorry - my bad. I didn't catch that comment. If I had, I would have given the > same warning. Not yours actually. > As I understand it, visibility: collapse removes the row and allows its space > ot be used by other content. Perhaps I am misunderstanding this, though. I didn't check Facebook issue, but this sounds likewise. > > > Taking into account we have a bug 591363, I would call this as a feature, not > > as a bug. Anyway I suggest to deal with this issue separately. > I'll comment on that bug as well and continue further discussion there. Ok, thank you. So are you fine to deal with it separately? > It's worth noting that this is a big backwards compatibility issue and ATs are > going to have to do version checks to know whether it's safe to use the > invisible state. Right. I need to check this with other AT vendors.
Comment on attachment 484257 [details] [diff] [review] wip9 So, asking for review. It seems Jamie is fine to deal with this issue separately per his comments and irc. Hoping you're ok with this as well.
Attachment #484257 - Flags: review?(marco.zehe)
Attachment #484257 - Flags: review?(bolterbugz)
Comment on attachment 484257 [details] [diff] [review] wip9 r=me for the test part, and the functionality seems to be working fine now, too.
Attachment #484257 - Flags: review?(marco.zehe) → review+
Attached patch patch — — Splinter Review
Boris, could you do review for content and layout changes. Do I need sr for these changes?
Attachment #484257 - Attachment is obsolete: true
Attachment #484689 - Flags: review?(bzbarsky)
Attachment #484689 - Flags: review?(bolterbugz)
Attachment #484257 - Flags: review?(bolterbugz)
Comment on attachment 484689 [details] [diff] [review] patch r=me. Those changes make me really happy! May be worth a followup bug to add the accessibility service to the service cache... I don't think you need sr here past what I can do. That said, your ContentRangeInserted seems to assume aContainer is not null. That's not going to be true for some cases (e.g. insertion of the root node of a document). That needs to be fixed.
Attachment #484689 - Flags: review?(bzbarsky) → review+
Comment on attachment 484689 [details] [diff] [review] patch We'll want to get this in beta7 if possible, since we change nsIAccessibilityService. Need to decide what to do for ContentRangeInserted when the container is null, possibly nothing. I like the change to nsAccessible::Shutdown. Just curious, do we hit the "Binding to the same parent" at all in nsAccessible::BindToParent? (When?) // Yes, this means we're only compatibible with 32 bit // MSAA is only available for 32 bit windows, so it's okay - return - NS_PTR_TO_INT32(uniqueID); + return aAccessible ? - NS_PTR_TO_INT32(aAccessible->UniqueID()) : 0; (Do we have a bug filed for this?) + // Ignore hidden accessible for name computation. Do we have test coverage for this case? Also, I didn't look over the tests here closely. Do we still have good visibility style change tests? +bool +nsHTMLComboboxListAccessible::IsPrimaryForNode() const +{ + return false; +} Please add a brief comment why we return false (same for nsHTMLListBulletAccessible, nsXULTreeGridCellAccessible) + nsIAtom* aid = cnt->GetID(); nit: aId or just id please. I think we need to land this sooner than later, and jump on any followup. r=me. Epic.
Attachment #484689 - Flags: review?(bolterbugz) → review+
> possibly nothing. How could that possibly be correct?
(In reply to comment #125) > > possibly nothing. > > How could that possibly be correct? I meant bail out == nothing :)
BTW Alexander, it is nice to see NodeToAccessibleMap implemented and working!
> I meant bail out == nothing :) Yes, I assumed that. My question stands! Don't you need to create accessibles for the newly inserted stuff?
(In reply to comment #128) > > I meant bail out == nothing :) > > Yes, I assumed that. My question stands! Don't you need to create accessibles > for the newly inserted stuff? Yep, we do need to create them. I was thinking that we'd trigger the creation from a subsequent event but I didn't think it through. We'll need to handle this case but I'm not sure how it is manifesting right now. I'd be okay with drilling down on it in a follow up if you agree.
(In reply to comment #123) > Comment on attachment 484689 [details] [diff] [review] > That said, your ContentRangeInserted seems to assume aContainer is not null. > That's not going to be true for some cases (e.g. insertion of the root node of > a document). That needs to be fixed. Boris, you've got me worried. What are all the cases where the root node of a document get inserted? Also, what other cases might include a null aContainer?
> What are all the cases where the root node of a document get inserted? Uh... It's just an element. The <html> element in a typical HTML document. It can get inserted via DOM calls, via display changes on that element, etc, etc. > Also, what other cases might include a null aContainer? That's it. Just insertion of the root element. I should have been clearer on that. Well, removal of the root will have a null aContainer too in ContentRemoved.
OK I think we go up the parent chain looking for the nearest container. I haven't debugged that code in a long while. Let's see what Alexander has to say.
(In reply to comment #124) > // MSAA is only available for 32 bit windows, so it's okay Was this comment meant to be Firefox specific (i.e. MSAA Is only available for 32 bit Windows in Firefox)? MSAA is most certainly available for 64 bit Windows applications, but the IDs must be 32 bit, so a bug needs to be filed to follow this up if one hasn't been already. I assume this is what was meant by: > (Do we have a bug filed for this?)
We ignore documents without document element or body element in HTML case so that the document accessible is tightly related with those elements and we don't need to create accessibles for them. I'm not aware of the case where it's used and whether this is important to expose in accessibility. I think it's ok to ignore these notifications for now.
(In reply to comment #133) > (In reply to comment #124) > > // MSAA is only available for 32 bit windows, so it's okay > Was this comment meant to be Firefox specific (i.e. MSAA Is only available for > 32 bit Windows in Firefox)? MSAA is most certainly available for 64 bit Windows > applications, but the IDs must be 32 bit, so a bug needs to be filed to follow > this up if one hasn't been already. I assume this is what was meant by: > > (Do we have a bug filed for this?) If IDs must be 32 bit then all we need to fix is the comment, no?
> and we don't need to create accessibles for them. So if I build a DOM in memory and insert it into a document, you don't need to do anything with any of those nodes?
(In reply to comment #124) > Comment on attachment 484689 [details] [diff] [review] > patch > > We'll want to get this in beta7 if possible, since we change > nsIAccessibilityService. if you mean interfaces freezing then it's not the case I think since nsIAccessibilityService is for internal needs only. Extensions should rely on nsIAccessibleRetrieval. > Just curious, do we hit the "Binding to the same parent" at all in > nsAccessible::BindToParent? (When?) I've never seen. I just reworked assertions in this place. > + // Ignore hidden accessible for name computation. > > Do we have test coverage for this case? Yes, otherwise I wouldn't add this line ;) > Also, I didn't look over the tests here closely. Do we still have good > visibility style change tests? I removed them, since we do nothing for visibility style changes. We will decide how to deal with them in follow up (for example fire invisible state change event). > > +bool > +nsHTMLComboboxListAccessible::IsPrimaryForNode() const > +{ > + return false; > +} > > Please add a brief comment why we return false (same for > nsHTMLListBulletAccessible, nsXULTreeGridCellAccessible) I don't think this is right place, they aren't primary because they don't "own" a node. The accessible hierarchy for these specific cases should be documented in header files, and it's done, at least partially. Perhaps it makes sense to extend a comment of IsPrimaryNode method in nsAccessNode. > + nsIAtom* aid = cnt->GetID(); > > nit: aId or just id please. aId looks like argument, id is already there :) aid means "atom id".
(In reply to comment #135) > > MSAA is most certainly available for 64 bit Windows > > applications, but the IDs must be 32 bit, so a bug needs to be filed to follow > > this up if one hasn't been already. > If IDs must be 32 bit then all we need to fix is the comment, no? That depends. In a 64 bit environment, I assume you *could* have 64 bit pointers which don't fit into 32 bits. Does NS_PTR_TO_INT32 simply cast the value or does it create a map somewhere? If it just casts, this will potentially break in 64 bit environments, as you could conceivably have two memory addresses with the same 32 bit ID.
(In reply to comment #136) > > and we don't need to create accessibles for them. > > So if I build a DOM in memory and insert it into a document, you don't need to > do anything with any of those nodes? It's like build DOM starting from root element and then replace existing root element on this one? Is it possible to have document in existing DOM tree without root element and then insert root element into it? We need to address this. Currently we were under assumption root element or body isn't changed, the document accessible keeps pointer on it and it's not get updated if changes occurs. I'll file bug for this. Thank you for bringing this issue.
(In reply to comment #123) > Comment on attachment 484689 [details] [diff] [review] > patch > > r=me. Those changes make me really happy! thank you, these changes keep me so happy as well :) > > May be worth a followup bug to add the accessibility service to the service > cache... we keep accessibiltiy service instance in cache in a11y and return it when we were asked, but if this is going to speed up things then it's worth to do.
(In reply to comment #138) > (In reply to comment #135) > > > MSAA is most certainly available for 64 bit Windows > > > applications, but the IDs must be 32 bit, so a bug needs to be filed to follow > > > this up if one hasn't been already. > > If IDs must be 32 bit then all we need to fix is the comment, no? > That depends. In a 64 bit environment, I assume you *could* have 64 bit > pointers which don't fit into 32 bits. Does NS_PTR_TO_INT32 simply cast the > value or does it create a map somewhere? If it just casts, this will > potentially break in 64 bit environments, as you could conceivably have two > memory addresses with the same 32 bit ID. It's just a cast. I'll file bug.
> Is it possible to have document in existing DOM tree without root element and > then insert root element into it? Yes, of course. > Currently we were under assumption root element or body isn't changed Yeah, that's a bad assumption. <body> will change on document.open, and document.removeChild(document.documentElement) works just fine... ;)
> but if this is going to speed up things then it's worth to do Yeah; it'll avoid going through the service manager at all to get the service..
I filed bug 606082 for root element change problem, bug 606080 for NS_PTR_TO_INT32 issue.
(In reply to comment #143) > > but if this is going to speed up things then it's worth to do > > Yeah; it'll avoid going through the service manager at all to get the service.. I filed bug 606085 for this.
(In reply to comment #46) > > I tried the testcase and when I tabbed through links then I get > > ContentRemoved/ContentRangeInserted pair for blur and focused links. > > Right. Like I said, we could avoid that for the specific case of overflow on > inlines; might be worth filing a bug. I filed bug 606087.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 604560
(In reply to comment #113) > > 5. Press NVDA+NUMPAD SLASH. This should route the mouse to the tweet, but in > > case of the try-server build, will not. > > Result: With a regular nightly, you should see that visually, options to > > favorite a tweet, to reply and to retweet will appear. > Again, I don't reliably see this with a Minefield nightly, nor Firefox 3.6. > > There's definitely some weirdness here. It looks like Firefox is returning the > wrong location via IAccessibleText in some cases. However, I think we need to > disregard this test case for this bug, as it is very unreliable for me even in > 3.6, and I'm not seeing different behaviour between the try build and a regular > Minefield nightly. Jamie, please do some investigations and file a bug, this sounds like quite serious problem.
Depends on: 606125
Depends on: 606207
Nice, looks like we got a 3.07% performance improvement on Linux x64 Firefox.
(In reply to comment #149) > Nice, looks like we got a 3.07% performance improvement on Linux x64 Firefox. not big deal for a11y I think, do we have right perf tests for accessible tree mutation?
Depends on: 606962
Depends on: 606961
Blocks: 579964
Depends on: 607882
Depends on: 610317
Depends on: 614829
Depends on: 636976
Depends on: 1532965
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: