601534 - Rewrite window resize handler

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

I propose modifying the window resize handler so that Items are only rearranged if the window actually became smaller such that the top-level Items no longer fit in the Panorama screen.

This will bring the behavior in line with the "all-windows" syncing behavior we were working on previously, and we may bring back over in 4.next. It also fixes many cases of unnecessary resizing.

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

Also, try to take advantage of trench data when we do have to resize.

Comment 3

[Ian Gilman [:iangilman]]

So it would only scale down when absolutely necessary and never scale up? 

I'm concerned that it would be easy to squish everything down really small (by resizing your window down) and then no good way to get it back to normal size (since resizing the window up will just add extra space). 

This is different from our proposed "all windows" behavior, where a "universal size" would always exist, and we'd scale down when we were below it, but we'd scale back up when we had the space.

What problem are we trying to solve here?

Comment 4

[Ian Gilman [:iangilman]]

Just took a look at bug 596871; I would think this would make that problem even worse... they're not complaining about when it resizes, but how it does. 

At any rate, I agree the resizing behavior is funky; just want to make sure we actually improve it rather than trading one problem for another.

Comment 5

[Aza Raskin [:aza]]

Mitcho, let me know if you need a better spec for this.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #5)
> Mitcho, let me know if you need a better spec for this.

Yes please.

Comment 7

[Aza Raskin [:aza]]

Attachment: First revision of the resize the window spec

Comment 8

[Ian Gilman [:iangilman]]

