810260 - xul:deck hidden pages shouldn't be offscreen

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Jamie gave me an example on irc. Help -> About dialog has xul:deck containing update controls (they are visible depending on firefox update state). Those controls shouldn't be really marked offscreen but rather invisible.

Suggestion was:
So tabpanels should do offscreen state
deck should do invisible state

alternatively we could ignore to create accessibles for deck and for hidden panels.

Thoughts?

Comment 1

[Hubert Figuiere [:hub]]

Isn't that regression related to bug 750612 ?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #1)
> Isn't that regression related to bug 750612 ?

I believe it was bug 686821.

Comment 3

[alexander :surkov (:asurkov)]

Trev, your opinion. (We should be fast to fix this in Firefox 19 and back port to Firefox 18 where we regressed).

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #0)
> Jamie gave me an example on irc. Help -> About dialog has xul:deck
> containing update controls (they are visible depending on firefox update
> state). Those controls shouldn't be really marked offscreen but rather
> invisible.

so this is a kind of funny use of set of tabs where which one is visible is controled by program not user  right?

> Suggestion was:
> So tabpanels should do offscreen state
> deck should do invisible state

why do you think tab pannels are more like for the user controlled case, and deck for the program controlled one?

> alternatively we could ignore to create accessibles for deck and for hidden
> panels.

I thing I vaguely think invisible accessible is safer than no  accessible.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> (In reply to alexander :surkov from comment #0)
> > Jamie gave me an example on irc. Help -> About dialog has xul:deck
> > containing update controls (they are visible depending on firefox update
> > state). Those controls shouldn't be really marked offscreen but rather
> > invisible.
> 
> so this is a kind of funny use of set of tabs where which one is visible is
> controled by program not user  right?

those aren't really tabs, I miss the question though

> > Suggestion was:
> > So tabpanels should do offscreen state
> > deck should do invisible state
> 
> why do you think tab pannels are more like for the user controlled case, and
> deck for the program controlled one?

it seems all we can do is try to figure out empirical regularities since deck frame is common engine for both cases.

> > alternatively we could ignore to create accessibles for deck and for hidden
> > panels.
> 
> I thing I vaguely think invisible accessible is safer than no  accessible.

safer in terms AT can workaround specific cases I guess, that's not what we'd want it seems.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > (In reply to alexander :surkov from comment #0)
> > > Jamie gave me an example on irc. Help -> About dialog has xul:deck
> > > containing update controls (they are visible depending on firefox update
> > > state). Those controls shouldn't be really marked offscreen but rather
> > > invisible.
> > 
> > so this is a kind of funny use of set of tabs where which one is visible is
> > controled by program not user  right?
> 
> those aren't really tabs, I miss the question though

I'm not sure there was a question.  What would you say they are if not tabs?

> > > Suggestion was:
> > > So tabpanels should do offscreen state
> > > deck should do invisible state
> > 
> > why do you think tab pannels are more like for the user controlled case, and
> > deck for the program controlled one?
> 
> it seems all we can do is try to figure out empirical regularities since
> deck frame is common engine for both cases.

That sounds like it will be broken for some cases no matter what we do, but I'm not sure exactly what you suggest.  Is there a reason we can't add attributes to xul to distinguish these cases?  Is there a reason the not visible tabs can't be made display: none / hidden?

> > > alternatively we could ignore to create accessibles for deck and for hidden
> > > panels.
> > 
> > I thing I vaguely think invisible accessible is safer than no  accessible.
> 
> safer in terms AT can workaround specific cases I guess, that's not what
> we'd want it seems.

true, that's atleast part of the reason, it also just feels more uniform or something, but I don't feel terribly strongly about this.

Comment 7

[alexander :surkov (:asurkov)]

Currently we have two cases:

tabpanels is used for tabs
deck is used for that About dialog

My suggestion was: expose offscreen state for tabpanels elements, expose invisible state for deck elements.

extra attribute on deck might be a good idea if we had a case of deck element used as tabpanels (where the user can manipulate the index).

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> Currently we have two cases:
> 
> tabpanels is used for tabs
> deck is used for that About dialog
> 
> My suggestion was: expose offscreen state for tabpanels elements, expose
> invisible state for deck elements.

that seems fine.

> extra attribute on deck might be a good idea if we had a case of deck
> element used as tabpanels (where the user can manipulate the index).

ok, I guess I'm fine to worry about this when and if it becomes an issue.

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 10

[Trevor Saunders (:tbsaunde)]

Comment on attachment 681936 [details] [diff] [review]
patch

>+    if (deckFrame && deckFrame->GetSelectedBox() != curFrame) {
>+      if (deckFrame->GetContent()->NodeInfo()->Equals(nsGkAtoms::tabpanels,
>+                                                      kNameSpaceID_XUL))

maybe we should add IsXUL(tag) like IsHTML(tag) etc?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> Comment on attachment 681936 [details] [diff] [review]
> patch
> 
> >+    if (deckFrame && deckFrame->GetSelectedBox() != curFrame) {
> >+      if (deckFrame->GetContent()->NodeInfo()->Equals(nsGkAtoms::tabpanels,
> >+                                                      kNameSpaceID_XUL))
> 
> maybe we should add IsXUL(tag) like IsHTML(tag) etc?

IsXUL() && Tag() == nsGKAtoms::tabpanels isn't it roughly the same?

Comment 12

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #11)
> (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > Comment on attachment 681936 [details] [diff] [review]
> > patch
> > 
> > >+    if (deckFrame && deckFrame->GetSelectedBox() != curFrame) {
> > >+      if (deckFrame->GetContent()->NodeInfo()->Equals(nsGkAtoms::tabpanels,
> > >+                                                      kNameSpaceID_XUL))
> > 
> > maybe we should add IsXUL(tag) like IsHTML(tag) etc?
> 
> IsXUL() && Tag() == nsGKAtoms::tabpanels isn't it roughly the same?

yeah, just shorter.  besides its kind of inconsistant we can do content->IsHTML(nsGkAtoms::div) or content->IsSVG(nsGkAtoms::Description) but not content->IsXUL(nsGkAtoms::Tabpannels)

Comment 13

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6ef7a00233aa

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6ef7a00233aa

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 681936 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 686821
User impact if declined: may confuse screen readers/users since it makes elements that are invisible for sighted users visible for screen reader users.
Testing completed (on m-c, etc.): m-c, behavior is covered by testsuite
Risk to taking this patch (and alternatives if risky): low risk (roll back to previous behavior)
String or UUID changes made by this patch: no

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 681936 [details] [diff] [review]
patch

[Triage Comment]
Low risk fix for a FF18 a11y regression - approving for FF18.

If this is landed on mozilla-aurora before ~11AM PT tomorrow, this will make the merge from Aurora 18 to Beta 18. If landed 11AM-5PM PT tomorrow on mozilla-beta, it will make the first FF18 Beta. If landed after that, it will end up in the second FF18 beta.

Comment 17

[Marco Zehe (:MarcoZ)]

Pushed: https://hg.mozilla.org/releases/mozilla-aurora/rev/bdd31f2f2b08