588999 - Add Tab Candy escape button to Tab Candy view

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Aza Raskin [:aza]]

Find the visual appearance here: http://www.flickr.com/photos/azaraskin/4908282585/

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: exit button

Comment 3

[Ian Gilman [:iangilman]]

Comment on attachment 467798 [details] [diff] [review]
v1

Looks good to me

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 467798 [details] [diff] [review]
v1

>diff -r 55ef0e0529bc -r ad03d642269b browser/themes/winstripe/browser/tabview/exit-button.png
>Binary file browser/themes/winstripe/browser/tabview/exit-button.png has changed

You should enable git-style-diffs so that your diffs include the binary data, avoiding the need to attach a separate file.

See: https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_customize_the_format_of_the_patches_Mercurial_creates.3f

Specifically:

[diff]
git=true

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v2

Included binary files in the patch

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 468890 [details] [diff] [review]
v2

>+++ b/browser/base/content/tabview/tabview.css	Sat Aug 21 00:34:53 2010 +0800
...
>+#exit-button {
>+  width: 29px;
>+  height: 28px;
...
>+  background: url("chrome://browser/skin/tabview/exit-button.png") no-repeat;
>+}

These parts should be in browser/themes/*/browser/tabview/tabview.css

>+++ b/browser/base/content/tabview/ui.js	Sat Aug 21 00:34:53 2010 +0800
...
>+  _addExitButton: function() {
>+    let self = this;
>+    let $container = iQ("<div>")
>+      .attr("id", "exit-button")
>+      .appendTo("body")

Why create the button programatically? Just add it to tabview.html since it's always present?

Also, this has some accessibility issues: things like screenreaders have no way of knowing this is a magic <div> acting as a button, and you can't use the keyboard to tab to it for activating it.

While this might be part of a broader tab view A11Y problem (isn't there a bug on this?), we should at least not make it worse. :)

I suspect the right thing to do here is to make this an HTML <input> with appropriate styling to make it look right, or at least add role="button". Might want to ask the A11Y guys on IRC #a11y (and if they've been looking at tab view!).

Google search also suggested: http://www.paciellogroup.com/blog/misc/ARIA/togglebutton.html

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v2

* Changed the div to input
* For some reasons, when setting the ALT attribute, the text would be shown on top of the button (without hovering over the button with mouse pointer).  If not setting the ALT, other texts are shown on the top. Therefore, using alt="" here.
* Filed meta Bug 592204 for accessibility issues.

Comment 8

[Michael Lefevre]

Just a thought, but this button (even pressed) doesn't really scream "exit". The mockup doesn't show the window controls - on Windows, there will be a big red "X" button right next to the little TabCandy button. Having the same button for exit and entry seems to assume that the user actually knows what they have got into and how they did it.

Comment 9

[Ian Gilman [:iangilman]]

Comment on attachment 470726 [details] [diff] [review]
v2

Looks good to me.

Comment 10

[Aza Raskin [:aza]]

Shouldn't the button be constructed from CSS and the Tab Candy icon rather than as a button as a whole. This will make it easier to go between various platforms...

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 470726 [details] [diff] [review]
v2

canceling review - can you address comment #8 and comment #10?

Comment 12

[Aza Raskin [:aza]]

Re: #8 we can iterate if that becomes a problem in user testing.

For #10, it should be pretty simple to change.

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v3, now simply using the tabview.png image and using CSS for the border (comment #10)

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 472007 [details] [diff] [review]
Patch v3, now simply using the tabview.png image and using CSS for the border (comment #10)

r=me w/ newlines added at the end of those files.

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Comment 16

[Josh Matthews [:jdm]]

And backed out due to Moth test failures.

Comment 17

[Michael Yoshitaka Erlewine [:mitcho]]

Josh, was this actually ever committed? Looking at hg and the tinderbox log, I don't think this patch was included in that failed checkin.

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

We need a test to be added before we can commit this.

Comment 19

[Josh Matthews [:jdm]]

Sorry about that, I missed it in the original push, so the earlier comment is bogus.

Comment 20

[Raymond Lee [:raymondlee]]

Attachment: Patch with test

Same as previous patch but with test

Comment 21

[Dietrich Ayala (:dietrich)]

Comment on attachment 472427 [details] [diff] [review]
Patch with test

r=me, looks good, thanks for adding the test!

Comment 22

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 23

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/729be7ad43cf

Comment 24

[Aza Raskin [:aza]]

Awesome, thanks everyone for getting this in!

Comment 25

[(no longer active)]

Backed out because of Linux test failures:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283899045.1283900225.31464.gz

Comment 26

[Aza Raskin [:aza]]

Not sure if I'm reading this right, but it appears to me to be one of the test cases that has intermittent timing-based failures. I believe Mitcho is working on fixing that test.

Comment 27

[Michael Yoshitaka Erlewine [:mitcho]]

This patch no longer applies cleanly and has to be redone. Perhaps I'll get to it a tad later today.

Comment 28

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 29

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/4bc623a55708

Comment 30

[Michael Monreal [:micmon]]

Attachment: Screenshot

Is this how this is supposed to look like) (screenshot taken on linux)

Comment 31

[(no longer active)]

This turned Win7 Moth orange:

<http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284069985.1284071830.22061.gz>

And I backed it out:

<http://hg.mozilla.org/mozilla-central/rev/b5896c8b6d41>

Now, looking at the try server, the exact same failure had happened on try as well:

<http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1284028054.1284029738.12758.gz>

Oranges causes pain for developers (because they can't push, and if they pull, they'll get a bad tree, based on which they might be pushing their own stuff to the try server etc), and they cause pain for the person who pushes a patch (because they have to watch the tree longer than they originally expected).  Please pay attention to the results of try server runs, and do not mark a bug as checkin-needed unless the results of the try server run confirm its good shape.

Comment 32

[Raymond Lee [:raymondlee]]

Attachment: v3

Should address the issue in comment #30 and comment #31

Pushed to the try and waiting for the results

Comment 33

[Raymond Lee [:raymondlee]]

Comment on attachment 473894 [details] [diff] [review]
v3

This patch passed the tests on Win7 opt on try server.

Comment 34

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in [r+a=dietrich]

Comment 35

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/65161587c8b4

Comment 36

[Tracy Walker [:tracy]]

We need a test in Litmus to make sure this button works correctly

Comment 37

[Tracy Walker [:tracy]]

As part of Bug Week, thanks to kbrosnan for the new test case in Litmus.