Last Comment Bug 810260 - xul:deck hidden pages shouldn't be offscreen
: xul:deck hidden pages shouldn't be offscreen
Status: RESOLVED FIXED
: access, regression
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla19
Assigned To: alexander :surkov
:
: alexander :surkov
Mentors:
Depends on:
Blocks: statea11y 686821
  Show dependency treegraph
 
Reported: 2012-11-09 03:56 PST by alexander :surkov
Modified: 2012-11-19 00:45 PST (History)
4 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
patch (4.45 KB, patch)
2012-11-15 04:14 PST, alexander :surkov
tbsaunde+mozbugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description alexander :surkov 2012-11-09 03:56:19 PST
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] 2012-11-09 11:55:08 PST
Isn't that regression related to bug 750612 ?
Comment 2 alexander :surkov 2012-11-11 16:45:53 PST
(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 2012-11-12 22:38:55 PST
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) 2012-11-13 01:58:46 PST
(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 2012-11-13 04:00:07 PST
(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) 2012-11-13 04:29:33 PST
(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 2012-11-13 05:33:24 PST
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) 2012-11-13 19:24:17 PST
(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 2012-11-15 04:14:23 PST
Created attachment 681936 [details] [diff] [review]
patch
Comment 10 Trevor Saunders (:tbsaunde) 2012-11-15 10:47:15 PST
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 2012-11-15 16:44:16 PST
(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) 2012-11-15 17:31:12 PST
(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 14 Ed Morley [:emorley] 2012-11-16 09:44:35 PST
https://hg.mozilla.org/mozilla-central/rev/6ef7a00233aa
Comment 15 alexander :surkov 2012-11-16 21:06:48 PST
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] 2012-11-18 20:52:45 PST
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) 2012-11-19 00:45:09 PST
Pushed: https://hg.mozilla.org/releases/mozilla-aurora/rev/bdd31f2f2b08

Note You need to log in before you can comment on or make changes to this bug.