Closed Bug 719318 Opened 8 years ago Closed 7 years ago

Default window size too tall for portrait-mode monitors.

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 22

People

(Reporter: Dolske, Assigned: waterlo1)

References

Details

(Whiteboard: [good first bug][mentor=dolske][lang=js])

Attachments

(2 files, 5 obsolete files)

Attached image OMG ORANGE SCREENSHOT
Just reinstalled my Windows 7 desktop, which has the monitor in portrait mode. On first-run of Firefox, the window fills about 4/5 of the screen width and the entire screen height, which seems excessive.
The default size is set in browser.js's BrowserStartup()...

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1322

1322   // Set a sane starting width/height for all resolutions on new profiles.
1323   if (!document.documentElement.hasAttribute("width")) {
1324     let defaultWidth = 994;
1325     let defaultHeight;
1326     if (screen.availHeight <= 600) {
1327       document.documentElement.setAttribute("sizemode", "maximized");
1328       defaultWidth = 610;
1329       defaultHeight = 450;
1330     }
1331     else {
1332       // Create a narrower window for large or wide-aspect displays, to suggest
1333       // side-by-side page view.
1334       if (screen.availWidth >= 1600)
1335         defaultWidth = (screen.availWidth / 2) - 20;
1336       defaultHeight = screen.availHeight - 10;
1337 #ifdef MOZ_WIDGET_GTK2
1338       // On X, we're not currently able to account for the size of the window
1339       // border.  Use 28px as a guess (titlebar + bottom window border)
1340       defaultHeight -= 28;
1341 #endif
1342     }
1343     document.documentElement.setAttribute("width", defaultWidth);
1344     document.documentElement.setAttribute("height", defaultHeight);
1345   }


My screen is 1200x1920 (wxh), so it only hits the case where the height is 10 pixels less than the screen.

It would also be nice to adjust the default *position* so that the window doesn't overlap the first column of desktop icons on the left (Windows) or right (OS X), not sure if there are additional complications with doing that.
Whiteboard: [good first bug][mentor=dolske][lang=js]
Would something like this help?
>if (screen.availWidth >= 1600)
>  defaultWidth = (screen.availWidth / 2) - 20;
>  if (screen.availHeight > screen.availWidth) { 
>    defaultHeight = (screen.availHeight / 2) - 20;
>  }
>  else {
>    defaultHeight = screen.availHeight - 10;
>  }
If you can point me to the code where default position is determined, I can take a look at that too.
jphan expressed interest in working on this bug.
Assignee: nobody → jphan9
How would I go about reproducing this bug short of uninstalling my installation of Firefox?

I tried deleting localstore.rdf but it only gave me the bug for a fraction of a second before it resized to what I'm assuming is the default window size for landscape mode for my resolution (1920x1200).
I'd also like to note that I did set my display to portrait mode before deleting "localstore.rdf".

jphan9 and I are working on this.
You could try creating a new profile - start the Firefox executable with the command line argument -P and create a new profile from the dialog that appears before the browser starts up.
Thanks, that's exactly what I needed to do.
What file should I be looking at?

I found browser.js in two places:

C:\Users\Marcos\mozilla-central\obj-i686-pc-mingw32\dist\bin\chrome\browser\content\browser

C:\Users\Marcos\mozilla-central\browser\base\content

My guess is that it's the one in the second directory (mozilla-central\browser) according to what I've been understanding in the guides.
obj-i686-pc-mingw32 is your build directory, so any changes you make to browser.js inside of it will be overwritten next time you build.
Would it be okay to include some minor calculations that include multiplication to calculate how much of the screen the browser takes up?

I'm not too sure, but it's my understanding that performing floating point multiplication costs are somewhat high for such a trivial calculation.

For example, instead of 

defaultHeight = screen.availHeight - 10;

I would do 

let defaultHeight = screen.availHeight * 1.3;

in order to obtain a defaultHeight  that takes up a percentage of the screen (as opposed to just obtaining a fixed size).
I changed the way default window sizes are calculated based on percentages of screen estate available. 

I also coupled the height and width sizes so that the window always keeps the same aspect ratio to prevent awkward behavior in extreme cases (i.e. triple monitor setups).

However, I'm not sure what the code below is for (what is "X"?), lines 1362-1366 in original code:

  1362 #ifdef MOZ_WIDGET_GTK2
  1363       // On X, we're not currently able to account for the size of the window
  1364       // border.  Use 28px as a guess (titlebar + bottom window border)
  1365       defaultHeight -= 28;
  1366 #endif
Attachment #619182 - Flags: review?(dolske)
Comment on attachment 619182 [details] [diff] [review]
First changes on how the window sizes are calculated.

I don't think my patch was very good; it's intent was just too far off of what was requested in the first place. 

jphan9 isn't working on this patch anymore either. It's best if this bug is reset to open to allow someone else the opportunity to work on it.
Attachment #619182 - Flags: review?(dolske)
Ok, thanks for notifying us!
Assignee: jphan9 → nobody
Is the "let defaultWidth = 994; let defaultHeight;" sane? can we use some expression related to availWidth and availHeight instead of a fixed value?
Attached patch Patch proposal (obsolete) — Splinter Review
Attachment #701336 - Flags: feedback?(jaws)
Assignee: nobody → waterlo1
Status: NEW → ASSIGNED
Comment on attachment 701336 [details] [diff] [review]
Patch proposal

