390979 - tabbrowser drag&drop code cleanup

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

We have code that adjusts the indicators position by half of the its width, which means that we depend on the element not being collapsed for it to be positioned correctly on the first "drag" event. We should do what the SeaMonkey code does and set the margin correctly on the image element in the theme. This will allow us to remove the halfIndWidth code at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tabbrowser.xml&rev=1.234#1803 , and switch to just setting collapse on the element instead of setting a class attribute (see also bug 389675).

I think there are probably other improvements in the SeaMonkey version that we could use (e.g. switching from fields to explicit getAnonymousElementById calls in some cases, removing unneeded variables, etc), but I haven't looked closely.

CCing some people who might be interested in taking this bug.

Comment 1

[Wayne Woods]

Attachment: Patch v1 - work in progress

I might as well take this if nobody else wants it. Here's a work in progress patch. It:
1. Sets the margin-left for the indicator-bar, and so that the halfIndWidth code has been removed
2. Uses collapse instead of hidden, but I haven't removed the use of the 'dragging' attribute yet, so the collapse is set via css. I didn't have time to be sure that the attribute wasn't being used anywhere else.
3. Reduces and simplifies the ltr/rtl section of the code. A lot of it was duplicated. We can never simplify it as much as the SeaMonkey code because we have tab scrolling to worry about as well.

I haven't tested this out on Windows, just Mac. If anyone could test it, that'd be much appreciated.

Also, when tabs are overflowing (scrolling is enabled), there's now a minor discrepancy between the maxMargin and the final tab index that I have to resolve. i.e. if you drag a tab to the very end of the tab strip, the arrow shifts depending on whether you're over the final tab or the scroll widget.

Anyway, I wanted to post what I have so far, in case I'm heading the wrong way.

Comment 2

[Wayne Woods]

Attachment: Trimmed indicator image for Mac

As part of the cleanup, I also trimmed off the transparent pixels surrounding the indicator on Mac (zip attached). The Mac indicator arrow is 8x10, but it's placed within an 11x11 image, so the arrow is off-centre within the image, making it harder to place it correctly. There's no such problem with the Windows version.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Thanks for taking this Wayne!

(In reply to comment #1)
> 2. Uses collapse instead of hidden, but I haven't removed the use of the
> 'dragging' attribute yet, so the collapse is set via css. I didn't have time to
> be sure that the attribute wasn't being used anywhere else.

I'm pretty sure it isn't being used anywhere else, I'd just remove it.

> 3. Reduces and simplifies the ltr/rtl section of the code. A lot of it was
> duplicated. We can never simplify it as much as the SeaMonkey code because we
> have tab scrolling to worry about as well.

I was wondering about this when I thought about this approach some more. Will this approach work given our RTL support, and the fact that we set a different margin (left or right) depending on direction?

Comment 4

[Wayne Woods]

(In reply to comment #3)
> I was wondering about this when I thought about this approach some more. Will
> this approach work given our RTL support, and the fact that we set a different
> margin (left or right) depending on direction?

I was wondering this, too. For rtl, the margin should be set to +width/2 rather than -width/2, shouldn't it? If so, I could just set an attribute on the indicator bar, depending on the direction, and set the margin either way in the css. Unless there's already something set globally in rtl themes that is accessible via css?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> If so, I could just set an attribute on the indicator bar, depending on the
> direction, and set the margin either way in the css.

Ah, yes, of course. The way we generally do this is to set chromedir="&locale.dir;" on the element and style it with a selector like |elem[chromedir="rtl"]|.

Comment 6

[Wayne Woods]

Attachment: Patch v2

Thanks Gavin. I got the chromedir to work. Changes in the new version of the patch are:
- dragging attribute removed. |collapsed| set directly
- margins set in css now ltr/rtl-dependent (margin-left or margin-right set)
- adjusted maxMargin and minMargin formulas so the indicator sits pretty at the extremes - apparently the use of ib.boxObject.x in the formula did nothing but throw the margin out once it had a margin set
- removed the "first-tab" 3px margin-left from the Pinstripe css and set it to use a 3px .tabs-left instead (but not when overflow is on = tab scrolling is active). This brings it in line with Winstripe, stops the first tab being artificially squished, and stops the dnd indicator from being pushed out of view at the extreme left (it's now only cropped by 1px or so)

One question with regards to maxMargin:
var maxMargin = Math.min(tabStripBoxObject.x + tabStripBoxObject.width, 
ib.boxObject.x + ib.boxObject.width - ind.boxObject.width);

In all my testing (including overflow situations), I can't find one case in which the latter arg is the smaller. If nobody can come up with any cases, I'd like to remove that, and just set maxMargin to the first arg.

Comment 7

[Wayne Woods]

(In reply to comment #6)
> - adjusted maxMargin and minMargin formulas so the indicator sits pretty at the
> extremes - apparently the use of ib.boxObject.x in the formula did nothing but
> throw the margin out once it had a margin set

OK, forget that. ib.boxObject.x is important when the History sidebar is on. I'm gonna have to add that back and work out another way of ensuring ironing out the kinks. New patch tomorrow.

Comment 8

[Wayne Woods]

Attachment: Patch v3

Version 3 changes:
- fixed max/minMargin problems from comment 7. Using |this.boxObject.x| instead of |ib.boxObject.x| in the minMargin formula got rid of the margin-left issue
- removed isTabDrag var. It was only declared and referenced once, so it was useless
- removes your "FIXME: bug 390979" patch, since it won't be needed once collapsed is used

Questions and comments:
1. the maxMargin question from the end of comment 6
2. if all things are equal, what is faster / better to use .x or .screenX? Basically in situations where (A.x - B.x) == (A.screenX - B.screenX), which should I go for?
3. from comment 0, I'm not sure in which situations it's best to do an explicit getAnonymousElementById call and remove the current use of a field, as I don't understand the performance differences between the two strategies (memory usage vs cycles, maybe?). Maybe when a field is only referenced once it's best to use the call instead?

Thanks

Comment 9

[Wayne Woods]

While the patch is waiting for review, if anyone following this bug can build on Windows and Linux and can test it out, that'd be much appreciated, too :) I haven't been able to test the Winstripe code, though it should all be fine.

Comment 10

[Wayne Woods]

Attachment: Patch v4 - updated for tabbrowser file migration

Updated for the recent tabbrowser file migrations.

Gavin, could I please get at least a part review of this soon? I'd like to know if there are any major problems with my method that I need to work on, but I hope not. I'm hoping I can get this in before it bitrots again, and then worry about further tab improvements. Thanks!

Comment 11

[Wayne Woods]

I also would like to get these changes in for bug 320638 to land, since the changes would dramatically affect the patch there. I believe the feature freeze is in a few days, so probably a pipe dream, though :)

Comment 12

[Wayne Woods]

Attachment: Trivial changes in spacing

Comment 13

[:Gavin Sharp [email: [e-mail]]]

I'll try to get this reviewed+landed today or Tuesday.

Comment 14

[Wayne Woods]

This Tuesday, maybe? :)

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Sorry for the delay here, this fell off the radar because of some other patches I needed to review.

(In reply to comment #8)
> Questions and comments:
> 1. the maxMargin question from the end of comment 6

Best to leave as is for now, unless you can prove it's unneeded. We can file another followup to investigate further.

> 2. if all things are equal, what is faster / better to use .x or .screenX?
> Basically in situations where (A.x - B.x) == (A.screenX - B.screenX), which
> should I go for?

If all you care about is the delta, I don't think it matters.

> 3. from comment 0, I'm not sure in which situations it's best to do an
> explicit getAnonymousElementById call and remove the current use of a field,
> as I don't understand the performance differences between the two strategies
> (memory usage vs cycles, maybe?)

That's pretty much it... fields should be avoided unless the element they hold is accessed so frequently that calling get*() each time becomes a problem.

I'll take a look at the patch now...

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 279450 [details] [diff] [review]
Trivial changes in spacing

>Index: browser/themes/pinstripe/browser/browser.css

>-.tabs-left {
>-	display: none !important;
>-}

>-.tabbrowser-tab[first-tab="true"] > .tab-image-left {
>-  margin-left: 3px !important;
>-}

>+.tabs-left {
>+  display: -moz-box;
>+  width: 3px;
>+}

>+.tabbrowser-tabs[overflow="true"] .tabs-left {
>+  display: none;
>+}

I'm not sure I understand this change, can you explain?

>Index: browser/themes/winstripe/browser/browser.css

>+.tab-drop-indicator-bar[chromedir="ltr"] {
>+  /* set margin-left to -half indicator width: */
>+  margin-left: -5px;
>+}
>
>+.tab-drop-indicator-bar[chromedir="rtl"] {
>+  /* set margin-right to -half indicator width: */
>+  margin-right: -5px;
> }

Now that I look at this again, can't you just use -moz-margin-start instead of margin-right/left? That should do the right thing depending on directionality (same for pinstripe).

>Index: browser/base/content/tabbrowser.xml

>-            var isTabDrag = (aDragSession.sourceNode && 
>-                             aDragSession.sourceNode.parentNode == this.mTabContainer);
>-            if (!isTabDrag && aEvent.target.localName == "tab") {
>+            if ((!aDragSession.sourceNode ||
>+                 aDragSession.sourceNode.parentNode != this.mTabContainer)
>+                && aEvent.target.localName == "tab") {

This is easier to understand with the temporary, I'd say just leave it...

Which platforms has this been tested on? Have you tested RTL with winstripe? I can help with testing as needed.

Comment 17

[Wayne Woods]

(In reply to comment #16)
> (From update of attachment 279450 [details] [diff] [review])
> >Index: browser/themes/pinstripe/browser/browser.css
> 
> >-.tabs-left {
> >-	display: none !important;
> >-}
> 
> >-.tabbrowser-tab[first-tab="true"] > .tab-image-left {
> >-  margin-left: 3px !important;
> >-}
> 
> >+.tabs-left {
> >+  display: -moz-box;
> >+  width: 3px;
> >+}
> 
> >+.tabbrowser-tabs[overflow="true"] .tabs-left {
> >+  display: none;
> >+}
> 
> I'm not sure I understand this change, can you explain?

In order to create a margin on the left side of the tabs bar, some css had been added to give the first tab a 3px margin-left. I presume this is either for aesthetics or so that the dnd indicator is still visible at the extreme left. However that approach causes the first tab image to shrink by 3px, which becomes obvious in certain situations. I think a more consistent approach is to simply set the tabs-left element (currently hidden) to 3px, which is the way it's done on Windows. It gives the same result without distorting the tab. The overflow check is because we don't want or need that gap if the tab bar has the overflow scroll arrows.


> >Index: browser/themes/winstripe/browser/browser.css
> 
> >+.tab-drop-indicator-bar[chromedir="ltr"] {
> >+  /* set margin-left to -half indicator width: */
> >+  margin-left: -5px;
> >+}
> >
> >+.tab-drop-indicator-bar[chromedir="rtl"] {
> >+  /* set margin-right to -half indicator width: */
> >+  margin-right: -5px;
> > }
> 
> Now that I look at this again, can't you just use -moz-margin-start instead of
> margin-right/left? That should do the right thing depending on directionality
> (same for pinstripe).

Thanks. I wasn't aware of that. I'll add it back in and test it out.

> >Index: browser/base/content/tabbrowser.xml
> 
> >-            var isTabDrag = (aDragSession.sourceNode && 
> >-                             aDragSession.sourceNode.parentNode == this.mTabContainer);
> >-            if (!isTabDrag && aEvent.target.localName == "tab") {
> >+            if ((!aDragSession.sourceNode ||
> >+                 aDragSession.sourceNode.parentNode != this.mTabContainer)
> >+                && aEvent.target.localName == "tab") {
> 
> This is easier to understand with the temporary, I'd say just leave it...

Will do.

> Which platforms has this been tested on? Have you tested RTL with Winstripe? I
> can help with testing as needed.

Tested on Mac only (only one I can build on). And not using RTL. Testing would be much appreciated. Thanks :)

Comment 18

[[e-mail]]

(In reply to comment #15)
>>2. if all things are equal, what is faster / better to use .x or .screenX?
>>Basically in situations where (A.x - B.x) == (A.screenX - B.screenX), which
>>should I go for?
>If all you care about is the delta, I don't think it matters.
Actually it depends. The C++ code generally deals in internal coordinates, which are neither event nor screen coordinates. That being said, it is conceptually easier to convert internal coordinates to screen coordinates, except for the stumbling block that WidgetToScreen is a pretty slow system call on Linux.

Comment 19

[Wayne Woods]

Attachment: Patch v6 - incorporates review changes

Thanks Gavin. Added your suggestion for -moz-margin-start and also restored the isTabDrag variable.

It'll need testing on Windows, for sure. If you're able to make a Win build that you can make available, I'm happy to test it as well (I use Win XP at work, I just can't build for it)

tabbrowser.xml has a lot of fields defined. Removing some of the less important ones should probably be the subject of a different patch at this point.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Yeah, I think there are existing bugs on reducing the number of fields in tabbrowser. I'll run the patch through the try server and see if I can get it to spit out a Windows/Mac/Linux builds.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Builds with the latest patch (attachment 280526 [details] [diff] [review]) are now available at: https://build.mozilla.org/tryserver-builds/

[e-mail]                11-Sep-2007 17:19   17M
[e-mail]2          11-Sep-2007 17:50  8.6M
[e-mail]    11-Sep-2007 18:30  5.9M

Comment 22

[Wayne Woods]

Thanks! To me, the Windows build behaves correctly in LTR.

I haven't tested RTL properly because it seems that Winstripe isn't RTL compatible yet, either. But your build behaves the same as the current nightly, at least. With both, the indicator arrow seems to be drawn mid-way between tabs, even if that's hard to determine because the tab ends are switched. Also with both in RTL, the arrow doesn't seem to be in the right place when dragging to the far right of the tab bar, but it's hard to tell what's at fault there.

Comment 23

[Wayne Woods]

Thanks Gavin. Requesting checkin.

Comment 24

[Phil Ringnalda (:philor)]

See http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/2e60838f82b9b49c/d4a28073630e89b7 - once the current freeze ends, you'll need approval1.9 for anything, including browser/ (unless you get blockingFx3, but approval's much more likely, and either way, you're not ready for checkin-needed yet).

Comment 25

[Wayne Woods]

Done, thanks.

Comment 26

[Wayne Woods]

Comment on attachment 276267 [details]
Trimmed indicator image for Mac

Probably obvious, but should have mentioned this one goes in browser/themes/pinstripe/browser/tabbrowser/

Comment 27

[Reed Loden [:reed]]

Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.79; previous revision: 1.78
done
Checking in browser/themes/pinstripe/browser/tabbrowser/tabDragIndicator.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/tabbrowser/tabDragIndicator.png,v  <--  tabDragIndicator.png
new revision: 1.3; previous revision: 1.2
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.97; previous revision: 1.96
done
Checking in browser/base/content/tabbrowser.xml;
/cvsroot/mozilla/browser/base/content/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.243; previous revision: 1.242
done

Comment 28

[Wayne Woods]

Thanks!

Is there somewhere I should post to notify third-party theme creators? Since other themes will have to set the indicator margin, too.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #28)
> Is there somewhere I should post to notify third-party theme creators? Since
> other themes will have to set the indicator margin, too.

I asked Sheppy, and he suggested creating a http://developer.mozilla.org/en/docs/Theme_changes_in_Firefox_3 similar to the existing http://developer.mozilla.org/en/docs/Theme_changes_in_Firefox_2 . If you could write up a short summary of the changes needed by theme authors and put them there that'd be great!

Comment 30

[Wayne Woods]

Sorry, having computer issues. Haven't been on much. I've added http://developer.mozilla.org/en/docs/Theme_changes_in_Firefox_3 and based it on the previous release page. Feel free to adjust it if you like :)