Last Comment Bug 632555 - Scale about:sessionrestore based on viewport height
: Scale about:sessionrestore based on viewport height
Status: RESOLVED FIXED
[inbound]
: polish
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Jez Ng [:int3]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-08 14:14 PST by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2011-08-06 02:51 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.40 KB, patch)
2011-07-31 06:06 PDT, Jez Ng [:int3]
no flags Details | Diff | Review
Patch v1 (1.40 KB, patch)
2011-07-31 06:08 PDT, Jez Ng [:int3]
paul: review-
Details | Diff | Review
Patch v2 (5.39 KB, patch)
2011-08-03 00:19 PDT, Jez Ng [:int3]
no flags Details | Diff | Review
Patch v2 (5.44 KB, patch)
2011-08-03 00:31 PDT, Jez Ng [:int3]
paul: review+
dao+bmo: review+
Details | Diff | Review
Screenshot of pixel imperfections (22.21 KB, image/gif)
2011-08-03 02:31 PDT, Jez Ng [:int3]
no flags Details
Patch v3 (5.21 KB, patch)
2011-08-04 18:44 PDT, Jez Ng [:int3]
no flags Details | Diff | Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-02-08 14:14:26 PST
We have media queries so we can do something to make about:sessionrestore not suck so much in a tall window.
Comment 1 Jez Ng [:int3] 2011-07-31 06:06:33 PDT
Created attachment 549648 [details] [diff] [review]
Patch v1

I had a go at this using flexboxes. With this patch the treeview expands such that the whole errorPageContainer takes up 70% of the total height. I thought about making the treeview expand only if there were enough treechildren entries to fill it up, but I couldn't find an easy CSS-only way to do it.
Comment 2 Jez Ng [:int3] 2011-07-31 06:08:30 PDT
Created attachment 549649 [details] [diff] [review]
Patch v1

Removed an extra newline.
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-08-02 13:42:30 PDT
Comment on attachment 549649 [details] [diff] [review]
Patch v1

Something with the spacing is wrong and it just looks off. Ultimately not a huge deal, but it's obvious enough. The min width also appears to be different (resizing the window will reflow at different widths for nightly vs this patch).

Even if all of that was good, you're missing winstripe & gnomestripe changes.

Dão, perhaps you can give some pointers on making this polished? I'm not familiar with -moz-box and how that plays with everything else.
Comment 4 Jez Ng [:int3] 2011-08-03 00:19:33 PDT
Created attachment 550316 [details] [diff] [review]
Patch v2

Fixed the sizing issues. A quick explanation:

width:auto doesn't cause flexboxes to expand to fill their available width; instead we should use width:-moz-available.

Vertical margins don't collapse for flexboxes. I fixed this by manually calculating the 'real' margin widths in the original about:sessionrestore and using those values instead.

I've tested on OS X and Linux; will request review once I get a look at Windows. It's the same code across platforms, though.
Comment 5 Jez Ng [:int3] 2011-08-03 00:31:29 PDT
Created attachment 550317 [details] [diff] [review]
Patch v2
Comment 6 Jez Ng [:int3] 2011-08-03 02:31:24 PDT
Created attachment 550327 [details]
Screenshot of pixel imperfections

So to get a more precise comparison I took screenshots and overlaid them in Photoshop and I discovered some pixel imperfections.. probably the most significant one is that the treeview is now eight pixels narrower. Occurs on OS X and Windows, and I wouldn't be surprised if it was on Linux as well. The OS X overlaid-screenshot is attached. I think the differences are insignificant enough though.
Comment 7 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-08-03 15:45:05 PDT
Comment on attachment 550317 [details] [diff] [review]
Patch v2

Looks good to me, but I'd like to get a second pair of eyes on it just to make sure.

We could have an XP stylesheet in sessiontore/content since you're adding a fair bit more duplication, but personally, it's not a huge deal either way. I don't think we want to spend any more time on this.
Comment 8 Dão Gottwald [:dao] 2011-08-04 02:59:37 PDT
Comment on attachment 550317 [details] [diff] [review]
Patch v2

>+#errorLongContent {
>+  display: -moz-box;
>+  -moz-box-flex: 1;
>+  -moz-box-orient: vertical;
>+}
>+
>+#errorTrailerDesc {
>+  display: -moz-box;
>+  -moz-box-flex: 1;
>+  -moz-box-orient: vertical;
> }

These two rules can be merged.
Comment 9 Jez Ng [:int3] 2011-08-04 18:44:37 PDT
Created attachment 550926 [details] [diff] [review]
Patch v3

Good catch.
Comment 11 Marco Bonardo [::mak] 2011-08-06 02:51:45 PDT
http://hg.mozilla.org/mozilla-central/rev/4d2c4722547c

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