(In reply to comment #7)
> Created attachment 487880 [details]
> First revision of the resize the window spec

Sounds great! This is very ambitious, though... it's basically what we tried to do for the coordinates in "all windows", and we found all sorts of edge cases and gotchas. On the other hand, knowing what we've learned from that, maybe it'll go smoother this time around. Certainly having it in place will pave the way nicely for "all windows". Also, it'll provide a solution for bug 587140: if things get so tight we don't have room, resize the "canonical bounds" so everything squishes down. 

A couple of bits not spelled out in the spec (though implied, I think): 

* If the window is smaller than the canonical bounds and you then resize back up, everything will scale back up until you hit the canonical bounds, at which point you'll just start getting more space.

* If you resize up past the canonical bounds and then move an item out into the newly created space, the canonical bounds would be adjusted to now include that new item (just like the canonical bounds adjust down to fit the items as spelled out in the spec).

Aza, agreed?

Mitcho, what do you think, both in terms of desirability and risk?

Comment 9

[Aza Raskin [:aza]]

Agreed, Ian. Let's wait for Mitcho's response.

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #7)
> Created attachment 487880 [details]
> First revision of the resize the window spec

I think, while there are trade-offs, this will result in much smoother "resizing" behavior and . The "canonical bounds" here is basically the same notion as the "universal bounds" I worked on on the all-windows branch, except, in this case, window-specific. The scaling code and resizing code should be easy to port from there.

One thing that confused/bothered me with the spec: this box:
http://img.skitch.com/20101104-xi1iq5b17n72qpci7msu8nc86r.png

How did a resizing of the window trigger this? Is the idea that the aspects of each of the groups stay the same, but we *do* shuffle group positions on resize? My understanding of the spec (and what we did in the all-windows work) was to never change the relative bounds/positions of top level items at all.

(In reply to comment #8)
> * If the window is smaller than the canonical bounds and you then resize back
> up, everything will scale back up until you hit the canonical bounds, at which
> point you'll just start getting more space.

That's my understanding and, again, what we had in the all-windows world. We didn't get around to polishing this, though... for example, if you have just one group in the bottom right corner and suddenly close that, we shouldn't just jump to the "larger resolution"... we should probably do some animation in that case. Anyway, that's for another ticket. :)

> * If you resize up past the canonical bounds and then move an item out into the
> newly created space, the canonical bounds would be adjusted to now include that
> new item (just like the canonical bounds adjust down to fit the items as
> spelled out in the spec).

Yes, though this shouldn't be visible, as the canonical:presented ratio should never be less than 1:1. (Again, what we did with all-windows.)

One last thought: do we want to let users "push" the edge to trigger this zooming out and creation of new space, or should this rescaling only be triggered by physical constraints (i.e., the bug 587140 scenario)? I would personally recommend the latter, but this is an implementation detail that we could try both ways and ui-review later.

tl;dr: YES.

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #10)
> (In reply to comment #7)
> > Created attachment 487880 [details] [details]
> > First revision of the resize the window spec
> 
> I think, while there are trade-offs, this will result in much smoother
> "resizing" behavior and .

LOL. That was "and indeed could be a good first step for later adding the all-windows functionality in fx 4.next."

Comment 12

[Ian Gilman [:iangilman]]

(In reply to comment #10)
> One thing that confused/bothered me with the spec: this box:
> http://img.skitch.com/20101104-xi1iq5b17n72qpci7msu8nc86r.png
> 
> How did a resizing of the window trigger this? Is the idea that the aspects of
> each of the groups stay the same, but we *do* shuffle group positions on
> resize? My understanding of the spec (and what we did in the all-windows work)
> was to never change the relative bounds/positions of top level items at all.

Yes, that confused me at first as well. I believe Aza's point there is that if the items were already in that alternate configuration, that the canonical bounds would be that dotten line, whereas if the items had already been in the previous configuration, that the canonical bounds would be the dotted line shown in the previous image. In other words, the canonical bounds snap to the items. 

Hey, I just thought of another wrinkle... what if a Windows user drags the window bigger from the left side? Is it possible to even know? The easiest thing, of course, is to just treat that resize like any other, but presumably their intention is to open up some empty space on the left rather than on the right.

> One last thought: do we want to let users "push" the edge to trigger this
> zooming out and creation of new space, or should this rescaling only be
> triggered by physical constraints (i.e., the bug 587140 scenario)? I would
> personally recommend the latter, but this is an implementation detail that we
> could try both ways and ui-review later.

I think I prefer the latter as well, but like you say, the former is certainly something we can experiment with. 

> tl;dr: YES.

Right on... this should probably be your top priority then, so we can get it in soon enough to bake. Agreed?

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #12)
> > tl;dr: YES.
> 
> Right on... this should probably be your top priority then, so we can get it in
> soon enough to bake. Agreed?

Yup! Let's clarify some of those couple items with Aza first.

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

THE PLAN: Every Panorama instance will have a UI.canonicalGeometry object which implements a particular interface to keep a canonical frame rect. Item placement will be scaled with respect to this rect, using code which I wrote for the all-windows branch. However, in Firefox 4, UI.canonicalGeometry will be an object that is per-window.

In the future, when (if) we move to an all-windows model, we would then be able to swap the Universal Geometry module back in, and the canonical rect will magically be shared across windows. :)

STEP 1:
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/7b23a5bae154

Refactored some things in our all-windows branch to facilitate porting of its code to m-c for the purposes of this bug. This includes:

- changed gUniversalGeometry to instead be UI.canonicalGeometry
- marking private methods in TabViewUniversalGeometry as such
- changing most external references to "universal" and "universalized" to be "canonical" and "canonicalized"

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

STEP 2:
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/d5f3c835e3cd

Updated the all-windows branch again while porting things to m-c:
- s/_universalRect/_canonicalRect/ for consistency
- fixed a bug wherein an Item which was forcing us to scale down, when moved, does not trigger a reevaluation of the scaling factor. (Found this bug after porting things to m-c, and realized it was actually a bug in the original branch as well.)

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1, work in progress

This is my first work in progress patch. Most of the additions are straight out of the related changes in the all-windows branch. Some notable modifications include the addition of a canonicalizedBounds property to all Items... this is because the all-windows work assumed abstract group and abstract tab objects which could carry these bounds. For our immediate needs, however, we have a need to keep the local bounds as well as the canonicalized ones within each Item.

There seems to be an issue with how the scaling is computed (it doesn't zoom out enough), which is odd given that it's the same code as in the all-windows work, so that's definitely something I'll look into soon.

There are also a number of edge cases where it seems like redrawing the frame makes it pause and be unresponsive for a half second. You can't tell that it's unresponsive, UI-wise, except that the group you were dragging (for example) may move without taking its tabs along for a split second. There are surely other things to catch as well.

Comment 17

[Michael Yoshitaka Erlewine [:mitcho]]

Requesting blocking as this patch will be part of the solution to bug 587140 (blocking final+).

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2

Updated work in progress. Scaling works correctly now, and weird behavior in cases of pushing a group against a wall or corner are now thwarted. There might still some edge cases remaining, but those could be separate bugs.

Setting browser.panorama.display_canonical = true in about:config will show you the canonical frame.

Comment 19

[Ian Gilman [:iangilman]]

Comment on attachment 488778 [details] [diff] [review]
Patch v2

Looks like you're on the right path, but I'd like to recommend one major change: I believe the whole thing will ultimately be a lot cleaner if all of the Item geometry happened in the canonical space, translated to screen coordinates only when dealing with the DOM. This makes a very clear boundary, and all of the things like default sizes, determining when to stack, etc. just work correctly without having to be scaled.

Incidentally, when working with systems like this, I often use the terms "world" and "screen" to denote the two coordinate systems... a little less unwieldy than "canonical", and a bit more informative.

>-  setPosition: function Item_setPosition(left, top, immediately) {
>+  //   dontUpdateCanonicalRect - if true, won't try to update Canonical Rect
>+  setPosition: function Item_setPosition(left, top, immediately,
>+                                         dontUpdateCanonicalRect) {

Seems like instead of ending donUpdateCanonicalRect in this fashion, we should introduce "options" like in setBounds. Otherwise it's one step further down the road of opaque boolean flags. 

>-  setSize: function Item_setSize(width, height, immediately) {
>+  //   dontUpdateCanonicalRect - if true, won't try to update Canonical Rect
>+  setSize: function Item_setSize(width, height, immediately,
>+                                 dontUpdateCanonicalRect) {

Ditto. 

>-  contains: function Rect_contains(rect) {
>-    return (rect.left > this.left &&
>-            rect.right < this.right &&
>-            rect.top > this.top &&
>-            rect.bottom < this.bottom);
>+  contains: function(rect) {
>+    return (rect.left >= this.left &&
>+            rect.right <= this.right &&
>+            rect.top >= this.top &&
>+            rect.bottom <= this.bottom);

Add back the Rect_contains name.

>+  // Function: css
>+  // Scales the Rect by an x scale and a y scale.  
>+  scale: function Rect_scale(xscl, yscl) {

Fix the function name in the comment.

>+  // Function: getSafePageBounds
>+  // Returns a local <Rect> which corresponds to the Canonical <Rect>
>+  getSafePageBounds: function UI_getSafePageBounds() {

Why "safe"? Seems misnamed. I'd call it something like getScreenPageBounds.

>+  // Function: getScaledRect
>+  // Scales the given Canonical Geometry <Rect> to the current page bounds
>+  getScaledRect: function UI_getScaledRect(rect) {
>+    let canonicalRect = this.canonicalGeometry.getRect();
>+    canonicalRect.inset(-1,-1);
>+
>+    Utils.assert(canonicalRect.contains(rect),
>+                      'input must be within the Canonical Rect');

I don't think we want this assertion. At higher levels we can strive to keep things within the canonical rect, but at this level everything should work fine regardless. I guess what I'm saying is that while the user experience may be that everything stays in the window, that doesn't mean that none of our geometry will ever even briefly poke out of the window. Also note that being outside of the canonical rect is not the same as being outside of the window.

>+  // A padding value is added to each Item's bounds. This padding should be
>+  // equal to Trenches.defaultRadius
>+  defaultRadius: 10,

If this should be the same as Trenches.defaultRadius, then it seems they should be the same variable... either have one point at the other, or have them both point at some variable in UI.

>+  init: function UI_canonicalGeometry_init() {

Please add naturalDocs comments for these routines.

>+  getRect: function TVUG_getRect() {

TVUG?

>+UI.canonicalGeometry.init();
>+
> // ----------
> UI.init();

Please call canonicalGeometry.init from within UI.init; that way UI.init remains our single point of external initialization, in complete control of the initialization sequence of all of the other objects.

Comment 20

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #19)
> Comment on attachment 488778 [details] [diff] [review]
> Patch v2
> 
> Looks like you're on the right path, but I'd like to recommend one major
> change: I believe the whole thing will ultimately be a lot cleaner if all of
> the Item geometry happened in the canonical space, translated to screen
> coordinates only when dealing with the DOM. This makes a very clear boundary,
> and all of the things like default sizes, determining when to stack, etc. just
> work correctly without having to be scaled.

I'm really not sure about that. I think what actually is (was) the cleanest was to have the "abstract" tabs/groups (à la bug 578512) use world bounds but then have their associated Items use local screen bounds. Otherwise, if we want to only keep world bounds on the Items themselves, there would be a *lot* of places to do the scaling on the fly: any time we touch the DOM, and there are a lot of those. I think the benefits to not having to look at "scaled" values for snapping, minimum sizes, etc., would be outweighed by adding this conversion code to everywhere we interact with the DOM.

If you think it makes sense, what we could do is to save only the world bounds on the Item but then have getBounds do the scaling and return the local bounds each time, and then change all our direct references to .bounds to use getBounds. This could actually be a good cleanup move anyway.

Comment 21

[Ian Gilman [:iangilman]]

(In reply to comment #20)
> I'm really not sure about that. I think what actually is (was) the cleanest was
> to have the "abstract" tabs/groups (à la bug 578512) use world bounds but then
> have their associated Items use local screen bounds. 

Actually, I felt that was pretty messy, and I think it contributed to some of the weirdness we saw at the time. 

> Otherwise, if we want to
> only keep world bounds on the Items themselves, there would be a *lot* of
> places to do the scaling on the fly: any time we touch the DOM, and there are a
> lot of those. 

We're trying to limit our DOM access. Most everything that has to do with coordinates should already be going through the setBounds routines. The other big area is when we get mouse events, like dragging. Are there other things you can thing of?

> If you think it makes sense, what we could do is to save only the world bounds
> on the Item but then have getBounds do the scaling and return the local bounds
> each time, and then change all our direct references to .bounds to use
> getBounds. This could actually be a good cleanup move anyway.

Either way, I'm sure we'll want easy access to both the screen and world bounds. If world is the primary coordinate system, we can add a getScreenBounds.

Comment 22

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #21)
> (In reply to comment #20)
> > I'm really not sure about that. I think what actually is (was) the cleanest was
> > to have the "abstract" tabs/groups (à la bug 578512) use world bounds but then
> > have their associated Items use local screen bounds. 
> 
> Actually, I felt that was pretty messy, and I think it contributed to some of
> the weirdness we saw at the time. 

Oh, okay. :/

> > Otherwise, if we want to
> > only keep world bounds on the Items themselves, there would be a *lot* of
> > places to do the scaling on the fly: any time we touch the DOM, and there are a
> > lot of those. 
> 
> We're trying to limit our DOM access. Most everything that has to do with
> coordinates should already be going through the setBounds routines. The other
> big area is when we get mouse events, like dragging. Are there other things you
> can thing of?

Well, there are a number of places... All the arranges, snapping... But I'll give it a try. Maybe I'll end up with some useful abstractions/utils along the way.

Comment 23

[Ian Gilman [:iangilman]]

(In reply to comment #22)
> Well, there are a number of places... All the arranges, snapping... But I'll
> give it a try. Maybe I'll end up with some useful abstractions/utils along the
> way.

Arranging and snapping should happen in world coordinates, otherwise things will shift around in funny ways when you scale up or down.

Comment 24

[Dietrich Ayala (:dietrich)]

This hasn't made progress in quite a while, is blocking a large number of bugs, and looks like a pretty major rewrite of some core tabview code. I'm going to blocking- this, and probably revert blocking+ on some of the dependent bugs.

The level of risk here seems very high.

Comment 25

[Michael Yoshitaka Erlewine [:mitcho]]

For reference, this is next on my list of high priority items in the layout/geometry polish department.

Comment 26

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: WIP 12/20

WIP. A *lot* of weirdness to still sort out.

Since the last patch, I've made it so the .bounds are always World bounds, which are then scaled using UI.getScreenRect().

Ian, If you could give me some high-level feedback on the approach, I'd appreciate it.

Comment 27

[Ian Gilman [:iangilman]]

Comment on attachment 498889 [details] [diff] [review]
WIP 12/20

The approach (world coordinates in .bounds, using getScreenRect()) looks good. How are you liking it?

As you say, there's lots of stuff still to attend to: 

* There are still some vestiges of the old .worldBounds
* You haven't addressed any of my comments from comment 19 (except the big one)
* There are a number of places where there's confusion about whether a particular coordinate is screen or world (say, GroupItem.expanded.bounds)

I won't comment on the details (unless you want me to), as I assume you have it all under control. 

This is a risky change, as Dietrich says, so I've been thinking about ways we can mitigate the risk. One idea is to have a "fail-safe" mode where it just pins the scaling factor to 1. At the very least, we need to make sure everything's working properly at 1:1. Also, for testing, it might be handy to be able to pin the scale to something like 2/3rds, so we can isolate the scaling code from the resize/world bounds code.

Another thing to think about is whether we save the world bounds between sessions. That's kind of the purpose UI._pageBounds (which doesn't really need to be saved any more if we're not using it like that). Probably we should.

Comment 28

[Nochum Sossonko [:Natch]]

The dependent bugs this patch fixes are both glaring and easily reproducible (some of them at least). Re-requesting blocking.

Comment 29

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 30

[Kevin Hanes]

bugspam (removing b9)

Comment 31

[Ian Gilman [:iangilman]]

We're down to the last beta now; the window of opportunity for landing a patch as risky as this is closing, if not already closed.

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking: says "rewrite" right on the label.

Comment 33

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #32)
> Not blocking: says "rewrite" right on the label.

Note: "rewrite" here actually means "change to follow a different spec".

Comment 34

[Michael Yoshitaka Erlewine [:mitcho]]

As per discussion with Ian earlier today, this will officially be punted to the future (post-fx4). I will now check each of the bugs this blocks and see how this affects their status.

Comment 35

[Michael Yoshitaka Erlewine [:mitcho]]

A (much) simpler version of this bug is proposed as bug 625269.

Comment 36

[alntnm]

There's also another simpler possibility - allowing panorama workspace to be larger than window size. See bug 671810.

Comment 37

[Justin Dolske [:Dolske]]

Panorama has been removed from Firefox 45, currently in Beta and scheduled for release on March 7th. As such, I'm closing all existing Panorama bugs.

If you are still using Panorama, you will see a deprecation message in Firefox 44, and when 45 is released your tab group data will be migrated to bookmarks, with a folder for each group. There are also a few addons offering similar functionality.

See https://support.mozilla.org/en-US/kb/tab-groups-removal for more info.

We're removing Panorama because it has extremely low usage (about 0.01% of users), and has a large number of bugs and usability issues. The cost of fixing all those issues is far too high to justify, and so we'll instead be focusing our time and energy on improving other parts of Firefox.