Last Comment Bug 596450 - DOM attribute for the tab node needed which references the tabgroup
: DOM attribute for the tab node needed which references the tabgroup
Status: VERIFIED FIXED
[good first bug]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: P2 normal
: Firefox 9
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks: 596264 596504 686895
  Show dependency treegraph
 
Reported: 2010-09-14 18:21 PDT by Henrik Skupin (:whimboo)
Modified: 2016-04-12 14:00 PDT (History)
9 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (1.18 KB, patch)
2011-09-05 04:35 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Review
v2 (1.75 KB, patch)
2011-09-05 18:49 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Review
Patch for checkin (1.98 KB, patch)
2011-09-07 02:02 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Review

Description Henrik Skupin (:whimboo) 2010-09-14 18:21:47 PDT
+++ This bug was initially created as a clone of Bug #596256 +++

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6pre) Gecko/20100913 Firefox/4.0b6pre

As talked on bug 596256 we need a way to be able to detect in which tabgroup a tab is contained. I talked with Mitcho on IRC and we came up with a solution that we can add a DOM attribute to any of the tabs which references the tabgroup. That would solve all of our problems and we could implement real ui testing with Mozmill.

Thanks Mitcho for that idea!
Comment 1 Michael Yoshitaka Erlewine [:mitcho] 2010-09-14 18:25:18 PDT
Note, to be specific, the idea is to add a DOM attribute (data-group attribute?) to all div.tab elements with the group id. Then also have an attribute on div.group elements which encode the group id's.
Comment 2 Aza Raskin [:aza] 2010-10-12 01:05:23 PDT
Henrik, I'm assigning this to you. Please unassign if you aren't the right person. I'm also moving this to post Firefox 4, because I don't think we will block on it? That said, I'd love to get the fix in.

Feel free to disagree and we'll discuss.
Comment 3 Henrik Skupin (:whimboo) 2010-10-12 01:30:08 PDT
Sadly I don't have time to work on code for TabCandy. I would love to see a fix for Firefox 4, but given other higher priority bugs on your side and our workaround, this is probably a future targeted one. :/

I don't think that this is a too hard to implement patch. So lets put the good first bug entry into the whiteboard.
Comment 4 Raymond Lee [:raymondlee] 2011-09-05 04:35:11 PDT
Created attachment 558261 [details] [diff] [review]
v1
Comment 5 Tim Taubert [:ttaubert] 2011-09-05 09:53:31 PDT
Comment on attachment 558261 [details] [diff] [review]
v1

Review of attachment 558261 [details] [diff] [review]:
-----------------------------------------------------------------

Is this still wanted/needed? I'd rather have bug 616576 implemented but that's a lot of work, sadly.

We need to unset/remove the "data-group" attribute once the tab gets removed from a group.
Comment 6 Raymond Lee [:raymondlee] 2011-09-05 18:49:26 PDT
Created attachment 558365 [details] [diff] [review]
v2

> We need to unset/remove the "data-group" attribute once the tab gets removed
> from a group.

Fixed.
Comment 7 Tim Taubert [:ttaubert] 2011-09-06 02:37:37 PDT
Henrik, would you still benefit from this patch? Do you want us to land it?
Comment 8 Henrik Skupin (:whimboo) 2011-09-06 02:52:24 PDT
Functional testing wise it would be a great improvement for us. It would make our life way easier to retrieve the list of tabs for a given tabgroup by simply using querySelector with the tabgroup class name.
Comment 9 Tim Taubert [:ttaubert] 2011-09-06 02:56:44 PDT
(In reply to Henrik Skupin (:whimboo) from comment #8)
> Functional testing wise it would be a great improvement for us. It would
> make our life way easier to retrieve the list of tabs for a given tabgroup
> by simply using querySelector with the tabgroup class name.

Ok, cool, this is an easy fix and doesn't cause too much hassle. Let's do it.
Comment 10 Raymond Lee [:raymondlee] 2011-09-06 20:38:16 PDT
Comment on attachment 558365 [details] [diff] [review]
v2

Any additional things we need to add in this patch?
Comment 11 Tim Taubert [:ttaubert] 2011-09-07 00:19:39 PDT
Comment on attachment 558365 [details] [diff] [review]
v2

Review of attachment 558365 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Comment 12 Raymond Lee [:raymondlee] 2011-09-07 02:02:42 PDT
Created attachment 558755 [details] [diff] [review]
Patch for checkin

Pushed to try and waiting for the results
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&tree=Try&rev=3947d2a47ffb
Comment 13 Tim Taubert [:ttaubert] 2011-09-09 04:57:09 PDT
http://hg.mozilla.org/integration/fx-team/rev/43f1efa4b17d
Comment 14 :Margaret Leibovic 2011-09-14 12:14:38 PDT
https://hg.mozilla.org/mozilla-central/rev/43f1efa4b17d
Comment 15 Henrik Skupin (:whimboo) 2011-09-15 09:52:03 PDT
Looks great with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a1) Gecko/20110915 Firefox/9.0a1

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