Closed Bug 606125 Opened 14 years ago Closed 14 years ago

develop a way to handle visibility style

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b8
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 3 obsolete files)

patch4
46.75 KB, patch
Details | Diff | Splinter Review 
After bug 570275 is fixed accessibles with visibility style applied (like hidden or collapsed) are exposed to AT, that bug reverted bug 309329.

I find correct to create accessibles when this style is applied (because accessible may take a place on the screen and it might be not right to hide this information from AT). I think these accessible should be exposed with invisible state. But this breaks backward compatibility since for example NVDA doesn't rely on invisible style change, bug 591363.

Another issue if AT is going to ignore invisible accessibles then they need a way to get notified accessible appears or hidden. We could fire invisible state change event but that's different way for AT to deal with show/hide changes.

We might extend definition of show/hide/reorder events to fire them when visibility of accessible is changed.

I'm looking forward to hear your opinions.
blocking2.0: --- → ?
(In reply to comment #0)
> I find correct to create accessibles when this style is applied (because
> accessible may take a place on the screen and it might be not right to hide
> this information from AT).
I can't see why an AT would care about invisible objects, but regardless, ATs should indeed honour the invisible state.
Out of interest (and for the sake of history), what was the test case that prompted you to change this and why?

> Another issue if AT is going to ignore invisible accessibles then they need a
> way to get notified accessible appears or hidden. We could fire invisible state
> change event but that's different way for AT to deal with show/hide changes.
I don't think another event is a good idea.

> We might extend definition of show/hide/reorder events to fire them when
> visibility of accessible is changed.
Personally, I think this is the best approach. As noted, I can't see why an AT would care about invisible objects. If they do, it'll be for a very specific reason and they'll probably get to them by traversal, rather than via events. So:
* Show/hide shouldn't be fired on invisible objects.
* Show should be fired when an object becomes visible.
* Hide should be fired when an object goes invisible.
(In reply to comment #1)
> (In reply to comment #0)
> > I find correct to create accessibles when this style is applied (because
> > accessible may take a place on the screen and it might be not right to hide
> > this information from AT).
> I can't see why an AT would care about invisible objects, but regardless, ATs
> should indeed honour the invisible state.

I don't know exposing invisible accessible having not empty size allows to return "right" accessible from point, if it's valuable. For example, if HTML input is overlapped with visibility hidden div then would you like to treat that mouse is over on HTML input, when actually it's on hidden div that have not null size. For example, if you click on it then HTML input won't be activated though it should be expected if we ignore this hidden accessible.

> Out of interest (and for the sake of history), what was the test case that
> prompted you to change this and why?

I wasn't inspired by test case, but the way how Gecko works and a difference between display/DOM mutation and visibility style, for example,

<div style="visibility: hidden">text1<div style="visibility:visible">text2</div</div>

We didn't handle this example correctly if it's inserted dynamically. Any visibility change was treated as display change what's wrong.

So if layout frames are created for visibility hidden nodes then it's easy and performant to keep them accessible. Otherwise we need to traverse whole subtree to check what's visible and what's not.

for the same reason it might be not correct to fire show/hide events since currently they means the target subtree was shown or hidden as well (until we change their meaning of course).

 Perhaps it makes sense to fire create/destroy events when accessibles are created/destroyed and fire show/hide events when they appear/hidden. Though this is even more serious change, but apparently it's possible no any existing AT listen show/hide events.

Also reorder event makes me bother because children aren't changed in this case.
(In reply to comment #2)
> I don't know exposing invisible accessible having not empty size allows to
> return "right" accessible from point, if it's valuable.
I don't think accessible from point should ever return an invisible accessible. I really can't see why an AT would want this.

> I wasn't inspired by test case
In bug 570275 comment #115, you wrote:
> 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.
This is what I was referring to. Can you elaborate on this? I don't quite understand.

> for the same reason it might be not correct to fire show/hide events since
> currently they means the target subtree was shown or hidden as well (until we
> change their meaning of course).
Ug. That's one major disadvantage of including invisible nodes in the tree. It doesn't really make sense in most cases to have an invisible accessible which contains visible accessibles. Certainly, I don't think I've seen this anywhere else. If the meaning is changed, they'll have to be fired on *every* node to be certain, which is really ugly. This may also cause a big mess for live regions, where show/hide events also refer to subtrees.

>  Perhaps it makes sense to fire create/destroy events when accessibles are
> created/destroyed and fire show/hide events when they appear/hidden. Though
> this is even more serious change, but apparently it's possible no any existing
> AT listen show/hide events.
We don't listen to create/destroys either, as normally, we don't care and there are just too many of them.

> Also reorder event makes me bother because children aren't changed in this
> case.
Reorder probably doesn't need to change. However, text inserted/removed do need to fire when these embedded objects are added/removed.
(In reply to comment #3)
> (In reply to comment #2)
> > I don't know exposing invisible accessible having not empty size allows to
> > return "right" accessible from point, if it's valuable.
> I don't think accessible from point should ever return an invisible accessible.
> I really can't see why an AT would want this.

That depends what you do on mouse move and how you use accessible from point. For example if you announce an accessible object under mouse pointer then if we skip invisible but having a size accessible object then you're going to announce wrong information.

Let's consider an example.

<button>button</button>

<div style="width: 200px; height: 200px; border: 4px solid red; background-color: blue; position: absolute; left: 0px; top: 0px;">
</div>

Here's you see a div with red border and blue background, you don't see a button, can't operate with it because div is placed on top of the button. Let's we ignore this invisible accessible that means AT users will see a button but sighted users won't see it.

> > open CKEditor table dialog.
> This is what I was referring to. Can you elaborate on this? I don't quite
> understand.

Ah, that one. I hadn't a chance to understand what do they do exactly and get a testcase (perhaps I should ask them for a help) but that was a problem I assume visibility styles weren't calculated while we asked for them.

But that's not a deal actually. Since we don't fire show/hide events when visibility styles are changed then it's wrong to create or not create an accessible depending on its visibility style.

> > for the same reason it might be not correct to fire show/hide events since
> > currently they means the target subtree was shown or hidden as well (until we
> > change their meaning of course).
> Ug. That's one major disadvantage of including invisible nodes in the tree. It
> doesn't really make sense in most cases to have an invisible accessible which
> contains visible accessibles.

Ok, but when visibility hidden accessible having visible children appears or accessible gets visibility hidden while its children stay visible then it's not clear what events we should fire. We can't use show/hide without their meaning changes. 

> > Also reorder event makes me bother because children aren't changed in this
> > case.
> Reorder probably doesn't need to change. However, text inserted/removed do need
> to fire when these embedded objects are added/removed.

In general invisible accessibles shouldn't be exposed in text since there's not a lot sense. But since invisible accessible may contain visible children then it's getting tricky if we expose invisible accessibles. On the another hand since visibility styles is not unique way to hide a node then it invisible accessible ignorance is tricky thing as well. And it doesn't make things clear  with events.
(In reply to comment #4)

> Let's consider an example.
> 
> <button>button</button>
> 
> <div style="width: 200px; height: 200px; border: 4px solid red;
> background-color: blue; position: absolute; left: 0px; top: 0px;">
> </div>

sorry, I've got tired. ignore this example please
Ok, I can't think of example when it's necessary to expose invisible accessibles. Perhaps it's reasonable to exclude them from hierarchy making invisible state not applicable to any accessible.
(In reply to comment #6)
> Ok, I can't think of example when it's necessary to expose invisible
> accessibles. 

Here's one (not related with visibility style though):

<ul>
<li style="display: block; float: left">list1
<li style="display: block; float: left">list2
</ul>

here's UL is invisible since it has 0 height but it's important for AT I think.
it's real example from http://www.microsoft.com/enable/, they do horizontal list
Blocks: 606207
So the problem is much wider than expose or not expose visibility hidden. And it doesn't sounds correct to treat the bug as visibility style problem only because not visible element with not empty size having visible children can be created without visibility style like

<div style="width: 100px; height: 100px;>
  <button style="position: absolute; left: 200px; top: 200px">btn2</button>
</div>
<div style="width: 100px; height: 100px;>
  <button>btn1</button>
</div>

Here's the first div is invisible (hidden by second div) but its button is visible. That's demo case but we need to process it correct.

So at this point we should decide how we should handle invisible elements.

I get the point they're not needed for AT in most cases (expect some examples like in comment #7) and having invisible accessible in general might require additional processing on AT side what's perf issue.

But the rules might be tricky to distinguish the cases when AT needs an accessible for invisible node or not. And I have a feeling Firefox needs to be really smart on this way.

On the another hand we have technical problem here. When we're about to create an accessible then we don't know about its layout so we don't know whether it's visible or not.

Perhaps there's a way to move a11y notifications after layout happens (anyway we need to know when element becomes visible or hidden (not the case when its frame is created or destroyed) to fix bugs like bug 606207). But not technical issue doesn't get resolved by this.
If we'll stay on current approach (expose invisible accessibles) then 

* Show should be fired when an object becomes visible.
* Hide should be fired when an object goes invisible.
* No text change events: that's bad but otherwise it's complex to implement taking into account the case when invisible can have visible children (at least not in timeframe of Firefox 4)

* Show/hide shouldn't be fired on invisible objects.

that's correct if we start to fire create/destroy/reorder events instead of show/hide/reorder when accessible is created or destroyed. But we need to check with other AT vendors if they rely on these.
(In reply to comment #10)
> * Show should be fired when an object becomes visible.
> * Hide should be fired when an object goes invisible.
If we don't cover subtrees, that's going to be a huge amount of show/hides. What if a huge subtree is inserted? You'll have to fire shows for *every* node in that subtree.

> * No text change events: that's bad but otherwise it's complex to implement
> taking into account the case when invisible can have visible children (at least
> not in timeframe of Firefox 4)
We depend on text inserted/removed for our buffers. In fact, we don't use show/hide at all for buffers. Supporting show/hide is going to be fairly tricky for us.

> that's correct if we start to fire create/destroy/reorder events instead of
> show/hide/reorder when accessible is created or destroyed.
Reorder is the other event that we use to determine whether to re-render a subtree. This means we won't be able to use this either. Put simply, I'm not sure how we can make our code handle these changes at all without a fairly large rewrite.
(In reply to comment #7)
> Here's one (not related with visibility style though):
> <ul>
> <li style="display: block; float: left">list1
> <li style="display: block; float: left">list2
> </ul>
> here's UL is invisible since it has 0 height but it's important for AT I think.
I don't think it's so important for the AT. The author clearly intended the list not to be visible as a list. The list items get a role of paragraph anyway because they're floated outside the list. Again, if you don't see it as a sighted user, the AT user probably shouldn't see it either.

It's also worth noting that our simple review code assumes that all nodes below an invisible node are also invisible and thus it skips the invisible node as an optimisation. I guess we could override this for Gecko if we had to.
(In reply to comment #11)
> > * Show should be fired when an object becomes visible.
> > * Hide should be fired when an object goes invisible.
> If we don't cover subtrees, that's going to be a huge amount of show/hides.
To clarify, this would require that the rules be changed so that show and hide only affect a single object. This is probably pretty non-standard. This is necessary because if hide is fired on an object, it only means that object was hidden, not necessarily all of its children. Thus, if an entire subtree gets hidden, you have to fire hide on every object in that subtree. Similarly, say you have a visible list with 1000 invisible list items. If all of those list items become visible, you'll have to fire 1000 shows.

I guess you could document that you will only fire show/hide on the top object and the AT must then check all descendants to see whether they are visible. This is fairly unintuitive, though.

> We depend on text inserted/removed for our buffers. In fact, we don't use
> show/hide at all for buffers. Supporting show/hide is going to be fairly tricky
> for us.
We did think of a way to do this. We'd have to render all nodes (even invisible ones) and then re-render whenever a show/hide event is seen.

Perhaps your idea of an invisible state change event is better to avoid breaking existing uses of show/hide. The same as above applies re event flooding.

Note that rendering buffers will certainly become more expensive with this change, as we need to collect even invisible nodes so we can keep track of them. It could also cause major event flooding.
Ok, the best way for AT is to not expose accessibles for invisible elements at all but I think we have exceptions on this way, for example, bug 476986. If that bug is fixed the the following is possible scenario.

<p>
<div style="visibility: hidden" id="hidden">
  text1
  <span style="visibility: visible">text2</div>
</div>
</p>
<input aria-labelledby="hidden">

so you don't need to see a text1 while you read the paragraph but the accessible should be created for div and text1 since that div is referred by input.

(perhaps aria-hidden may bring some mess here as well)

Side note: we can't change semantics of show/hide events since they are used by existing AT.

It sounds like we can't drop invisible accessible from tree.
Boris, sorry to bother you but I need your help again.

Does nsIFrame::GetStyleVisibility()->IsVisible() always return correct result when a11y is notified from  nsCSSFrameConstructor::InsertContentRangeInserted/RemoveContent?

Where's the best place to notify a11y that visibility style of element is changed?

Is there a proper place to move a11y notifications from nsCSSFrameConstructor::InsertContentRangeInserted/RemoveContent so that notifications triggers when layout happens so that we can know whether element is visible or not (visibility styles, clipping, zero width/height).

Is it a bad idea to store notifications from InsertContentRangeInserted/RemoveContent and process them after layout?
blocking2.0: ? → final+
Assignee: nobody → surkov.alexander
> Does nsIFrame::GetStyleVisibility()->IsVisible() always return correct result

Correct in what sense?

> Where's the best place to notify a11y that visibility style of element is
> changed?

Probably DidSetStyleContext.... but note that this can lead to a flood of notifications.  We don't keep track of the "root" of a visibility change anywhere (esp. because the concept is not very meaningful).

> so that notifications triggers when layout happens

You can easily trigger notifications when layout is done.  Triggering them on every (temporary, even) size change during layout would be a performance nightmare.

> Is it a bad idea to store notifications from
> InsertContentRangeInserted/RemoveContent and process them after layout?

No, if you're suitably careful.
(In reply to comment #16)
> > Does nsIFrame::GetStyleVisibility()->IsVisible() always return correct result
> 
> Correct in what sense?

Easier to provide an example, to describe a sense. If I insert node as a child of node that has visibility: hidden style, ContentRangeInserted triggers for a child. Will IsVisible() return false in this case? If I append node without any visibility styles on ancestor chain then will it return true? Is this property about CSS visibility style only, right?

> > Where's the best place to notify a11y that visibility style of element is
> > changed?
> 
> Probably DidSetStyleContext.... but note that this can lead to a flood of
> notifications.  We don't keep track of the "root" of a visibility change
> anywhere (esp. because the concept is not very meaningful).

So if I set visibility style then all children in subtree are notified about this change, right?

> > so that notifications triggers when layout happens
> 
> You can easily trigger notifications when layout is done.  Triggering them on
> every (temporary, even) size change during layout would be a performance
> nightmare.

I'm not sure I understand this right. Is what we have (I mean notifications from nsCSSFrameConstructor::InsertContentRangeInserted/ContentRemoved) are performance nightmare now? How can I trigger these notifications when layout done? Do I understand right done layout means coordinates and size of frames are calculated at this point? 

> > Is it a bad idea to store notifications from
> > InsertContentRangeInserted/RemoveContent and process them after layout?
> 
> No, if you're suitably careful.

Is that a way to trigger these notifications after layout you keep in mind?
Blocks: 355521
> Will IsVisible() return false in this case?

It depends on the child's style.  If the child's computed visibility is hidden, then yes.

> If I append node without any visibility styles on ancestor chain then will it
> return true?

If "ancestor chain" includes the node itself, then yes.

> Is this property about CSS visibility style only, right?

Yes:

1256   PRBool IsVisible() const {
1257     return (mVisible == NS_STYLE_VISIBILITY_VISIBLE);
1258   }

> So if I set visibility style then all children in subtree are notified about
> this change, right?

Yes.

> Is what we have (I mean notifications from
> nsCSSFrameConstructor::InsertContentRangeInserted/ContentRemoved) are
> performance nightmare now? 

Hopefully, no.  ;)  You're sending one notification per frame subtree operation, which should not be too bad.

> How can I trigger these notifications when layout done?

I'm not sure what you're asking.  Maybe catch me on irc and we'll talk?

> Do I understand right done layout means coordinates and size of frames
> are calculated at this point? 

Yes.
Some observations based on talk with Boris and Jamie.

1) Get back old processing of visibility style, do not expose accessibles for hidden/collapsed visibility, fire show/hide/reorder events when visibility is changed (accessible is created or destroyed), if parent node gets hidden while child node stays visible then fire hide event for parent (reorder for grandparent), show for child.

2) Consider an idea to not create accessibles for invisible nodes. For this we need to gather ContentRangeInserted/ContentRemoved notifications and process them after layout. We should add a hook to trigger this processing as we do for accessibility events flushing. We need to take care about stale information in stored notifications. This will allow us to get updated information whether the node not visible or not. To keep the accessible tree updated we need to add notifications when node's frame gets visible or invisible (in means of visibility, size, overlapping with other frames). For this we need to wait a bug 539356.

3) While invisible accessible may exit in tree then we should fix text interface to not expose them. Gets obsolete with fixed 2nd item.

Based on that we'll keep this bug for 1) item. We need to file other one for 2) item.
(In reply to comment #19)

> 3) While invisible accessible may exit in tree then we should fix text
> interface to not expose them. Gets obsolete with fixed 2nd item.

Ah, if we're ever going to fix bug bug 476986 then 3d is not obsolete if 2nd is implemented.
(In reply to comment #16)

> > Where's the best place to notify a11y that visibility style of element is
> > changed?
> 
> Probably DidSetStyleContext.... but note that this can lead to a flood of
> notifications.  We don't keep track of the "root" of a visibility change
> anywhere (esp. because the concept is not very meaningful).

We'd need a root. Prior to bug 570275 we used to "get" a root in nsFrameManager::ReResolveStyleContext (http://hg.mozilla.org/mozilla-central/file/8f8d9eb61a5c/layout/base/nsFrameManager.cpp#l1411).

We need to follow the rules:

1) When subtree is shown then we should fire show event for root of subtree
1.2) if element in subtree stays hidden then we should fire hide event for it
2) When subtree is hidden then we should fire hide event for root of subtree
2.2) if element in subtree stays visible then we should fire show event for it

Boris, any ideas who to achieve this?
> we used to "get" a root 

At the expense of having bug 355521, right?  Which this seems to be a duplicate of, possibly...

> When subtree is shown

This concept doesn't necessarily apply well to CSS visibility changes.  What's shown is sometimes a subtree, but sometimes only a _subset_ of a subtree.  And it's possible to a node and some of its descendants to be shown (in the sense of going from hidden to visible visibility) while at the same time some of its other descendants are hidden (in the sense of going from visible to hidden visibility).

Does applying your rules cover this scenario?
(In reply to comment #22)
> > we used to "get" a root 
> 
> At the expense of having bug 355521, right?

It sounds visibility peculiarities weren't taken into account what resulted in bug 355521.

>  Which this seems to be a duplicate
> of, possibly...

it might be, "might be" because visibility isn't processed at all by a11y after bug 570275, but if this one is fixed then bug 355521 should be fixed as well.

> > When subtree is shown
> 
> This concept doesn't necessarily apply well to CSS visibility changes.  What's
> shown is sometimes a subtree, but sometimes only a _subset_ of a subtree.  And
> it's possible to a node and some of its descendants to be shown (in the sense
> of going from hidden to visible visibility) while at the same time some of its
> other descendants are hidden (in the sense of going from visible to hidden
> visibility).
> 
> Does applying your rules cover this scenario?

If the example would be like

<div style="visibility: hidden">
  <div style="visibility: hidden">
    <div style="visibility: visible"></div>
  </div>
</div>

if top div is shown then it sounds we should send reorder event to div parent and show event for div, i.e. follow rule 1 (actually I think 1.2 rule doesn't make lot of sense).

if some node and its descendents are hidden while some descendants stays visible then we need reorder on container, hide event for top hidden element and show events for every element that stays visible (except of visible children of visible elements), i.e. follow rule 2-2.1

If visibility properties are changed for two elements belonging to the same subtree like

<div style="visibility: hidden"> - make visible
  <div style="visibility: hidden">
    <div id="deepest" style="visibility: visible"></div> - make hidden
  </div>
</div>

then are they handled separately and DidSetStyleContext is called twice for "deepest" div (and its subtree if it was presented)?
> then are they handled separately and DidSetStyleContext is called twice for
> "deepest" div (and its subtree if it was presented)?

No.  On trunk, at this point, we would coalesce the style changes.  There would be exactly one DidSetStyleContext on each of the nodes in the tree there: the topmost element would go from hidden to visible, the middle one would not change, and the innermost one would go from visible to hidden.
So DidSetStyleContext won't be called for the middle, right?


<div style="visibility: hidden"> - make visible
  <div style="visibility: hidden">
    <div style="visibility: visible"></div>
  </div>
</div>

Is it possible to get notifications for top (gets hidden) and for deepest (stays visible)? I would like to avoid to traverse accessible tree to figure out that deepest div stays visible because layout is likely makes frame tree traversal already.
> So DidSetStyleContext won't be called for the middle, right?

It will.  If the style context for a node changes, the style contexts for all its descendants will change.

> Is it possible to get notifications for top (gets hidden) and for deepest
> (stays visible)?

You'd have to hack that into layout code somehow.... Right now we don't keep track of it, because all layout needs to know in this case is "repaint top and all its descendants".
(In reply to comment #26)

> You'd have to hack that into layout code somehow.... 

Any ideas?
One option may be is to keep track of whether the parent's visibility changed (and in which direction) in in ReResolveStyleContext and to do something if the visibility of the kid doesn't change the same way.

Would that give you what you want?
I think so. Though what does "and in which direction" mean?
visibility can change from visible to hidden, or from hidden to visible.  Just knowing that it changed is enough if you know either the before or after value; if you don't then you need to keep track of which direction it changed in.
Attached patch patch (obsolete) — — Splinter Review 
makes tree/test_applicationacc.xul fail, perhaps ReResolveStyleContext notifications makes us create document accessibles that we didn't create.
(In reply to comment #31)
> Created attachment 486646 [details] [diff] [review]
> patch
> 
> makes tree/test_applicationacc.xul fail, perhaps ReResolveStyleContext
> notifications makes us create document accessibles that we didn't create.

oops, that was disabled test
Attachment #486646 - Flags: superreview?(bzbarsky)
Attachment #486646 - Flags: review?(marco.zehe)
Attachment #486646 - Flags: review?(bolterbugz)
So the comment says we may have visible descendants, but another comment says we won't create accessibles for them?

>+    PRBool wasFrameVisible = oldContext->GetStyleVisibility()->IsVisible();

This should probably use PeekStyleVisibility() and assume not visible if there is no struct.  Not sure though.  David?

>+        notifyA11yFlag = eDontNotifyA11y;

Why?
(In reply to comment #33)
> So the comment says we may have visible descendants, but another comment says
> we won't create accessibles for them?

I must miss something. Which comment? If we have visible descendants then we need accessible for them.

if you meant "// We don't need to process notifications from shown subtree" then perhaps it's bad wording, I meant subtree excluding root.

> >+        notifyA11yFlag = eDontNotifyA11y;
> 
> Why?

it'll be fired in outer ReResolveStyleContext after hide event is fired for root.
Ah, I see, and descendants will just have all their accessibles torn down and then recreated as needed.  Alright.
So the notification flags aren't really a bitfield, right?

The child provider bit looks wrong to me; doesn't that lose all notifications inside tables?
(In reply to comment #37)
> So the notification flags aren't really a bitfield, right?

yes, they aren't. I don't really like them but didn't come with better idea.

> The child provider bit looks wrong to me; doesn't that lose all notifications
> inside tables?

I copied/pasted this from patch of bug 452564. I'm not sure whether this is correct. What would scenario be?
> What would scenario be?

  <div><table><tr><td><div style="visibility: hidden">text</div></td></tr></table>

Then change the color of the outer div and change the visibility of the inner div to visible.
How would affect this on visibility of child provider? Who is child provider in this case, is it inner div and ReResolveStyleContext is called on outer div?
The child provider is the nsTableFrame.  The thing it's a child of is an nsTableOuterFrame.  The frame structure is a block containing an nsTableOuterFrame containing an nsTableFrame containing all sorts of stuff.

ReResolveStyleContext will be entered for the outermost block frame.  This will call ReResolveStyleContext on its kids, which includes the nsTableOuterFrame.  This will call ReResolveStyleContext on the nsTableFrame and pass eDontNotifyA11y for aNotifyA11yFlag.  That means no a11y notifications on anything in the subtree rooted at the nsTableFrame, and in particular no notification for the inner div.

Did just doing what I propose in comment 39 not show the problem?
Attached patch patch2 (obsolete) — — Splinter Review 
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-bc6142cd2fa9
Attachment #486646 - Attachment is obsolete: true
Attachment #487302 - Flags: superreview?(bzbarsky)
Attachment #487302 - Flags: review?(marco.zehe)
Attachment #487302 - Flags: review?(bolterbugz)
Attachment #486646 - Flags: superreview?(bzbarsky)
Attachment #486646 - Flags: review?(marco.zehe)
Attachment #486646 - Flags: review?(bolterbugz)
Initial feedback: This try-server build properly handles the mouse-over style changes in Twitter. NVDA updates correctly when I mouse over any part of the tweet, without the need to refresh.
Also, the visibility of the favorite, reply and retweet links is once again properly handled, e.g. it only appears for the tweet in the vritual buffer that currently has the mouse hovering over it.
neu.de seems to have relaunched, and the site looks totally different now. Guess I can't really test that scenario any more.
Comment on attachment 487302 [details] [diff] [review]
patch2

r=me for the tests. Very thorough! Did you collect some of these constructs from real-life examples? I also looked over the code briefly and didn't find anything glaring obvious for my little knowledge. ;)
Attachment #487302 - Flags: review?(marco.zehe) → review+
(In reply to comment #44)
> Comment on attachment 487302 [details] [diff] [review]
> patch2
> 
> r=me for the tests. Very thorough! Did you collect some of these constructs
> from real-life examples? 

No, they come from how ReResolveStyleContext work.
Attached patch patch3 (obsolete) — — Splinter Review 
fixing a comments
Attachment #487302 - Attachment is obsolete: true
Attachment #487514 - Flags: superreview?(bzbarsky)
Attachment #487514 - Flags: review?(bolterbugz)
Attachment #487302 - Flags: superreview?(bzbarsky)
Attachment #487302 - Flags: review?(bolterbugz)
Comment on attachment 487514 [details] [diff] [review]
patch3

>+  // Check frame and its visibility. Note, hidden frame allows visible
>+  // elements in subtree.
>+  if (!weakFrame.GetFrame() || !weakFrame->GetStyleVisibility()->IsVisible()) {
>+    if (aIsSubtreeHidden && !weakFrame.GetFrame())
>+      *aIsSubtreeHidden = true;


Just curious, can a call to IsVisible cause the frame to go bad? (I seems to recall 'yes')




> nsresult
> nsTextEquivUtils::AppendFromAccessible(nsAccessible *aAccessible,
>                                        nsAString *aString)
> {
>-  // Ignore hidden accessible for name computation.
>-  nsIFrame* frame = aAccessible->GetFrame();
>-  if (!frame || !frame->GetStyleVisibility()->IsVisible())
>-    return NS_OK;
>-

What is the effect of this change?
(In reply to comment #47)

> Just curious, can a call to IsVisible cause the frame to go bad? (I seems to
> recall 'yes')

I'm not sure I understand well what's bad frame. Let's get an answer from Boris on this.

> What is the effect of this change?

no effect, it's backout code from bug 570275 where visibility styles were out of law.
> Just curious, can a call to IsVisible cause the frame to go bad?

No, absolutely totally not.  ;)
Attachment #487514 - Flags: review?(bolterbugz) → review+
(In reply to comment #49)
> > Just curious, can a call to IsVisible cause the frame to go bad?
> 
> No, absolutely totally not.  ;)

Thanks :)
Comment on attachment 487514 [details] [diff] [review]
patch3

>+++ b/layout/base/nsFrameManager.cpp
>@@ -995,17 +999,19 @@ CaptureChange(nsStyleContext* aOldContex
>+                                      ProcessA11YFlags       aProcessA11yFlag,

I would prefer it if we named the argument aDesiredAccessibilityNotifications and the enum something like DesiredAccessibilityNotifications.

>+                                      nsTArray<nsIContent*>& aVisibleContents)

aVisibleKidsOfHiddenElement, perhaps?  That's not great either... but more on this below.

>@@ -1042,16 +1048,21 @@ nsFrameManager::ReResolveStyleContext(ns
>+    PRBool wasFrameVisible = oldContext->GetStyleVisibility()->IsVisible();

This is only used if mPresShell->IsAccessibilityActive(), right?  Please condition it on that.

Alternately, condition on aAccessibilityNotificationType and set the type to eSkipNotifications on entry if !IsAccessibilityActive().

>@@ -1393,16 +1406,49 @@ nsFrameManager::ReResolveStyleContext(ns
>+    NotifyA11yFlags notifyA11yFlag = eDontNotify;

Call that enum AccessibilityNotificationType.  And make it clear that this variable is the notifications we'll send about _ourselves_.  Perhaps |ourNotifications|?

>+    ProcessA11YFlags processA11yFlag = aProcessA11yFlag;

The variable name should be clearer: this is the desired notification we'll ask for from our _kids_.

>+    if (mPresShell->IsAccessibilityActive() && !aFrame->GetPrevContinuation()) {

This whole block should be conditioned on aMinChange not having nsChangeHint_ReconstructFrame, no?

Also, for block-inside-inline splits, do you want one notification per part, or just one for the first chunk of the split?

>+      if (aProcessA11yFlag == eProcessAll) {

I'd prefer we name that eSendAllNotifications.

>+            // Notify a11y the element (perhaps within its children) was shown.

s/within/with/ ?

>+            // while its parent is hidden.

You mean "becomes hidden"?

>+            processA11yFlag = eDontProcess;

I'd call that eSkipNotifications.

>+            // The element gets hidden while its children may get or stay
>+            // visible.

  The element is being hidden; its children may stay visible, or become visible
  after being hidden previously.

>+            // a11y about that as they were inserted into tree. Notify a11y

"as if they were inserted into the tree".

>+            processA11yFlag = eProcessIfShown;

I'd call that eNotifyIfShown.

>+        // Notify a11y that element stay visible while its parent gets hidden.

"stayed visible while its parent was hidden".

>+        aVisibleContents.AppendElement(aFrame->GetContent());

Perhaps aVisibleContents should be called aStillVisibleDescendants?

>+    // Notify a11y the elements changed their visibility.

"Send notifications about visibility changes".

>+    if (notifyA11yFlag == eNotifyOfShow) {

I'd call that eNotifyShown.

>+    } else if (notifyA11yFlag == eNotifyOfHide) {

eNotifyHidden.

Note that when you hits this code there might be chunks of the frame tree that have style information (because they got reframe hints, and we plan to call ContentRemoved and ContentRangeInserted on them).  Can the accessibility service deal ok with that during the notifications you're dispatching?

>+          accService->ContentRangeInserted(aFrame->PresContext()->GetPresShell(),

Just pass |presShell| there?

>+        aVisibleContents.Clear();

So the key here is that aStillVisibleDescendants is only processed/cleared when dispatching a hide on a root of a visibility change, and that we collect all descendants of this root that stay visible but have nothing visible between them and the root in aStillVisibleDescendants?  This should be documented somewhere; this pattern looks very fishy until you figure out what's going on here.

>@@ -1499,22 +1585,25 @@ nsFrameManager::ComputeStyleChangeFor(ns
>   // reresolving style on all the frames we encounter in this walk.
> 
>   FramePropertyTable *propTable = GetPresContext()->PropertyTable();
> 
>   do {
>     // Outer loop over special siblings
>     do {
>       // Inner loop over next-in-flows of the current frame
>+      nsTArray<nsIContent*> visibleContents;

Is there a reason to not put this outside all the loops, so we don't keep creating/destroying arrays when we only need one?

>+++ b/layout/base/nsFrameManager.h
>+  enum ProcessA11YFlags {
>+    eDontProcess = 0x00,
>+    eProcessAll = 0x01,
>+    eProcessIfShown = 0x02
>+  };
>+
>+  enum NotifyA11yFlags {
>+    eDontNotify= 0x00,
>+    eNotifyOfShow = 0x01,
>+    eNotifyOfHide = 0x02

These end up looking like bitflags, when they're not.  Just don't assign numbers to them at all?

r=me with those nits fixed, I think.
Attachment #487514 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #51)

> >+    if (mPresShell->IsAccessibilityActive() && !aFrame->GetPrevContinuation()) {
> 
> This whole block should be conditioned on aMinChange not having
> nsChangeHint_ReconstructFrame, no?

If aMinChange contains nsChangeHint_ReconstructFrame then we should be notified from nsCSSFrameConstruction::ContentRangeInserted/ContentRemoved? Otherwise what is this check about?

> Also, for block-inside-inline splits, do you want one notification per part, or
> just one for the first chunk of the split?

Could you give me an example please?
> then we should be notified from 
> nsCSSFrameConstruction::ContentRangeInserted/ContentRemoved?

You will be sometime in the future, yes.

> Could you give me an example please?

<div>
  <span>
    This
    <span style="display: block">is some</span>
    text
  </span>
<div>

The frame tree here looks like this:

  BlockFrame(div)
    InlineFrame(outer span)
      TextFrame("This")
    BlockFrame(anonymous block, mContent is outer span)
      BlockFrame(inner span)
        TextFrame("is some")
    InlineFrame(outer span)
      TextFrame("text")

where the "inner span" blockframe inherits style from the "outer span" inline frame.  So does the "anonymous block" blockframe.  If you change visibility on the outer span, there will be three "root" frames changing visibility.  But presumably you only need/want one notification, since those are content-based?  (Will three notifications be just slower than they should be, or actually lead to bugs?)

This is similar to continuation frames, which you do check for in the patch, but requires a different check in addition to the prev-continuation check.
(In reply to comment #51)

> >+    } else if (notifyA11yFlag == eNotifyOfHide) {
> 
> eNotifyHidden.
> 
> Note that when you hits this code there might be chunks of the frame tree that
> have style information (because they got reframe hints, and we plan to call
> ContentRemoved and ContentRangeInserted on them).  Can the accessibility
> service deal ok with that during the notifications you're dispatching?

It should be ok, at least I don't spot anything evident. It should result in excess processing on a11y side. But can you think of example I could try?

> >+        aVisibleContents.Clear();
> 
> So the key here is that aStillVisibleDescendants is only processed/cleared when
> dispatching a hide on a root of a visibility change, and that we collect all
> descendants of this root that stay visible but have nothing visible between
> them

this part is correct

> and the root in aStillVisibleDescendants? 

here's I miss something, why should the root be in aStillVisibleDescendants?

> Is there a reason to not put this outside all the loops, so we don't keep
> creating/destroying arrays when we only need one?

I think I can move it outside loops.
> It should be ok, at least I don't spot anything evident.

Wow.  My comment didn't say what I wanted to say.  What I was _trying_ to say is that they have _stale_ style information, because we don't give frames that are about to die the new style context.  It used to be that even asking them for style information was broken, but I think we fixed that since...

In any case, the example is just that you have a visibility change on a node and a display change on some descendant of the same node.

> here's I miss something, why should the root be in aStillVisibleDescendants?

It shouldn't.  Let me try to rewrite that sentence a bit to make the antecedent of the preposition clear....

  For a given node R (the visibility change root) that is changing from visible
  to hidden and has no ancestors changing visibility, aStillVisibleDescendants
  is an array containing all nodes that have R as an ancestor, are staying
  visible, and have no ancestor between them and R that is staying visible.
(In reply to comment #53)
> If you change visibility on
> the outer span, there will be three "root" frames changing visibility.  But
> presumably you only need/want one notification, since those are content-based? 
> (Will three notifications be just slower than they should be, or actually lead
> to bugs?)

So, we'll get notifications from InlineFrame(outer span), BlockFrame(anonymous block) and InlineFrame(outer span), i.e. three times for outer span, correct? If so then it's ok, we'll be just slower: recache child accessibles for text nodes and for child span three times in the case of show, destroy them once in the case of hide. We'll fire right events for AT because of event coalescence.

> This is similar to continuation frames, which you do check for in the patch,
> but requires a different check in addition to the prev-continuation check.

If we can handle this then it'll be nice.

(In reply to comment #55)
> > It should be ok, at least I don't spot anything evident.
> 
> Wow.  My comment didn't say what I wanted to say.  What I was _trying_ to say
> is that they have _stale_ style information, because we don't give frames that
> are about to die the new style context.  It used to be that even asking them
> for style information was broken, but I think we fixed that since...

> In any case, the example is just that you have a visibility change on a node
> and a display change on some descendant of the same node.

We should be ok. I can't think of how this example (or its variations) might break a11y.

>   For a given node R (the visibility change root) that is changing from visible
>   to hidden and has no ancestors changing visibility, aStillVisibleDescendants
>   is an array containing all nodes that have R as an ancestor, are staying
>   visible, and have no ancestor between them and R that is staying visible.

correct

>   For a given node R (the visibility change root) that is changing from visible
>   to hidden and has no ancestors changing visibility, 

is it possible to have a visibility change root and have its ancestors changing visibility?
> i.e. three times for outer span, correct?

In this simple markup, yes.  If there were more blocks in there, you get more notifications; worst-case is 2n+1 notifications where n is number of block kids of the inline.

> If we can handle this then it'll be nice.

Sure.  Just use nsLayoutUtils::FrameIsNonFirstInIBSplit.

> is it possible to have a visibility change root and have its ancestors
> changing visibility?

No; the ancestors bit is just the definition of "visibility change root".
Attached patch patch4 — — Splinter Review 
bz's comments are addressed
Attachment #487514 - Attachment is obsolete: true
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/e030f7ac043b
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
No longer depends on: 591363
I pushed a followup to fix the fact that this detached a comment from the code it was commenting.
https://hg.mozilla.org/integration/mozilla-inbound/rev/80ad04db9f3c
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: