Tab aspect ratio gets changed on drag-drop orphaning

VERIFIED FIXED in Firefox 5

Status

defect
P2
normal
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: mitcho, Assigned: ttaubert)

Tracking

({regression, ux-consistency})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [visual][ux])

Attachments

(2 attachments, 8 obsolete attachments)

See attached video. I'm pretty sure this didn't happen before.

Nightly: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11pre) Gecko/20110202 Firefox/4.0b11pre

The tab thumbnail aspect ratio should be sacred.
Posted video Video
Presumably this is the result of bug 625666? Should one be a duplicate of the other?
Blocks: 627096
Priority: -- → P2
(Assignee)

Updated

8 years ago
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Version: unspecified → Trunk
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #513902 - Flags: review?(ian)
Comment on attachment 513902 [details] [diff] [review]
patch v1

>-    return Math.max(0, Math.max(TabItems.minTabHeight, height - titleSize)) * 
>+    return Math.max(0, Math.max(TabItems.minTabHeight, height + titleSize)) * 

Why do we need Math.max(0, ...) here? Tabitems.minTabHeight is always positive, no?

Same question goes for getHeightForWidth, too.
Attachment #513902 - Flags: feedback-
Also, in general: I'm not convinced that this is the only fix required to solve this bug. For example, try dragging a tab out of an expanded stack. This is a consistent way right now to mangle the tab aspect.

Because it's expanded, it should be displaying its title when you drag it out, but because it's isStacked, we call calcValidSize with hideTitle = true (see TabItem_setBounds). I think that's part of the issue here as well, at least for that STR.
Comment on attachment 513902 [details] [diff] [review]
patch v1

Please address Mitcho's comments.
Attachment #513902 - Flags: review?(ian)
Posted patch patch v2 (obsolete) — Splinter Review
(In reply to comment #5)
> Why do we need Math.max(0, ...) here? Tabitems.minTabHeight is always positive,
> no?
> 
> Same question goes for getHeightForWidth, too.

Removed.

(In reply to comment #6)
> Also, in general: I'm not convinced that this is the only fix required to solve
> this bug. For example, try dragging a tab out of an expanded stack. This is a
> consistent way right now to mangle the tab aspect.

Tested when dragging tabs out of a stacked group and an expanded stacked group. Works for both.
Attachment #513902 - Attachment is obsolete: true
Attachment #514072 - Flags: review?(ian)
Comment on attachment 514072 [details] [diff] [review]
patch v2

Does it need a test?
Attachment #514072 - Flags: review?(ian) → review+
This is testable so I should write one.
Tim, as we discussed briefly on IRC, please also add some better comments, particularly in the headers of _getHeightForWidth and _getWidthForHeight which makes clear whether the height values expected/being dealt with include title space or not.
(In reply to comment #11)
> Tim, as we discussed briefly on IRC, please also add some better comments,
> particularly in the headers of _getHeightForWidth and _getWidthForHeight which
> makes clear whether the height values expected/being dealt with include title
> space or not.

Sorry I forgot about that :/ Will add them with the next patch.
Posted patch patch v3 (obsolete) — Splinter Review
So this is a completely new (and correct) approach. I don't know why the former one worked but Mitcho's intuition was right :)
Attachment #514072 - Attachment is obsolete: true
Attachment #514103 - Flags: feedback?(ian)
Instead of setting the tab's height with every call to setBounds() we can just force the re-calculation once when removing the tabItem from a stacked group.
Attachment #514110 - Flags: feedback?(ian)

Updated

8 years ago
Attachment #514110 - Flags: feedback?(mitcho)

Updated

8 years ago
Attachment #514103 - Flags: feedback?(mitcho)
Comment on attachment 514110 [details] [diff] [review]
patch v3 (alternative approach)

I think I prefer this alternative approach. Is there an advantage to making the fix in patch v3 as well?

A couple nits:

>+      // force tab item resize if we're stacked. the tab's title will be visible
>+      // and that's why we need to recalculate its height.
>+      if (this._isStacked)

We should use the method this.isStacked() here instead of accessing _isStacked directly.

Also, empirical question: do we need this if the group is expanded? I don't think we do. I think we only need this if the group is stacked and not expanded. If that's the case, while it shouldn't hurt to run this regardless of expanded status, we might as well check for expanded-ness and only run when it's needed.

Do you want me to feedback the other patch as well? I guess that depends on your answer to my first question.
Attachment #514110 - Flags: feedback?(mitcho)
Attachment #514110 - Flags: feedback?(ian)
Attachment #514110 - Flags: feedback+
(In reply to comment #15)
> I think I prefer this alternative approach. Is there an advantage to making the
> fix in patch v3 as well?

Well I think v3b makes not much sense when v3 is applied. While v3b may have less performance impact, v3 will make sure this glitch will never happen when transitioning from stacked mode to non-stacked mode and the other way around - so that is a bit safer. What do you think? :)

> We should use the method this.isStacked() here instead of accessing _isStacked
> directly.

Ok, that's better.

> Also, empirical question: do we need this if the group is expanded? I don't
> think we do. I think we only need this if the group is stacked and not
> expanded. If that's the case, while it shouldn't hurt to run this regardless of
> expanded status, we might as well check for expanded-ness and only run when
> it's needed.

Yep, we need this, otherwise the tab aspect still gets invalid when dragging a tab out of an expanded stacked group.

> Do you want me to feedback the other patch as well? I guess that depends on
> your answer to my first question.

Please do if you think we should take the safer path or even do both.
[bugspam: betaN -> final]

Patch in progress, hopefully pretty trivial. Let's try to see if we can get it approved soon.
Blocks: 585689
No longer blocks: 627096
(Assignee)

Updated

8 years ago
Attachment #514103 - Flags: feedback?(mitcho)
Attachment #514103 - Flags: feedback?(ian)
Posted patch patch v4 (obsolete) — Splinter Review
Used the approach from v3b and added a test.
Attachment #514103 - Attachment is obsolete: true
Attachment #514110 - Attachment is obsolete: true
Attachment #514761 - Flags: feedback?(mitcho)
Comment on attachment 514761 [details] [diff] [review]
patch v4

Passed try (though it doesn't look like that because of the many oranges):

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=ead62885292d
(Assignee)

Updated

8 years ago
Attachment #514761 - Flags: review?(ian)
Comment on attachment 514761 [details] [diff] [review]
patch v4

Patch looks good. Comments on the test:

>+    return Math.floor(bounds.height / bounds.width * 100) / 100;

Is computing this to one decimal point a good metric? Should we have an "acceptable range" of wiggle room, like plus or minus 2% or something instead? Not sure...

>+  let dragTabItem = function (tabItem) {

>+    let aspect = getTabItemAspect(tabItem);

The expected tab aspect should be the standard one: TabItems.tabHeight/TabItems.tabWidth, not computed for each individual tab.

>+    EventUtils.synthesizeMouseAtCenter(container, {type: "mousedown"}, cw);
>+    for (let x = 200; x <= 400; x += 100)
>+      EventUtils.synthesizeMouse(doc, x, 100, {type: "mousemove"}, cw);
>+    is(getTabItemAspect(tabItem), aspect, "tabItem aspect didn't change");
>+
>+    EventUtils.synthesizeMouseAtCenter(container, {type: "mouseup"}, cw);
>+    is(getTabItemAspect(tabItem), aspect, "tabItem aspect didn't change");

Make sure that the tab actually (a) moved physically and (b) has the right parent (or no parent), both before and after the move.

>+  showTabView(function () {

This test has the potential to move things around, snap, etc. Use newWindowWithTabView instead.

>diff --git a/browser/base/content/test/tabview/browser_tabview_bug634158.js b/browser/base/content/test/tabview/browser_tabview_bug634158.js

>+      tabItem.setSize(200, 200, true);

What's this about?
Attachment #514761 - Flags: review?(ian)
Attachment #514761 - Flags: feedback?(mitcho)
Attachment #514761 - Flags: feedback-
Comment on attachment 514761 [details] [diff] [review]
patch v4

I like this fix better; I'm concerned about the performance impact with version 3.

Why the options.dontArrange check?

Please address Mitcho's test comments, of course.
Posted patch patch v5 (obsolete) — Splinter Review
(In reply to comment #20)
> Is computing this to one decimal point a good metric? Should we have an
> "acceptable range" of wiggle room, like plus or minus 2% or something instead?
> Not sure...

I use TabItems.tabAspect now and allow a variance of +-1%.

> The expected tab aspect should be the standard one:
> TabItems.tabHeight/TabItems.tabWidth, not computed for each individual tab.

Fixed (see above).

> Make sure that the tab actually (a) moved physically and (b) has the right
> parent (or no parent), both before and after the move.

Done.

> This test has the potential to move things around, snap, etc. Use
> newWindowWithTabView instead.

Fixed.

> >diff --git a/browser/base/content/test/tabview/browser_tabview_bug634158.js b/browser/base/content/test/tabview/browser_tabview_bug634158.js
> 
> >+      tabItem.setSize(200, 200, true);
> 
> What's this about?

This is because the .expander element has a specific size and if the tabItem gets too small this does not pass the bounds.contains() check. I rewrote that test to use newWindowWithTabView(), so we always get the same tab size and the test doesn't move things around.

(In reply to comment #21)
> Why the options.dontArrange check?

setBounds() causes a groupItem.arrange() call that we don't want at this point because the groupItem gets closed.
Attachment #514761 - Attachment is obsolete: true
Attachment #516229 - Flags: review?(ian)
Attachment #516229 - Flags: feedback?(mitcho)
Posted patch patch v6 (obsolete) — Splinter Review
Raised the variance to +-1.5% to let the patch for bug 634672 pass.
Attachment #516229 - Attachment is obsolete: true
Attachment #516322 - Flags: review?(ian)
Attachment #516322 - Flags: feedback?(mitcho)
Attachment #516229 - Flags: review?(ian)
Attachment #516229 - Flags: feedback?(mitcho)
Comment on attachment 516322 [details] [diff] [review]
patch v6

>+      if (!options.dontArrange && this.isStacked())
>+        item.setBounds(item.getBounds(), true, {force: true});

As discussed on IRC, please remove the options.dontArrange, and figure out what the error you were seeing was. 

The test looks good.
Attachment #516322 - Flags: review?(ian)
Attachment #516322 - Flags: review-
Attachment #516322 - Flags: feedback?(mitcho)
No longer blocks: 585689
Blocks: 603789
Posted patch patch v7 (obsolete) — Splinter Review
(In reply to comment #25)
> As discussed on IRC, please remove the options.dontArrange, and figure out what
> the error you were seeing was.

The condition is now (item.isDragging && this.isStacked()) because we only need to cover the case when dragging a tabItem out of a stacked groupItem (expanded and not expanded).

The error occured because setBounds() was called on an unlinked tabItem when groupItem.remove(unlinkedTabItem) was called.
Attachment #516322 - Attachment is obsolete: true
Attachment #522054 - Flags: review?(ian)
Comment on attachment 522054 [details] [diff] [review]
patch v7

Looks good
Attachment #522054 - Flags: review?(ian) → review+
Attachment #522054 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 30

8 years ago
bugspam

Comment 31

8 years ago
http://hg.mozilla.org/mozilla-central/rev/22fe7cd1264b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox4.2

Comment 32

8 years ago
Verified issue on Mozilla/5.0 (X11; Linux i686; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre.
Status: RESOLVED → VERIFIED
Target Milestone: Firefox5 → Firefox 5
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.