Closed
Bug 631752
Opened 14 years ago
Closed 14 years ago
Tab aspect ratio gets changed on drag-drop orphaning
Categories
(Firefox Graveyard :: Panorama, defect, P2)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 5
People
(Reporter: mitcho, Assigned: ttaubert)
References
Details
(Keywords: regression, ux-consistency, Whiteboard: [visual][ux])
Attachments
(2 files, 8 obsolete files)
(deleted),
video/ogg
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•14 years ago
|
||
Comment 2•14 years ago
|
||
Presumably this is the result of bug 625666? Should one be a duplicate of the other?
Blocks: 627096
Priority: -- → P2
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
Assignee | ||
Updated•14 years ago
|
Version: unspecified → Trunk
Assignee | ||
Comment 3•14 years ago
|
||
Attachment #513902 -
Flags: review?(ian)
Assignee | ||
Comment 4•14 years ago
|
||
Comment on attachment 513902 [details] [diff] [review]
patch v1
Passed try: http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=a68c8e825be6
Reporter | ||
Comment 5•14 years ago
|
||
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-
Reporter | ||
Comment 6•14 years ago
|
||
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 7•14 years ago
|
||
Comment on attachment 513902 [details] [diff] [review]
patch v1
Please address Mitcho's comments.
Attachment #513902 -
Flags: review?(ian)
Assignee | ||
Comment 8•14 years ago
|
||
(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 9•14 years ago
|
||
Comment on attachment 514072 [details] [diff] [review]
patch v2
Does it need a test?
Attachment #514072 -
Flags: review?(ian) → review+
Assignee | ||
Comment 10•14 years ago
|
||
This is testable so I should write one.
Reporter | ||
Comment 11•14 years ago
|
||
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.
Assignee | ||
Comment 12•14 years ago
|
||
(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.
Assignee | ||
Comment 13•14 years ago
|
||
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)
Assignee | ||
Comment 14•14 years ago
|
||
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•14 years ago
|
Attachment #514110 -
Flags: feedback?(mitcho)
Updated•14 years ago
|
Attachment #514103 -
Flags: feedback?(mitcho)
Reporter | ||
Comment 15•14 years ago
|
||
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+
Assignee | ||
Comment 16•14 years ago
|
||
(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.
Reporter | ||
Comment 17•14 years ago
|
||
[bugspam: betaN -> final]
Patch in progress, hopefully pretty trivial. Let's try to see if we can get it approved soon.
Assignee | ||
Updated•14 years ago
|
Attachment #514103 -
Flags: feedback?(mitcho)
Attachment #514103 -
Flags: feedback?(ian)
Assignee | ||
Comment 18•14 years ago
|
||
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)
Assignee | ||
Comment 19•14 years ago
|
||
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•14 years ago
|
Attachment #514761 -
Flags: review?(ian)
Reporter | ||
Comment 20•14 years ago
|
||
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 21•14 years ago
|
||
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.
Assignee | ||
Comment 22•14 years ago
|
||
(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)
Assignee | ||
Comment 23•14 years ago
|
||
Comment on attachment 516229 [details] [diff] [review]
patch v5
Passed try:
http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=8b6e59cbcec8
Assignee | ||
Comment 24•14 years ago
|
||
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 25•14 years ago
|
||
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)
Assignee | ||
Comment 26•14 years ago
|
||
(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)
Assignee | ||
Comment 27•14 years ago
|
||
Comment on attachment 522054 [details] [diff] [review]
patch v7
Passed try:
http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=c942a4196b2f
Comment 28•14 years ago
|
||
Comment on attachment 522054 [details] [diff] [review]
patch v7
Looks good
Attachment #522054 -
Flags: review?(ian) → review+
Assignee | ||
Comment 29•14 years ago
|
||
Attachment #522054 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 30•14 years ago
|
||
bugspam
Comment 31•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox4.2
Comment 32•14 years ago
|
||
Verified issue on Mozilla/5.0 (X11; Linux i686; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre.
Status: RESOLVED → VERIFIED
Updated•14 years ago
|
Target Milestone: Firefox5 → Firefox 5
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•