Last Comment Bug 715857 - Disable a11y tests that put <tabbrowser> in random XUL documents and expect it to work
: Disable a11y tests that put <tabbrowser> in random XUL documents and expect i...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on:
Blocks: a11ytestdev 719754
  Show dependency treegraph
 
Reported: 2012-01-06 04:19 PST by Dão Gottwald [:dao]
Modified: 2012-01-20 06:12 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.43 KB, patch)
2012-01-06 04:31 PST, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v2 (5.41 KB, patch)
2012-01-16 05:10 PST, Dão Gottwald [:dao]
mzehe: review+
surkov.alexander: review-
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2012-01-06 04:19:19 PST
<tabbrowser> was removed from toolkit a long time ago. It's expected to work in browser.xul and navigator.xul, respectively, together with the associated scripts. It's not supposed to work in random other contexts.

People wasted enough time on these tests. This needs to stop. In case somebody cares about them, they need to be rewritten as browser-chrome tests or something.
Comment 1 Dão Gottwald [:dao] 2012-01-06 04:31:05 PST
Created attachment 586386 [details] [diff] [review]
patch
Comment 2 Marco Zehe (:MarcoZ) 2012-01-06 05:44:14 PST
I would request that these be disabled, but kept in the files so we can still properly refer to them when these tests get rewritten. Since these test important parts of accessibility code, we need to have the right sense of direction here on how to embed a web page that we can test on. CCing David and Alex.
Comment 3 David Bolter [:davidb] 2012-01-09 13:00:20 PST
I think we need to create a suite of browser chrome accessibility tests. Marco do you want to drive this?

(I'm fine with disabling our naughty tests, as long as we're active on an alternative)
Comment 4 Dão Gottwald [:dao] 2012-01-16 05:10:47 PST
Created attachment 588848 [details] [diff] [review]
patch v2
Comment 5 Marco Zehe (:MarcoZ) 2012-01-16 05:17:04 PST
Comment on attachment 588848 [details] [diff] [review]
patch v2

r=me with this change, thanks!

Do you have a good suggestion how we can best test these UI pieces in the future? We need these tests obviously, and we need the environment that these tests need as a prerequisite, and which we thought xul:tabbrowser was there for.
Comment 6 Dão Gottwald [:dao] 2012-01-16 05:22:37 PST
https://developer.mozilla.org/en/Browser_chrome_tests is what we usually use for testing the browser front-end.
Comment 8 Justin Wood (:Callek) (Away until Aug 29) 2012-01-16 19:41:47 PST
https://hg.mozilla.org/mozilla-central/rev/dc636e517089
Comment 9 alexander :surkov 2012-01-18 08:04:52 PST
Comment on attachment 588848 [details] [diff] [review]
patch v2

Guys, I'm surprised you disable tests without keeping exact alternative in your mind. If the reason is that people waste time to keep these tests working then I wonder why we weren't said about that earlier and we are faced to the fact "people wasted enough time".

Please work out the solution and do not disable them, just fix. Especially when the fix is nearly trivial (see events/test_focus_browserui.xul as example).

r- until we *have* to disable them
Comment 10 Dão Gottwald [:dao] 2012-01-18 08:17:20 PST
Ok, I'm telling you now, the tabbrowser binding isn't supposed to work in random contexts. This is a simple fact. We regularly add dependencies on browser.js & Co. and get bitten by these bogus tests every time. Keeping them as they are should not be considered an option. I also don't think it's reasonable to expect random browser devs to fix your tests.
Comment 11 alexander :surkov 2012-01-18 08:41:03 PST
I realize these tests misuse XUL and that brings problem to developers. But I wonder why you disable them rather than say us there's a problem and ask whether we can fix it in reasonable time. If tomorrow layout, toolkit or whatever else regress us then we'll spend hours for regression finding, debugging, fixing and etc and even worth we can ship release with regression because of these disabled tests.
Comment 12 alexander :surkov 2012-01-20 03:11:02 PST
ping, guys, any ongoing actions to fix that?
Comment 13 Dão Gottwald [:dao] 2012-01-20 04:16:25 PST
I'm not sure what exactly you're asking. Somebody needs to rewrite the tests. I don't feel that I'm responsible for that...
Comment 14 alexander :surkov 2012-01-20 04:28:14 PST
(In reply to Dão Gottwald [:dao] from comment #13)
> I'm not sure what exactly you're asking. Somebody needs to rewrite the
> tests. I don't feel that I'm responsible for that...

You don't since you're not familiar with a11y tests infrastructure I think. I hoped Marco can drive that since he approved tests disabling.

At least we need to have a bug and (ideally) get an assignee for that.
Comment 15 John P Baker 2012-01-20 05:28:52 PST
Comment on attachment 588848 [details] [diff] [review]
patch v2

>-          if (window.TabsOnTop)
>-            TabsOnTop.syncUI();
>-
>-          if (window.TabsInTitlebar)
>-            TabsInTitlebar.allowedBy("tabs-visible", visible);
>+          TabsOnTop.syncUI();
>+
>+          TabsInTitlebar.allowedBy("tabs-visible", visible);

Has this come from elsewhere?
Comment 16 Dão Gottwald [:dao] 2012-01-20 06:12:52 PST
(In reply to John P Baker from comment #15)
> Comment on attachment 588848 [details] [diff] [review]
> patch v2
> 
> >-          if (window.TabsOnTop)
> >-            TabsOnTop.syncUI();
> >-
> >-          if (window.TabsInTitlebar)
> >-            TabsInTitlebar.allowedBy("tabs-visible", visible);
> >+          TabsOnTop.syncUI();
> >+
> >+          TabsInTitlebar.allowedBy("tabs-visible", visible);
> 
> Has this come from elsewhere?

nope

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