Closed Bug 591363 Opened 14 years ago Closed 13 years ago

(in)visible state is not always correct?

Categories

(Core :: Disability Access APIs, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla12

People

(Reporter: davidb, Assigned: davidb)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

This testcase contains a paragraph inside a div, however the pargraph has been absolute positioned 200 pixels in from the left and 200 pixels down from the top (i.e. the paragraph is nolonger visuall
202 bytes, text/html
Details
patch 3
9.67 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
Spin off from Bug 581096 comment 9. The NVDA dudes are using height and width 0 instead of invisible state for some reason.
Jamie, can you comment in this bug? I wanted to know why you guys don't trust the STATE_SYSTEM_INVISIBLE if you can recall why...
I can't remember specifics - I'm not even sure if we ever figured out exactly what was causing it - and we unfortunately don't have any test cases for this. It shows up in some obscure places, so I'm reluctant to start trusting this again now and then discover regressions later. That said, I believe it had something to do with floating a parent element using css, even though that node had visible children. When it happens, the problematic node has the invisible state but still contains visible children. Mick, do you remember anything more about this?
The attachment I just added contains a paragraph inside a div, however the paragraph has been absolutely positioned 200 pixels in from the left and 200 pixels down from the top thus, the paragraph is no longer visually inside the div. Looking at this testcase in Firefox 3.6.10, although the accessibility hierarchi shows the paragraph inside the div (as that is what the DOM really is), the Accessible for the div has both the invisible and offscreen states. No doubt this is because the div now visually contains nothing as the paragraph has been positioned outside of it. However, this is in deed the reason why NVDA can not rely on the invisible state, as if we did pay attention to it, we would never traverse the div and therefore the paragraph would never be rendered in our virtualBuffer.
The same is true for Firefox 4.0b7pre. The div is also reported as being off screen and invisible.
Further to my previous comment: Some more places we see the invisible state used in Gecko are: If the accessible is in a background tab (i.e. you switch tabs away from the current tab) that that accessible will now have the invisible state. Also, if a node is empty (e.g. a div with no children at all, not even a text node) it will also have the invisible state. So to summarize where we see the invisible state (note offscreen seems to also be used in these places): *The accessible is in a background tab *The accesible is some form of container (e.g. div) and has no children *The children of the accessible have been positioned visually outside the bounds of the accessible. Note that styles such as visibility:hidden, and display: none, cause no accessible to exist at all, as opposed to one with the invisible state. So, reasons why NVDA does not use the invisible state are: *it would never render content in its virtualBuffer for a node that was positioned outside the bounds of its parent. E.g. a paragraph in a div where the paragraph is positioned outside the bounds of the parent would never bbe seen in the virtualBuffer. *For pages with dynamic content, information could start to go missing if the user switches away from the current tab and the page changes in some way. The reason for this is because NVDA would see the event representing the change, it would remove that subtree, it would then try and re-render that subtree, but the root node in the subtree has the invisible state and therefore it would not try rendering any more of that subtree. So, what NVDA currently does (mainly to get around the missing info in background tabs problem) is: if the node has children, or if the nodes width and height are greater than 0, it will render the node in its virtualBuffer. Finally then: If NVDA was required to no longer render nodes with the invisible state (for instance it was conveying aria-hidden), then the most useful thing to do would probably be one of the following: *Change Gecko to stop setting the invisible state on nodes in background tabs, or *change Gecko so that it never fired events in background tabs, but instead collected them up and fired them all when the tab became active again, or *Change NVDA to completely re-render a document when its tab became active (would be slow and unpopular with users I'm sure). However NVDA would still not render content that was positioned visually outside its parent, so to get around this, I guess Gecko would also have to stop exposing invisible there as well.
Thanks very much Mick!
Assignee: nobody → bolterbugz
In the case of accessible in background tab what state should it have?
ATK: visible means potentially visible (i.e. user may make it visible, no script), showing means visible, visible and showing states may be exposed on the same accessible (for the reference (http://comments.gmane.org/gmane.comp.gnome.accessibility.devel/2449) MSAA: invisible means not visible on the screen (in any case), offscreen means it's not visible right now but the user can make it visible (for the reference http://comments.gmane.org/gmane.comp.gnome.accessibility.devel/2450, invisible) So based on that: 1) visible accessible 1.a) MSAA: no invisible & no offscren 1.b) ATK: visible & showing 2) potentially visible accessible (scrolled off, in background tab) 2.a) MSAA: invisible & offscreen 2.b) ATK: visible & no showing 3) not visible accessible 3.a) MSAA: invisible & no offscreen 3.b) ATK: no visible & no showing Does it sound valid?
(In reply to comment #9) > MSAA: invisible means not visible on the screen (in any case), offscreen means > it's not visible right now but the user can make it visible (for the reference > http://comments.gmane.org/gmane.comp.gnome.accessibility.devel/2450, invisible) the url should be http://blogs.msdn.com/b/vsaccessibility/archive/2004/09/20/232157.aspx
Right that's how I understood those specs. I'm not confident they are followed in practice.
(In reply to comment #11) > Right that's how I understood those specs. I'm not confident they are followed > in practice. We don't as this bug states. I wonder if the spec defined behavior (or this treatment) works for NVDA
(In reply to comment #9) > MSAA: invisible means not visible on the screen (in any case), offscreen means > it's not visible right now but the user can make it visible ... > 2) potentially visible accessible (scrolled off, in background tab) > 2.a) MSAA: invisible & offscreen Hmm. I disagree with the spec here; it seems silly. To me, invisible means that it's programmatically invisible so the user can't access it, whereas offscreen just means it's not available on the screen at that moment in time, which would include objects in background tabs. It seems silly to set both. The spec doesn't even seem to be very clear on explaining this. I'd be inclined to violate the spec here <gasp> unless another AT vendor has a major objection. Having said that, if Firefox does expose both, I guess we can just check for both states and ignore invisible if offscreen is set. Ug. (In reply to comment #12) > I wonder if the spec defined behavior (or this > treatment) works for NVDA Not with current versions of NVDA. Any change will require change on our side as well.
Ok, I gather info about expectations from other ATs. Personally I'm fine with either solution.
(In reply to comment #14) > Ok, I gather info about expectations from other ATs. Personally I'm fine with > either solution. Anyway in Gecko absence of invisible state and presence of offscreen state is possible, so AT can deal with that. The algorithm should be: 1) visible accessible 1.a) MSAA: no invisible & no offscren 1.b) ATK: visible & showing 2) potentially visible accessible (scrolled off, in background tab) 2.a) MSAA: no invisible & offscreen 2.b) ATK: visible & no showing 3) not visible accessible 3.a) MSAA: invisible & no offscreen 3.b) ATK: no visible & no showing That's mostly how things are handled in Gecko now (expect some inconsistencies).
Reassigning to Alexander since he is active on this one and will be touching this part of the code anyway.
Assignee: bolterbugz → surkov.alexander
There is now another reason that we need ATs to trust the invisible state. Bug 570275 causes accessibles to be created for visibility: hidden/visibility: collapse, where they previously weren't (see bug 570275 comment #4 and bug 570275 comment #117). These don't have a width and height of 0, so NVDA renders them when it shouldn't. Examples are Twitter (see bug 570275 comment #114) and Facebook. If the author intends these to be invisible, they should also not be visible to AT users. Unfortunately, this is going to break backwards compatibility and will require ATs to check the Gecko version to know whether they can trust the invisible state. That said, bug 581096 will probably require that anyway. Ug!
needs blocking status since invisible state is important state for AT, especially in the light of bug 570275.
blocking2.0: --- → ?
I'm taking this bug back, as per IRC. I forgot I had a WIP started.
Assignee: surkov.alexander → bolterbugz
blocking2.0: ? → final+
(In reply to comment #12) > (In reply to comment #11) > > Right that's how I understood those specs. I'm not confident they are followed > > in practice. > > We don't as this bug states. Note I meant "in practice" outside our implementation (by other a11y servers).
Attached patch WIP (needs more test coverage) (obsolete) — — Splinter Review
Needs more tests and needs investigation into how to optimize a guess as to whether there might be a visible descendant in a containing element that has no area (which is the case in Mick's example). The guess will have to err on the side of visibility. As is, this WIP errs on the side of visibility. I need to get a better idea of the impact though.
Try build: http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/dbolter@mozilla.com-8ba017eb4bdd/tryserver-win32/ Code forensics on the code I removed here is difficult. Marco can you give this a spin and see if you notice anything like extra accessibles or slowness?
No longer blocks: 606125
blocking2.0: final+ → .x
status2.0: --- → wanted
I gave this build a good go a while ago and didn't find anything wrong with it.
Whiteboard: [has patch]
blocking2.0: .x+ → ---
status2.0: wanted → ---
Whiteboard: [has patch] → [FF5/6]
Attached patch patch (obsolete) — — Splinter Review
This makes the accompanying test pass. The code that I remove/change was simply wrong as far as I can tell. I've tried various things after discussion with dbaron and ehsan, but current layout API doesn't have what we need. I think this patch is not perfect but takes us to a better place. Perhaps we should file a bug to add more visibility tests (good first bug)?
Attachment #488546 - Attachment is obsolete: true
Attachment #576985 - Flags: review?(surkov.alexander)
Wouldn't the following do a trick? if (IsVisibleConsideringAncestors()) { nsRectVisibility rectVisibility = shell->GetRectVisibility(); if (rectVisibility != nsRectVisibility_kVisible) { // make sure special cases below are exposed as invisible if (frame->GetRect()->IsEmpty()) { // do special processing } } } that should be equivalent what you do but that keeps offscreen state for those special cases, I'm not sure if it's desired.
(In reply to David Bolter [:davidb] from comment #24) > Perhaps we should file a bug to add more visibility tests (good first bug)? I would say this. Assignee should be qualified enough to figure out all cases we want to test so it doesn't look like good first bug until you list all cases. On the another hand I want to see tests here to be sure your patch does correct things (I can't 100% say that layout stuffs produce expected result without debugging).
Also note tests here can be fragile (if the harness content scrolls for example).
Hmmm and I think getting rid of the VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY flag might address Mick's concern about background tabs.
(In reply to David Bolter [:davidb] from comment #28) > Hmmm and I think getting rid of the VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY > flag might address Mick's concern about background tabs. that's a whole point of offscreen state exposing. But surely my code snippet from comment #25 doesn't take that into account.
After further IRCing, I'm looking at walking the accessible parent chain. Sort of our own version of IsVisibleConsideringAncestors. We'll probably halt at tab-panel or similar.
Whiteboard: [FF5/6]
Attached patch WIP (walk a8e parent chain) (obsolete) — — Splinter Review
Latest WIP. Walk the accessible parent chain as per IRC chat. I'll post a try build link. Note this patch removes the questionable empty text and inline edge case checks, allowing those cases to be visible, because Voltaire wouldn't mind.
Attachment #576985 - Attachment is obsolete: true
Attachment #576985 - Flags: review?(surkov.alexander)
Comment on attachment 577345 [details] [diff] [review] WIP (walk a8e parent chain) f=me. I get correct updates for content and also no longer see the invisible state on the div in Mick's testcase. Note that this will make testing bug 706335 even harder since the removed text, with this patch, now not even gets the invisible state any more. One nit: Please indent the body of the doTest function by two spaces. And, where do you define the children array the testTree function should test? Or is this part not finished yet?
Attachment #577345 - Flags: feedback?(marco.zehe) → feedback+
Attachment #577345 - Attachment is obsolete: true
Attachment #578325 - Flags: review?(surkov.alexander)
Attachment #578325 - Flags: review?(surkov.alexander)
Attachment #578325 - Attachment is obsolete: true
Attachment #578327 - Flags: review?(surkov.alexander)
Comment on attachment 578327 [details] [diff] [review] patch (walk accessible parent chain, restore textframe check) (In reply to Marco Zehe (:MarcoZ) from comment #33) > One nit: Please indent the body of the doTest function by two spaces. Done. > And, > where do you define the children array the testTree function should test? Or > is this part not finished yet? I pass the object with empty child array right in.
Attachment #578327 - Flags: review?(marco.zehe)
Comment on attachment 578327 [details] [diff] [review] patch (walk accessible parent chain, restore textframe check) r=me for the test bit.
Attachment #578327 - Flags: review?(marco.zehe) → review+
Comment on attachment 578327 [details] [diff] [review] patch (walk accessible parent chain, restore textframe check) I'd like to get Trevor for review before me. It must be interesting patch.
Attachment #578327 - Flags: review?(trev.saunders)
Comment on attachment 578327 [details] [diff] [review] patch (walk accessible parent chain, restore textframe check) > bool > nsAccessible::IsVisible(bool* aIsOffscreen) Consider PRUint64 DetermineVisibilityStates()? > *aIsOffscreen = true; > if (IsDefunct()) > return false; not needed >+ nsWeakFrame weakFrame = GetFrame(); why don't you just use a nsIFrame*? I don't think anything you call should change the frame tree. >+ nsAccessible* accessible = this; >+ while (accessible) { >+ if (accessible->IsDefunct()) >+ return false; that shouldn't be needed. >+ if (accessible->Role() == nsIAccessibleRole::ROLE_PROPERTYPAGE || >+ accessible->Role() == nsIAccessibleRole::ROLE_PANE) { >+ break; ?! >+ accessible = accessible->Parent(); >+ } maybe do { ... } while (accessible = accessible->Parent()); ? >+ const nsCOMPtr<nsIPresShell> shell(GetPresShell()); > if (!shell) > return false; it seems like this should never be the case because ::Shutdown() sets mWeakShell and mContent to NULL so IsDefunct() sees the accessible as defunct, and we know that the accessible here is not defunct.
Thanks Trevor. (In reply to Trevor Saunders (:tbsaunde) from comment #41) > Comment on attachment 578327 [details] [diff] [review] [diff] [details] [review] > patch (walk accessible parent chain, restore textframe check) > > > > bool > > nsAccessible::IsVisible(bool* aIsOffscreen) > > Consider PRUint64 DetermineVisibilityStates()? Hmm maybe, but there are only two states (and what I'm hearing so far from AT is they don't use the offscreen state). > > > *aIsOffscreen = true; > > if (IsDefunct()) > > return false; > > not needed Ok with me but this was existing code. Alexander, promise you are not going to ask me to put those defunct checks back in? :) > > >+ nsWeakFrame weakFrame = GetFrame(); > > why don't you just use a nsIFrame*? I don't think anything you call should > change the frame tree. The key phrase here is "don't think" (smile). The general rule is to not hold onto a raw frame pointer if there is any chance now (or in the reasonable future) of the executing code causing a frame to go bad, or in other words, if we are not immediately using the frame pointer. This has bitten us before with really bad timing. My worry is that here we are looping and asking for state (and possibly more things in the future). If there is some reflow that might happen by us asking for this (on demand, and since reflow was possibly interrupted), now or in the future, we can get bitten. The fact the biting doesn't have to happen deteriministically makes me especially cautious. My inclination is to use the weak frame outside the loop. > >+ nsAccessible* accessible = this; > >+ while (accessible) { > >+ if (accessible->IsDefunct()) > >+ return false; > > that shouldn't be needed. OK with me. > >+ if (accessible->Role() == nsIAccessibleRole::ROLE_PROPERTYPAGE || > >+ accessible->Role() == nsIAccessibleRole::ROLE_PANE) { > >+ break; > > ?! NVDA doesn't want us to return offscreen or invisible due to a tab not being in the foreground. It messes with their cache and makes them not want to use the visibility state, which causes us pain. So bottom line, if we get here we can bail. Note: this is also why I removed the VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY parameter in the rect bounds call since we won't need that. > > >+ accessible = accessible->Parent(); > >+ } > > maybe do { ... } while (accessible = accessible->Parent()); ? Sounds good thanks. > >+ const nsCOMPtr<nsIPresShell> shell(GetPresShell()); > > if (!shell) > > return false; > > it seems like this should never be the case because ::Shutdown() sets > mWeakShell and mContent to NULL so IsDefunct() sees the accessible as > defunct, and we know that the accessible here is not defunct. That's fair. We should catch crashes pretty quick if this situation ever changes.
(In reply to David Bolter [:davidb] from comment #42) > Thanks Trevor. > > (In reply to Trevor Saunders (:tbsaunde) from comment #41) > > > bool > > > nsAccessible::IsVisible(bool* aIsOffscreen) > > > > Consider PRUint64 DetermineVisibilityStates()? > > Hmm maybe, but there are only two states If we can't keep computation of invisible and offscreen states separately then I think I prefer one int over two bools. States as return value suite well here. But I would like to name the methods as VisibilityState() to keep it closer to States() (I'm not sure about Native prefix though). > (and what I'm hearing so far from > AT is they don't use the offscreen state). I didn't catch how it's related with method name. Do you want just get rid offscreen state? If yes then it's a topic for openAT group ;) > > > > > *aIsOffscreen = true; > > > if (IsDefunct()) > > > return false; > > > > not needed > > Ok with me but this was existing code. > > Alexander, promise you are not going to ask me to put those defunct checks > back in? :) This method is supposed for internal usage so we don't need that. I think you can trust Trevor in questions like this :) > > > > >+ nsWeakFrame weakFrame = GetFrame(); > > > > why don't you just use a nsIFrame*? I don't think anything you call should > > change the frame tree. > > The key phrase here is "don't think" (smile). Yeah, Trevor should have asked "why do you think anything you call should change the frame tree" :) > The general rule is to not > hold onto a raw frame pointer if there is any chance now (or in the > reasonable future) of the executing code causing a frame to go bad, or in > other words, if we are not immediately using the frame pointer. agree, key phrase "any chance" > This has > bitten us before with really bad timing. You must say that was an age of tree creation at random timing. > My worry is that here we are > looping and asking for state (and possibly more things in the future). If > there is some reflow that might happen by us asking for this (on demand, and > since reflow was possibly interrupted), now or in the future, we can get > bitten. iirc the asking for visibility styles don't trigger reflow. But yes can you check this with bz please? In general states calculation triggering reflow could bring more problems than crash. So we need to know why it can happen. > The fact the biting doesn't have to happen deteriministically makes > me especially cautious. deteriministically means reproducible or analyzable here?
(In reply to alexander :surkov from comment #43) > (In reply to David Bolter [:davidb] from comment #42) > > Thanks Trevor. > > > > (In reply to Trevor Saunders (:tbsaunde) from comment #41) > > > > bool > > > > nsAccessible::IsVisible(bool* aIsOffscreen) > > > > > > Consider PRUint64 DetermineVisibilityStates()? > > > > Hmm maybe, but there are only two states > > If we can't keep computation of invisible and offscreen states separately > then I think I prefer one int over two bools. States as return value suite > well here. But I would like to name the methods as VisibilityState() to keep > it closer to States() (I'm not sure about Native prefix though). that's fine by me. > > > >+ nsWeakFrame weakFrame = GetFrame(); > > > > > > why don't you just use a nsIFrame*? I don't think anything you call should > > > change the frame tree. > > > > The key phrase here is "don't think" (smile). > > Yeah, Trevor should have asked "why do you think anything you call should > change the frame tree" :) yeah, I that makes sense, I wish it wasn't the case but I can certainly live with it. > > My worry is that here we are > > looping and asking for state (and possibly more things in the future). If > > there is some reflow that might happen by us asking for this (on demand, and > > since reflow was possibly interrupted), now or in the future, we can get > > bitten. > > iirc the asking for visibility styles don't trigger reflow. But yes can you > check this with bz please? In general states calculation triggering reflow > could bring more problems than crash. So we need to know why it can happen. agree > > >+ if (accessible->Role() == nsIAccessibleRole::ROLE_PROPERTYPAGE || > > >+ accessible->Role() == nsIAccessibleRole::ROLE_PANE) { > > >+ break; > > > > ?! > > NVDA doesn't want us to return offscreen or invisible due to a tab not being > in the foreground. It messes with their cache and makes them not want to use > the visibility state, which causes us pain. So bottom line, if we get here that seems like a weird definition of "offscreen" but ok. I was objecting to calling Role() twice on the same accessible.
(In reply to alexander :surkov from comment #43) > (In reply to David Bolter [:davidb] from comment #42) > > Thanks Trevor. > > > > (In reply to Trevor Saunders (:tbsaunde) from comment #41) > > > > bool > > > > nsAccessible::IsVisible(bool* aIsOffscreen) > > > > > > Consider PRUint64 DetermineVisibilityStates()? > > > > Hmm maybe, but there are only two states > > If we can't keep computation of invisible and offscreen states separately > then I think I prefer one int over two bools. States as return value suite > well here. But I would like to name the methods as VisibilityState() to keep > it closer to States() (I'm not sure about Native prefix though). It seems to be a pattern that we are dropping the verbs from our method names. Is this intended? I'm a little unsure of how the code will read as this happens more and more... won't it be difficult for new contributors to tell the difference between method calls, constructors, and casts without having to pause and think? I'm not going to fight hard on the method name but I do worry about this pattern overall. > > (and what I'm hearing so far from > > AT is they don't use the offscreen state). > > I didn't catch how it's related with method name. Do you want just get rid > offscreen state? If yes then it's a topic for openAT group ;) Yes, I'm thinking we might get rid of it after discussion (not on this bug). > > > > *aIsOffscreen = true; > > > > if (IsDefunct()) > > > > return false; > > > > > > not needed > > > > Ok with me but this was existing code. > > > > Alexander, promise you are not going to ask me to put those defunct checks > > back in? :) > > This method is supposed for internal usage so we don't need that. I think > you can trust Trevor in questions like this :) Oh there is trust, but great minds can differ ;) > > > >+ nsWeakFrame weakFrame = GetFrame(); > > > > > > why don't you just use a nsIFrame*? I don't think anything you call should > > > change the frame tree. > > > > The key phrase here is "don't think" (smile). > > Yeah, Trevor should have asked "why do you think anything you call should > change the frame tree" :) > > > The general rule is to not > > hold onto a raw frame pointer if there is any chance now (or in the > > reasonable future) of the executing code causing a frame to go bad, or in > > other words, if we are not immediately using the frame pointer. > > agree, key phrase "any chance" > > > This has > > bitten us before with really bad timing. > > You must say that was an age of tree creation at random timing. It was but I don't think there are any guarantees about layout calls not changing the frame tree. I'll ask bz/roc/ehsan. > > My worry is that here we are > > looping and asking for state (and possibly more things in the future). If > > there is some reflow that might happen by us asking for this (on demand, and > > since reflow was possibly interrupted), now or in the future, we can get > > bitten. > > iirc the asking for visibility styles don't trigger reflow. But yes can you > check this with bz please? In general states calculation triggering reflow > could bring more problems than crash. So we need to know why it can happen. Yep. > > The fact the biting doesn't have to happen deteriministically makes > > me especially cautious. > > deteriministically means reproducible or analyzable here? Heh well I spelled it wrong for starters. What I want to say is that if we did cause frames to go bad, it might only be infrequently and unreliably.
(In reply to Trevor Saunders (:tbsaunde) from comment #44) > (In reply to alexander :surkov from comment #43) > > > >+ if (accessible->Role() == nsIAccessibleRole::ROLE_PROPERTYPAGE || > > > >+ accessible->Role() == nsIAccessibleRole::ROLE_PANE) { > > > >+ break; > > > > > > ?! > > > > NVDA doesn't want us to return offscreen or invisible due to a tab not being > > in the foreground. It messes with their cache and makes them not want to use > > the visibility state, which causes us pain. So bottom line, if we get here > > that seems like a weird definition of "offscreen" but ok. > > I was objecting to calling Role() twice on the same accessible. Will fix thanks.
(In reply to David Bolter [:davidb] from comment #45) > It seems to be a pattern that we are dropping the verbs from our method > names. Is this intended? I'm a little unsure of how the code will read as > this happens more and more... won't it be difficult for new contributors to > tell the difference between method calls, constructors, and casts without > having to pause and think? > > I'm not going to fight hard on the method name but I do worry about this > pattern overall. I'm not sure I follow worries but it makes sense to file a bug for this, with examples, why it can be bad and what is another way. To have some place for discussions? > It was but I don't think there are any guarantees about layout calls not > changing the frame tree. I'll ask bz/roc/ehsan. yep, we need to figure if we have guarantees. In general weak frame has performance penalty depending on number of weak frames per presshell. It's not just a null check. > > > The fact the biting doesn't have to happen deteriministically makes > > > me especially cautious. > > > > deteriministically means reproducible or analyzable here? > > Heh well I spelled it wrong for starters. What I want to say is that if we > did cause frames to go bad, it might only be infrequently and unreliably. ok, reproducible then :) agree, that's hard to debug
Ehsan sides with reviewers on the use of weak frame here. I'm definitely outnumbered and will remove it forthwith! Aside: there was discussion with Trevor and Surkov about using a raii class to assert reflow appeared later in the stack.
Attached patch patch 2 (obsolete) — — Splinter Review
Attachment #578327 - Attachment is obsolete: true
Attachment #580427 - Flags: review?(trev.saunders)
Attachment #578327 - Flags: review?(trev.saunders)
Attachment #578327 - Flags: review?(surkov.alexander)
Attached patch patch 2.01 (obsolete) — — Splinter Review
Forgot to qrefresh. This is more recent.
Attachment #580427 - Attachment is obsolete: true
Attachment #580427 - Flags: review?(trev.saunders)
Attachment #580430 - Flags: review?(trev.saunders)
Comment on attachment 580430 [details] [diff] [review] patch 2.01 >+ if (role == nsIAccessibleRole::ROLE_PROPERTYPAGE || >+ role == nsIAccessibleRole::ROLE_PANE) { >+ break; >+ } Curleys feel silly, but I think Alex is ok with it in this case so I'll survive >+ nsIFrame* frame = GetFrame(); // Should be fine, was null checked in loop. The comment seems redundant, please put it on its own line atleast. >+ // Zero area rects can occur in the first frame of a multi-frame text flow, >+ // in which case the rendered text is not empty and the frame should not be >+ // marked invisible. accessible marked invisible you mean? >+ if (frame->GetType() == nsGkAtoms::textFrame && >+ !frame->GetStateBits() && NS_FRAME_OUT_OF_FLOW && I don't think that's what you want. >+ frame->GetRect().IsEmpty()) { >+ nsAutoString renderedText; >+ frame->GetRenderedText(&renderedText, nsnull, nsnull, 0, 1); >+ if (!renderedText.IsEmpty()) >+ return vstates; isn't this backwards? shouldn't we be returning invisible if the text is empty?
Attached patch patch 3 — — Splinter Review
Fixed sloppiness from when I added the text frame check back. Thanks Trevor.
Attachment #580430 - Attachment is obsolete: true
Attachment #580908 - Flags: review?(trev.saunders)
Attachment #580430 - Flags: review?(trev.saunders)
Comment on attachment 580908 [details] [diff] [review] patch 3 iF NOTHING ELSE THIS SEEMS WORTH WHILE AS SOMETHING MUCH EASIER TO UNDERSTAND, AND PRETTY EASY TO SEE IS FAIRLY CORRECT.
Attachment #580908 - Flags: review?(trev.saunders)
Attachment #580908 - Flags: review?(surkov.alexander)
Attachment #580908 - Flags: review+
Trevor, note the patch includes a test that fails without the code change (the absolute child test). I'd like to get this in before migration. Landing could get busy Monday/Tuesday.
Comment on attachment 580908 [details] [diff] [review] patch 3 Alex still care that your happy here, but I think David wants to get this landed which makes sense since its pretty low ris and shouldn't be too bad to back out if I've missed something horrible
Attachment #580908 - Flags: review?(surkov.alexander)
https://hg.mozilla.org/mozilla-central/rev/3a47e5f593ad
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Depends on: 717505
Comment on attachment 580908 [details] [diff] [review] patch 3 Review of attachment 580908 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/base/nsAccessible.cpp @@ +614,5 @@ > + if (view && view->GetVisibility() == nsViewVisibility_kHide) > + return vstates; > + > + } while (accessible = accessible->Parent()); > + I wonder if this algorithm work when deck container is not visible. I assumed that should be quite different to nsIFrame::IsVisibleConsideringAncestors logic but it's not. For example, they check frame visibility on each item in parent chain, we don't. Does GetRectVisibility make a trick? ::: accessible/tests/mochitest/states/Makefile.in @@ +62,5 @@ > test_selects.html \ > test_stale.html \ > test_textbox.xul \ > test_tree.xul \ > + test_visibility.html \ what about XUL code paths like accessible inside current tab, background tab, hidden tabbrowser, xul:deck, xul:stack.
Comment on attachment 580908 [details] [diff] [review] patch 3 Review of attachment 580908 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/base/nsAccessible.cpp @@ +601,5 @@ > + // We don't want background tab page content to be aggressively invisible. > + // Otherwise this foils screen reader virtual buffer caches. > + PRUint32 role = accessible->Role(); > + if (role == nsIAccessibleRole::ROLE_PROPERTYPAGE || > + role == nsIAccessibleRole::ROLE_PANE) { btw, what is the idea to test propertypage and pane roles both? Why does comment say about background tabs only?
(In reply to alexander :surkov from comment #58) > > + test_visibility.html \ > > what about XUL code paths like accessible inside current tab, background > tab, hidden tabbrowser, xul:deck, xul:stack. Good idea. We'll need to solve how we want to test browser chrome first I think. I've filed bug 719419 so we don't forget to add these.
(In reply to alexander :surkov from comment #59) > Comment on attachment 580908 [details] [diff] [review] > patch 3 > > Review of attachment 580908 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/src/base/nsAccessible.cpp > @@ +601,5 @@ > > + // We don't want background tab page content to be aggressively invisible. > > + // Otherwise this foils screen reader virtual buffer caches. > > + PRUint32 role = accessible->Role(); > > + if (role == nsIAccessibleRole::ROLE_PROPERTYPAGE || > > + role == nsIAccessibleRole::ROLE_PANE) { > > btw, what is the idea to test propertypage and pane roles both? Why does > comment say about background tabs only? I thought we discussed this on IRC but I forget. I'm not 100% sure about PANE actually. Do you have specific concerns?
(In reply to David Bolter [:davidb] from comment #60) > (In reply to alexander :surkov from comment #58) > > > > + test_visibility.html \ > > > > what about XUL code paths like accessible inside current tab, background > > tab, hidden tabbrowser, xul:deck, xul:stack. > > Good idea. We'll need to solve how we want to test browser chrome first I > think. sorry, what's up with that? we have tests like events/test_focus_browserui.xul (In reply to David Bolter [:davidb] from comment #61) > (In reply to alexander :surkov from comment #59) > > Comment on attachment 580908 [details] [diff] [review] > > patch 3 > > > > Review of attachment 580908 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: accessible/src/base/nsAccessible.cpp > > @@ +601,5 @@ > > > + // We don't want background tab page content to be aggressively invisible. > > > + // Otherwise this foils screen reader virtual buffer caches. > > > + PRUint32 role = accessible->Role(); > > > + if (role == nsIAccessibleRole::ROLE_PROPERTYPAGE || > > > + role == nsIAccessibleRole::ROLE_PANE) { > > > > btw, what is the idea to test propertypage and pane roles both? Why does > > comment say about background tabs only? > > I thought we discussed this on IRC but I forget. I don't recall details too. > I'm not 100% sure about > PANE actually. Do you have specific concerns? iirc, ROLE_PANE is used for document accessible as fallback, not sure why we might need it here. PROPERTYPAGE is used for tabbrowsers, maybe it's used for generic xul:deck too, I'm not sure, somebody needs to check that.
(In reply to alexander :surkov from comment #58) > ::: accessible/src/base/nsAccessible.cpp > @@ +614,5 @@ > > + if (view && view->GetVisibility() == nsViewVisibility_kHide) > > + return vstates; > > + > > + } while (accessible = accessible->Parent()); > > + > > I wonder if this algorithm work when deck container is not visible. I > assumed that should be quite different to > nsIFrame::IsVisibleConsideringAncestors logic but it's not. For example, > they check frame visibility on each item in parent chain, we don't. Does > GetRectVisibility make a trick? It would be good to avoid looking at view visibility. What does this check accomplish? Can we provide an API that does what you need instead?
(In reply to Timothy Nikkel (:tn) from comment #63) > > I wonder if this algorithm work when deck container is not visible. I > > assumed that should be quite different to > > nsIFrame::IsVisibleConsideringAncestors logic but it's not. For example, > > they check frame visibility on each item in parent chain, we don't. Does > > GetRectVisibility make a trick? > > It would be good to avoid looking at view visibility. What does this check > accomplish? I think this is a more or less copy of nsIFrame::IsVisibleConsideringAncestors. GetRectVisibility is sort of old thing I think it was used to protect us from visible elements having 0 dimensions due to some reason. > Can we provide an API that does what you need instead? that'd be great. It sounds like nsIFrame::IsVisibleConsideringAncestors would work for a11y without deck stuff check. Not sure about GetRectVisibility check though. Technically we need to expose 1) accessible invisible state when element is really invisible (it sounds no frame like display: none) 2) accessible offscreen state when element is not a screen (like scrolled off or placed not in current tab).
Timothy, would you like to file a new bug or will this API be happening on the view removal bug (is that bug 337801)?
(In reply to alexander :surkov from comment #64) > > Can we provide an API that does what you need instead? > > that'd be great. It sounds like nsIFrame::IsVisibleConsideringAncestors > would work for a11y without deck stuff check. What problem does checking if the frame is in the hidden panel of a deck cause? Is this just a problem with the deck that contains all of the tabs in the Firefox chrome, or all decks in general?
Or put another way: if a frame is hidden because it is in a hidden panel of a deck, why do you want to consider it visible?
(In reply to Timothy Nikkel (:tn) from comment #67) > Or put another way: if a frame is hidden because it is in a hidden panel of > a deck, why do you want to consider it visible? a11y has two states: invisible and offscreen. Screen readers ignore invisible accessible as they weren't in hierarchy at all. So we want to use offscreen state here because screen readers want to be aware of changes that happen in background tab. I think we shouldn't be restricted to tabs controls only and do all deck elements the same way.
(In reply to alexander :surkov from comment #68) > a11y has two states: invisible and offscreen. Screen readers ignore > invisible accessible as they weren't in hierarchy at all. So we want to use > offscreen state here because screen readers want to be aware of changes that > happen in background tab. It should be fairly easy to tell when a document is in a non-visible tab, IsActive() on the presshell will tell you this. So combining a call to IsVisibleConsideringAncestors() which doesn't cross the chrome/content boundary (the default behaviour with no flags passed) with a check if the document is in an active tab or not should handle this case. > I think we shouldn't be restricted to tabs controls only and do all deck > elements the same way. Are there other cases besides background tabs that make you think this way? My initial feeling is that decks shouldn't be special cased just for the case of background tabs.
(In reply to Timothy Nikkel (:tn) from comment #69) > > I think we shouldn't be restricted to tabs controls only and do all deck > > elements the same way. > > Are there other cases besides background tabs that make you think this way? > My initial feeling is that decks shouldn't be special cased just for the > case of background tabs. I'm not sure what UI controls are based on deck element except tabbox (and tabbrowser) but I'd say we should process all tabboxes in consistent way. However all that AT take care about is a case of tabbrowser where your IsActive approach should work fine.
Given that there are still so many unanswered questions as well as quite serious regressions with lots of inconsistent state reporting, I strongly suggest we back this out for tomorrow's Aurora-uplift and reland it afterwards so we have one more cycle to get all this stuff sorted out.
Agreed. This is not really a surprise and is why I wanted to land it at the beginning of a new cycle in the first place. Back it out please.
Whiteboard: [davidb looking into fix vs backout]
Whatever our reasoning in comment 30 (probably premature optimization), I think we need to go back to IsVisibleConsideringAncestors. I'll do that on bug 722248.
Whiteboard: [davidb looking into fix vs backout]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: