Closed Bug 588999 Opened 11 years ago Closed 11 years ago

Add Tab Candy escape button to Tab Candy view

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 4.0b7

People

(Reporter: aza, Assigned: raymondlee)

References

Details

(Whiteboard: [in-litmus-bug-week])

Attachments

(2 files, 10 obsolete files)

Find the visual appearance here: http://www.flickr.com/photos/azaraskin/4908282585/
Blocks: 588980
Assignee: nobody → raymond
Attached patch v1 (obsolete) — Splinter Review
Attachment #467798 - Flags: review?(dolske)
Attachment #467798 - Flags: feedback?(ian)
Attached image exit button (obsolete) —
Comment on attachment 467798 [details] [diff] [review]
v1

Looks good to me
Attachment #467798 - Flags: feedback?(ian) → feedback+
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
Attached patch v2 (obsolete) — Splinter Review
Included binary files in the patch
Attachment #467798 - Attachment is obsolete: true
Attachment #467800 - Attachment is obsolete: true
Attachment #467798 - Flags: review?(dolske)
Attachment #468890 - Flags: review?(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
Attachment #468890 - Flags: review?(dolske) → review-
Blocks: 586990
Attached patch v2 (obsolete) — Splinter Review
* 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.
Attachment #468890 - Attachment is obsolete: true
Attachment #470726 - Flags: review?(dolske)
Attachment #470726 - Flags: feedback?(ian)
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.
Attachment #470726 - Attachment is patch: true
Attachment #470726 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 470726 [details] [diff] [review]
v2

Looks good to me.
Attachment #470726 - Flags: feedback?(ian) → feedback+
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...
Attachment #470726 - Flags: review?(dolske) → review?(dietrich)
Comment on attachment 470726 [details] [diff] [review]
v2

canceling review - can you address comment #8 and comment #10?
Attachment #470726 - Flags: review?(dietrich)
Re: #8 we can iterate if that becomes a problem in user testing.

For #10, it should be pretty simple to change.
Assignee: raymond → mitcho
Attachment #470726 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #472007 - Flags: review?(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.
Attachment #472007 - Flags: review?(dietrich)
Attachment #472007 - Flags: review+
Attachment #472007 - Flags: approval2.0+
Attached patch Patch for checkin (obsolete) — Splinter Review
Attachment #472007 - Attachment is obsolete: true
And backed out due to Moth test failures.
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.
Attachment #472161 - Attachment is obsolete: true
We need a test to be added before we can commit this.
Assignee: mitcho → raymond
Sorry about that, I missed it in the original push, so the earlier comment is bogus.
Attached patch Patch with test (obsolete) — Splinter Review
Same as previous patch but with test
Attachment #472427 - Flags: review?(dietrich)
Attachment #472427 - Attachment is patch: true
Attachment #472427 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 472427 [details] [diff] [review]
Patch with test

r=me, looks good, thanks for adding the test!
Attachment #472427 - Flags: review?(dietrich)
Attachment #472427 - Flags: review+
Attachment #472427 - Flags: approval2.0+
Attached patch Patch for check-in (obsolete) — Splinter Review
Attachment #472427 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/729be7ad43cf
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b6
Awesome, thanks everyone for getting this in!
Backed out because of Linux test failures:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283899045.1283900225.31464.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
This patch no longer applies cleanly and has to be redone. Perhaps I'll get to it a tad later today.
Attached patch Patch for check-in (obsolete) — Splinter Review
Attachment #472430 - Attachment is obsolete: true
Blocks: 594094
http://hg.mozilla.org/mozilla-central/rev/4bc623a55708
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Attached image Screenshot
Is this how this is supposed to look like) (screenshot taken on linux)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch v3 (obsolete) — Splinter Review
Should address the issue in comment #30 and comment #31

Pushed to the try and waiting for the results
Attachment #473358 - Attachment is obsolete: true
Comment on attachment 473894 [details] [diff] [review]
v3

This patch passed the tests on Win7 opt on try server.
Attachment #473894 - Flags: review?(dietrich)
Attachment #473894 - Flags: review?(dietrich)
Attachment #473894 - Flags: review+
Attachment #473894 - Flags: approval2.0+
Attachment #473894 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/65161587c8b4
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
We need a test in Litmus to make sure this button works correctly
Status: RESOLVED → VERIFIED
Flags: in-litmus?
As part of Bug Week, thanks to kbrosnan for the new test case in Litmus.
Flags: in-litmus? → in-litmus+
Whiteboard: [in-litmus-bug-week]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.