Review of attachment 701336 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +1141,5 @@
> +      // For portrait monitors
> +      // Select a shorter window to suggest top-and-bottom page view
> +      else if (screen.availWidth <= screen.availHeight) {
> +        defaultWidth = screen.availWidth * .9;
> +        defaultHeight = defaultWidth * .75;

For top/bottom viewing, shouldn't defaultHeight be closer to 50%? With this it will be 67.5%.

@@ +1152,5 @@
> +      }
> +      // For normal monitors
> +      else {
> +        defaultWidth = screen.availWidth * .9;
> +        defaultHeight = screen.availHeight * .9;

I think 90% and maximized are too close in relative sizes. If the code reaches here then it is a restored window, and we should make it more obvious. Something like 75% x 75% may be better here.

@@ +1159,4 @@
>  #ifdef MOZ_WIDGET_GTK2
>          // On X, we're not currently able to account for the size of the window
>          // border.  Use 28px as a guess (titlebar + bottom window border)
>          defaultHeight -= 28;

I don't think we will always want to hit this code. As I'm reading the code right now, the defaultHeight will be changed even for maximized windows. I'm curious to hear what you think about this.

If we do decide to keep this change, then you will have to un-indent this code so it is more obvious that it will be hit by all code paths.
Attachment #701336 - Flags: feedback?(jaws) → feedback+
Attached patch Patch v2 from Brandon Waterloo (obsolete) — Splinter Review
Similar to previous patch, except incorporating the feedback from jaws on last proposal.
Differences:
--Now uses 75% x 75% for normal monitors, instead of previous 90% x 90%, per jaws' suggestion
--The GTK window height reduction has been returned to the ELSE block, so that it does not apply for tiny monitors

NOTE:
--For 3x4 and 10x16 portrait monitors, the actual height would factor to about 50% and about 42%, respectively; not 67.5%, since monitors aren't square.  No change was made here.
Attachment #701336 - Attachment is obsolete: true
Attachment #703531 - Flags: feedback?
Attachment #703531 - Flags: feedback? → feedback?(jaws)
Comment on attachment 703531 [details] [diff] [review]
Patch v2 from Brandon Waterloo

Review of attachment 703531 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. Dolske, what do you think?
Attachment #703531 - Flags: review?(dolske)
Attachment #703531 - Flags: feedback?(jaws)
Attachment #703531 - Flags: feedback+
Comment on attachment 703531 [details] [diff] [review]
Patch v2 from Brandon Waterloo

Review of attachment 703531 [details] [diff] [review]:
-----------------------------------------------------------------

Blech. I keep going back-and-forth on this. First-run UX is pretty important, and there are various edge cases in both the before/after versions of this code. But I think we can get a net improvement without trying to address all the existing issues. :)

This patch is basically there, just have the one comment and a style nit...

I hate to do style nits -- especially on a day when there are blog posts about best review practices! -- but I am going to anyway... :) The comment lines in between the if-else clauses make it really hard to read. I'd generally suggest either a single comment block at the top, moving the comment lines inside the blocks, or in just remove them entirely (in this case, the code is fairly simple and thus self-documenting).

::: browser/base/content/browser.js
@@ +1149,5 @@
> +        // For portrait monitors
> +        // Select a shorter window to suggest top-and-bottom page view
> +        if (screen.availWidth <= screen.availHeight) {
> +          defaultWidth = screen.availWidth * .9;
> +          defaultHeight = defaultWidth * .75;

I don't think this makes sense, it creates a very squat (landscape ratio) window.

The existing comment implies the landscape intent is side-by-side, although I think that's debatable. I think the stronger argument is that webpages tend to be designed for up-down scrolling, and not left-right; ergo a really wide browser just wastes space. But same net outcome.

For portrait-mode usage, which is relatively rare, I think the likely user intent is to get more vertical space to reduce up-down scrolling (see previous), and not to have top-and-bottom views. The problem is that defaulting to the entire height (as we currently do) feels like too much of a good thing. :)

Simply changing this line to:

  defaultHeight = screen.availHeight * .75;

would accomplish the goal of this bug as-filed.
Attachment #703531 - Flags: review?(dolske) → review-
New patch including dolske's feedback.  Changes default portrait height from 75% times the default width (so, .75 * .9 * available width) to just use simply 75% of available height.

Moves comment lines to beginning of block for readability.
Attachment #703531 - Attachment is obsolete: true
Attachment #704354 - Flags: feedback?(jaws)
Attachment #704354 - Flags: feedback?(dolske)
Comment on attachment 704354 [details] [diff] [review]
New patch including dolske's comments

Review of attachment 704354 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. I think this is ready for checkin, but I think it would be good to let Dolske take a look at it since he's expressed his opinion in this bug.
Attachment #704354 - Flags: review?(dolske)
Attachment #704354 - Flags: feedback?(jaws)
Attachment #704354 - Flags: feedback?(dolske)
Attachment #704354 - Flags: feedback+
Comment on attachment 704354 [details] [diff] [review]
New patch including dolske's comments

Review of attachment 704354 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +1159,1 @@
>            defaultWidth = (screen.availWidth / 2) - 20;

We should consider adjusting this too.
1600px / 2 - 20 = 780px, which is too narrow for many web pages, which require at least 1024px wide viewports, e.g. the 960px-wide grid pattern plus gutter space.
(google.com requires ~1000px; nytimes.com requires 987px.)
I'd suggest replacing 1600 with 2048px.

This shouldn't block the landing of this patch, of course.
This new patch includes dolske's previous comments (changing meaning of "widescreen" to mean >= 2048, not >= 1600, because most websites expect a horizontal resolution of ~1000).

Additionally, rearranged some comments to make more sense and actually be ordered like the file.
Attachment #704354 - Attachment is obsolete: true
Attachment #704354 - Flags: review?(dolske)
Attachment #712602 - Flags: feedback?(jwein)
Attachment #712602 - Flags: feedback?(dolske)
Attachment #712602 - Flags: feedback+
Comment on attachment 712602 [details] [diff] [review]
New patch with dolske's new comments

Review of attachment 712602 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +1283,5 @@
> +          defaultHeight = screen.availHeight * .75;
> +        }
> +        // Ensure no dimensions exceed available space
> +        defaultWidth = Math.min(defaultWidth, screen.availWidth);
> +        defaultHeight = Math.min(defaultHeight, screen.availHeight);

In which cases are these Math.min lines needed?
It looks mathematically impossible for this to ever occur.

The rest of the patch looks fine.
Attachment #712602 - Flags: feedback?(jwein)
Attachment #712602 - Flags: feedback?(dolske)
Attachment #712602 - Flags: feedback+
Attachment #619182 - Attachment is obsolete: true
Comment on attachment 712602 [details] [diff] [review]
New patch with dolske's new comments

Yeah, the Math.min() is no longer needed in this version of the patch.

r+ for landing without that! Thanks! (And sorry for the review delay.)
Attachment #712602 - Flags: review+
This version of the patch removes the unnecessary Math.min() lines.
Attachment #712602 - Attachment is obsolete: true
Attachment #712985 - Flags: review?(dolske)
Attachment #712985 - Flags: review?(dolske) → review+
Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0528e2561bde

Thanks for working on this Brandon! :)
OS: Windows 7 → All
Hardware: x86 → All
Target Milestone: --- → Firefox 21
Version: unspecified → Trunk
Backed out because this caused browser_social_chatwindow.js to start failing permanently on the Mac slaves (e.g. https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=957ede94acc3).

Not really your fault; we'll need to either fix or disable the busted parts of that test - this was just the most expedient/safest way to get inbound green again.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a20345bfe817
Looks like Win7 also changed size, and triggered failures in mochitest-1 (test_text_selection.html), mochitest-4 (test_plugin_mouse_coords.html), mochitest-5 (test_videocontrols.html), and probably browser-chrome (browser_newtab_focus.js), though it's harder to tell about browser-chrome because we only managed to find a slave and get around to running it once during the 12 rounds while your patch was in.
The browser_social_chatwindow.js test failure is a (relatively minor) bug in the chat window code that only happened to be seen once this new screen size was in place - and FWIW, I did manage to repro (and fix) the problem using this patch and a VM with an artificially restricted screen-size.

I've opened bug 840832 for this and uploaded a patch.  I've pushed the patch in this bug, plus the patch in that bug, to a try run at https://tbpl.mozilla.org/?tree=Try&rev=ffb3e2b9a30d, which will hopefully show no more failures in browser_social_chatwindow.js.  I haven't marked this bug as depending on that new bug as that seems a little unfair, but obviously someone else can do that if desired.
(In reply to Mark Hammond (:markh) from comment #30)
> I haven't marked
> this bug as depending on that new bug as that seems a little unfair, but
> obviously someone else can do that if desired.

Thanks for investigating this. Since we can't push this bug's patch until the chat code and test are fixed, setting the dependency will at least help us track the status. No rush, no blame. Let's just do it right. :)
Depends on: 840832
Looks like there's also extreme unhappiness in Win7 mochitest-other - the only one that's finished so far was https://tbpl.mozilla.org/php/getParsedLog.php?id=19686608&tree=Mozilla-Inbound, but there are a couple others which are still running after 5 or 5.5 hours, when it's generally a 30-45 minute suite, so something's hanging in a rather unpleasant way.
A more comprehensive try push to make sure that it is ready to land,
https://tbpl.mozilla.org/?tree=Try&rev=a69bc2287749
Flags: needinfo?(jAwS)
The try push looks good, no failures seem related to the new window size. I've pushed this to inbound, and will monitor the results there.

https://hg.mozilla.org/integration/mozilla-inbound/rev/65f79a842388
Flags: needinfo?(jAwS)
Target Milestone: Firefox 21 → Firefox 22
https://hg.mozilla.org/mozilla-central/rev/65f79a842388
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.