Closed Bug 481914 Opened 15 years ago Closed 15 years ago

dialogs shouldn't be larger than available screen space

Categories

(Toolkit :: UI Widgets, defect)

ARM
Windows CE
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1
Tracking Status
fennec 1.0a1-wm+ ---

People

(Reporter: blassey, Assigned: blassey)

References

Details

(Keywords: mobile)

Attachments

(7 files, 11 obsolete files)

40.85 KB, image/png
Details
73.97 KB, image/png
Details
3.00 KB, patch
dougt
: review+
Details | Diff | Splinter Review
69.06 KB, image/png
Details
70.50 KB, image/png
Details
2.00 KB, patch
blassey
: review+
Details | Diff | Splinter Review
3.80 KB, patch
neil
: review+
Details | Diff | Splinter Review
Attached image screen shot
      No description provided.
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0a1-wm+
Assignee: nobody → bugmail
Comment on attachment 368291 [details] [diff] [review]
sets the maxWidth and maxHeight of the dialog to screen width and height

Does dialog.setAttribute("maxwidth"), screen.width); etc. not work?
Comment on attachment 368291 [details] [diff] [review]
sets the maxWidth and maxHeight of the dialog to screen width and height

Also, shouldn't you be using availWidth/Height?
Component: General → XUL Widgets
Product: Fennec → Toolkit
QA Contact: general → xul.widgets
Summary: slow/busy script dialog is big, ugly and unusable → dialogs shouldn't be larger than available screen space
Attachment #368291 - Attachment is obsolete: true
Attachment #368414 - Flags: review?(neil)
Attachment #368291 - Flags: review?(neil)
I guess I didn't transfer the build when I changed over to SetAttribute, but when I checked it again it was back to being too big.
Attachment #368414 - Attachment is obsolete: true
Attachment #368421 - Flags: review?(neil)
Attachment #368414 - Flags: review?(neil)
Comment on attachment 368421 [details] [diff] [review]
patch v.3, SetAttribute doesn't work

>diff --git a/toolkit/content/commonDialog.js b/toolkit/content/commonDialog.js
>+      if(window.screen.width < 640) {
>+      var buttons = document.getAnonymousElementByAttribute(dialog, "anonid", "buttons");
>+      buttons.style.maxWidth = (window.screen.availWidth - 100) +"px";
>+      }

I wonder if we could use CSS media queries for this part.
Comment on attachment 368421 [details] [diff] [review]
patch v.3, SetAttribute doesn't work

>diff --git a/toolkit/content/commonDialog.js b/toolkit/content/commonDialog.js

>+  if (dialog && dialog.style) {

These null checks should be unnecessary. Are you hitting cases where either of these is null?
(In reply to comment #7)
> (From update of attachment 368421 [details] [diff] [review])
> >diff --git a/toolkit/content/commonDialog.js b/toolkit/content/commonDialog.js
> 
> >+  if (dialog && dialog.style) {
> 
> These null checks should be unnecessary. Are you hitting cases where either of
> these is null?

No, that's just force of habit. I have no problem removing the check.
(In reply to comment #5)
> Created an attachment (id=368421)
> patch v.3, SetAttribute doesn't work
As per comment #2, the attribute name is all in lower case...
Comment on attachment 368421 [details] [diff] [review]
patch v.3, SetAttribute doesn't work

>+      if(window.screen.width < 640) {
>+	  var buttons = document.getAnonymousElementByAttribute(dialog, "anonid", "buttons");
>+	  buttons.style.maxWidth = (window.screen.availWidth - 100) +"px";
>+      }
It looks to me that this is trying to work around the fact that long strings cause the document to "overflow" the window. Maybe it would be better to give the #infoContainer an overflow style in toolkit/content/commonDialog.css?
(In reply to comment #9)
> (In reply to comment #5)
> > Created an attachment (id=368421) [details]
> > patch v.3, SetAttribute doesn't work
> As per comment #2, the attribute name is all in lower case...

Is setAttribute preferable? Out of curiosity, why?

(In reply to comment #10)
> (From update of attachment 368421 [details] [diff] [review])
> >+      if(window.screen.width < 640) {
> >+	  var buttons = document.getAnonymousElementByAttribute(dialog, "anonid", "buttons");
> >+	  buttons.style.maxWidth = (window.screen.availWidth - 100) +"px";
> >+      }
> It looks to me that this is trying to work around the fact that long strings
> cause the document to "overflow" the window. Maybe it would be better to give
> the #infoContainer an overflow style in toolkit/content/commonDialog.css?

It seems the buttons are laid out with a lot of padding for some reason.  Without this, having more than two buttons causes the window to be larger than the screen if the screen is less than 640px wide.  

As far as I can tell, the #infoContainer is the message part of the dialog, which isn't the part that's causing the trouble
Since the summary has been changed to include all dialogs and not just the slow/busy script one, I tried to log into a basic auth  page. Its fine in landscape, but we fall over in portrait

http://people.mozilla.org/~blassey/wm-screenshots/login-good.png
http://people.mozilla.org/~blassey/wm-screenshots/login-no-keyboard.png
http://people.mozilla.org/~blassey/wm-screenshots/login-portrait.png

The solution there may be a similar treatment of text boxes as buttons in the attached patch.  I'll wait for Neil to reply to my last comment before playing around with it though.  

Also, since there is no title bar to grab, the user cannot move the dialog to see the text box they're typing into when the soft keyboard comes up.

Finally, the files download dialog is unusable in both portrait and landscape:

http://people.mozilla.org/~blassey/wm-screenshots/filedownload-landscape.png
http://people.mozilla.org/~blassey/wm-screenshots/filedownload-portrait.png

We might just need an entirely different file download dialog. For reference, here are screen shots of pie' and opera's file download dialog:

http://people.mozilla.org/~blassey/wm-screenshots/ie-filedownload.png
http://people.mozilla.org/~blassey/wm-screenshots/opera-filedownload.png
Blocks: 477628
Status: NEW → ASSIGNED
Sorry for the delay...

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #5)
> > > Created an attachment (id=368421)
> > > patch v.3, SetAttribute doesn't work
> > As per comment #2, the attribute name is all in lower case...
> Is setAttribute preferable? Out of curiosity, why?
I'd like to take this opportunity to correct myself and switch to properties (dialog.maxWidth = screen.availWidth; [camelCase!]) which are better still.

> (In reply to comment #10)
> > (From update of attachment 368421 [details] [diff] [review]
> > >+      if(window.screen.width < 640) {
> > >+	  var buttons = document.getAnonymousElementByAttribute(dialog, "anonid", "buttons");
> > >+	  buttons.style.maxWidth = (window.screen.availWidth - 100) +"px";
> > >+      }
> > It looks to me that this is trying to work around the fact that long strings
> > cause the document to "overflow" the window. Maybe it would be better to give
> > the #infoContainer an overflow style in toolkit/content/commonDialog.css?
> It seems the buttons are laid out with a lot of padding for some reason. 
No, they're just centred on the dialog. You can demonstrate the effect by adding buttonpack="left" to the XUL (for testing only of course). If the window is too big then the buttons will simply reflect this.
This makes things a lot better, but there are still two problems I'm aware of.  The first is that some of the text gets cut off. I'm hoping Neil can shed some light on why that's happening.

The second is if the dialog pops up with the soft keyboard already up, it doesn't get the notification the the keyboard is popping up (and as a result the dialog is obscured by the keyboard.)  Lowering and raising the keyboard will put the dialog where it should be.
Attachment #368421 - Attachment is obsolete: true
Attachment #372009 - Flags: review?(neil)
Attachment #368421 - Flags: review?(neil)
Comment on attachment 372011 [details] [diff] [review]
implements a notification of changes to the soft keyboard

>diff --git a/widget/src/windows/nsWindow.cpp b/widget/src/windows/nsWindow.cpp
>+  if (observerService)
>+    {
<snip>
>+    }
>+}

Indent spacing

Nice "JSON"! I wonder if this is considered good or bad? I haven't seen data passed like that with observers.
(In reply to comment #14)
> The first is that some of the text gets cut off. I'm hoping Neil can shed some
> light on why that's happening.
Can you attach screen shots (mobile/desktop) of a reproducible test case?
I meant publicly reproducible, of course ;-)
Comment on attachment 372011 [details] [diff] [review]
implements a notification of changes to the soft keyboard

what mfinkle said.  great use of the |data| param.


Fix up the whitespace.  The curly brace should go on the same line.  Also, just keep do_getService on the same line too.

please file a bug against maemo.  we should probably do the same thing there.  Having consistent notifications is extra good.

Lastly add a comment about the format of that string.  It might be easy to see as a js developer, but not so much to C++ hackers.
Attachment #372011 - Flags: superreview?(emaijala)
Attachment #372011 - Flags: review?(doug.turner)
Attachment #372011 - Flags: review+
Attached patch widget patch v.2Splinter Review
This patch switches from using SHSipPreference to SipShowIM because the former is async and I wasn't able to reliably get the size of the IM window.  It also adds code to nsScreenWin::GetAvailRect() to take the size of IM window into account.
Attachment #372011 - Attachment is obsolete: true
Attachment #372550 - Flags: superreview?(emaijala)
Attachment #372550 - Flags: review?(doug.turner)
Attachment #372011 - Flags: superreview?(emaijala)
Attachment #372550 - Attachment is patch: true
Attachment #372550 - Attachment mime type: application/octet-stream → text/plain
review ping for Doug and Neil

Neil, I don't think any of these problems would be reproducible on a standard laptop or desktop screen since we cap the length of the text.  Perhaps adjust your screen resolution to VGA to reproduce?
(In reply to comment #22)
> Neil, I don't think any of these problems would be reproducible on a standard
> laptop or desktop screen since we cap the length of the text.  Perhaps adjust
> your screen resolution to VGA to reproduce?

I tried that and I tried changing my font settings and I just can't get a prompt to exceed the width of the screen, either on Windows XP or on a Fedora 7 VM that I dredge up for occasional Linux testing.

One possibility does occur to me though; can you try removing the min- prefix on the width and height in line 18 of commonDialog.xul?
Attached patch gets rid of min- (obsolete) — Splinter Review
this seems to work well for me.  Screen shot to follow.
Attachment #372009 - Attachment is obsolete: true
Attachment #373344 - Flags: review?(neil)
Attachment #372009 - Flags: review?(neil)
Comment on attachment 373344 [details] [diff] [review]
gets rid of min-

Just some nits

>+            var top = (rect.top +  (height - window.innerHeight)/2);
>+            window.moveTo( 0 , top);

nit: param spacing

>+        }
>+        }
>+    }
>+    };

nit: indenting

>+    
>+    
>+    var observerService = Components.classes["@mozilla.org/observer-service;1"]

nit: remove extra blank line

>     case 3:
>       dialog.getButton("extra1").hidden = false;
>+      if(window.screen.width < 640) {
>+      var buttons = document.getAnonymousElementByAttribute(dialog, "anonid", "buttons");
>+      buttons.style.maxWidth = window.screen.availWidth +"px";
>+      }

nit: if( spacing and indenting the block

>   }
> 
>+  if (dialog && dialog.style) {
>+      dialog.style.maxWidth = window.screen.availWidth +"px";
>+      dialog.style.maxHeight = window.screen.availHeight +"px";
>+      var textBoxes = document.getElementsByTagName("textbox");
>+      textBoxes.forEach = function(tb) {
>+      tb.style.maxWidth = window.screen.availWidth +"px";
>+      };
>+  }

nit: indenting again. 2 spaces not 4 and body of anon func should be indented
Attached patch Possible patch (obsolete) — Splinter Review
Actually I didn't mean for you to remove the width completely, as this makes the dialog look odd on bigger screens. But I retested in several scenarios, and changing the min-width to width isn't right either. (Don't ask me what the min-height is doing, it seems to have no effect...) So I came up with this version of the min-width removal, but, given the trouble I've had reproducing the bug, I'm not sure what effect it will have on a small screen. In the worst case I think we should be able to combine this with one of your earlier patches.
Attachment #373384 - Flags: review?(bugmail)
Neil, with your patch the dialog looks like the previous screen shots (too wide).  Is there a way to do this with media queries?
And what if you combine it with attachment 368291 [details] [diff] [review]?
that's what I tested.
Attached patch Another go (obsolete) — Splinter Review
If this doesn't work then I'm starting to suspect that it may be easier to limit the window's width at the widget layer.
Attachment #373384 - Attachment is obsolete: true
Attachment #373506 - Flags: review?(bugmail)
Attachment #373384 - Flags: review?(bugmail)
Attached patch combined patch (obsolete) — Splinter Review
this combines Neil's patch with my old one plus the listener for softkb changes. 

The dialog wastes a lot of space due to the small icon being in a column the size of the text field labels. Neil, do you have any objection to pulling the icon and text out of the grid and into a hbox as I did in the previous patch?
Attachment #373344 - Attachment is obsolete: true
Attachment #373506 - Attachment is obsolete: true
Attachment #373509 - Flags: review?(neil)
Attachment #373506 - Flags: review?(bugmail)
Attachment #373344 - Flags: review?(neil)
Attached image screen shot
this is the dialog with the latest patch
Attached patch Better stillSplinter Review
This is an iteration of my previous patch; it should display the correct window size although of course it doesn't consider the icon or keyboard.
Attachment #373513 - Flags: review?(bugmail)
Comment on attachment 373513 [details] [diff] [review]
Better still

I tried another variation which didn't work out so this seems to be the ideal combination of maximum and preferred width.

The spacer exists to set the preferred width of the dialog to 29em, but inside the hbox so that it doesn't affect the calculation of the preferred with of the text. The min-width is provided to stop it defaulting to the same as the preferred width, allowing the maxWidth to override it.

I haven't had a chance to look at your other changes yet although is it true that the icon now takes up the same amount of space whether or not there are any textboxes?
Attachment #373513 - Flags: review?(bugmail) → review+
Comment on attachment 373513 [details] [diff] [review]
Better still

This looks the same as the last patch.  I'd really like to do something about all that wasted space around the icon though. Perhaps it will have to be a follow up bug though.

I believe you only get that extra space when there are textboxes.  See the first screen shot on this bug for the case with no textboxes.
Attached patch softkb observer code (obsolete) — Splinter Review
this is the fourth patch I've been testing to pick up the softkb notifications from the widget layer.
Attachment #373509 - Attachment is obsolete: true
Attachment #373535 - Flags: review?(neil)
Attachment #373509 - Flags: review?(neil)
(In reply to comment #36)
> (From update of attachment 373513 [details] [diff] [review])
> This looks the same as the last patch.
There are some cases where the two patches will produce different results.

> I'd really like to do something about all that wasted space around
> the icon though. Perhaps it will have to be a follow up bug though.
Regarding that one aspect of the bug in particular, I really didn't like the way your patches were headed, which is why I worked on my version, but I can now go back and look at your changes for the icon and soft keyboard.
Attachment #373535 - Flags: review?(neil) → review-
Comment on attachment 373535 [details] [diff] [review]
softkb observer code

>+    
Nit: trailing spaces

>+  var softkbObserver = {
>+    dialog : document.documentElement,
Unused?

>+    QueryInterface: function (aIID) {
>+      if (aIID.equals(Ci.nsISupports) ||
>+          aIID.equals(Ci.nsIObserver))
>+        return this;
>+      throw Cr.NS_ERROR_NO_INTERFACE;
I didn't think Ci/Cr were defined here.

>+    },
>+    observe: function(subject, topic, data) {
>+      if (topic === "softkb-change") {
>+       var rect = JSON.parse(data);
Nit: incorrect indentation

>+       if (rect) {
>+         var height =  rect.bottom - rect.top;
>+         var top = (rect.top +  (height - window.innerHeight)/2);
>+         window.moveTo( 0 , top);
Nit: strange mixture of spacing styles.
Shouldn't you centre the window horizontally too in case you get a device with a wider window?

>+  var observerService = Components.classes["@mozilla.org/observer-service;1"]
>+    .getService(Components.interfaces.nsIObserverService);
>+  observerService.addObserver(softkbObserver, "softkb-change", false);
You need to remove this observer again otherwise it will leak.
Comment on attachment 372009 [details] [diff] [review]
pulls icon/text out of grid, observes "softkb-change"

>+  </hbox>
>   <grid>
Although I haven't actually tried it out, I would say that the icon change looks reasonable, except that I would have left a blank line in between the hbox and the grid.
in addition to fixing nits and adding an unload function I converted this file to use Ci, Cc and Cr.
Attachment #373535 - Attachment is obsolete: true
Attachment #373611 - Flags: review?(neil)
Comment on attachment 372550 [details] [diff] [review]
widget patch v.2

switching the sr to vlad since he's been looking at wince stuff lately (and he's actually a super-reviewer)
Attachment #372550 - Flags: superreview?(emaijala) → superreview?(vladimir)
Attachment #372550 - Flags: superreview?(vladimir) → superreview+
Comment on attachment 372550 [details] [diff] [review]
widget patch v.2

Changes look fine, but they need to be #ifdef WINCE_WINDOWS_MOBILE, not #ifdef WINCE
Comment on attachment 372550 [details] [diff] [review]
widget patch v.2

fix up what vlad mentioned.

ooc, the change from SHSipPreference -> SipShowIM.  Is this because you wanted to avoid the request queue?  What happens when a user goes between two edit boxes very quickly?  I think with the SHSipPreference, the delay allows you to keep the keyboard raised.
Attachment #372550 - Flags: review?(doug.turner) → review+
The delay meant that a new window wouldn't get the softkb notification.  This was a problem when the dialog comes up as the page loads in the case of a http auth, since the keyboard is up from the address bar.
Attachment #368291 - Attachment is obsolete: false
Attachment #368291 - Flags: review?(neil)
Attachment #373611 - Flags: review?(neil) → review+
Comment on attachment 373611 [details] [diff] [review]
softkb observer code v.2

>+        var height =  rect.bottom - rect.top;
>+        var width =  rect.right - rect.left;
>+        var top = (rect.top +  (height - window.innerHeight)/2);
>+        var left = (rect.left +  (width - window.innerWidth)/2);
Spacing is still odd here - all lines containing doubled spaces, but missing spaces around /s. You also don't need the outer sets of parentheses.

>+  var observerService = Cc["@mozilla.org/observer-service;1"]
>+    .getService(Ci.nsIObserverService);
...
>+      var prefs = Cc["@mozilla.org/preferences-service;1"]
>+                    .getService(Ci.nsIPrefBranch);
Inconsistent indentation style (sorry I don't know what the preferred style is offhand).

>         onload="commonDialogOnLoad();"
>+        onunload="commonDialogOnUnload()"
Nit: missing semicolon.
r=me with all the above fixed.
Attachment #368291 - Attachment is obsolete: true
Attachment #375636 - Flags: review?(gavin.sharp)
Attachment #368291 - Flags: review?(neil)
Comment on attachment 375636 [details] [diff] [review]
uses avail width/height, doesn't null check dialog and style

Looks ok, bug 357725 should help here too
Attachment #375636 - Flags: review?(gavin.sharp) → review+
(In reply to comment #47)
> Created an attachment (id=375636)
> uses avail width/height, doesn't null check dialog and style
Attachment 373513 [details] [diff] wasn't sufficient?
pushed:
http://hg.mozilla.org/mozilla-central/rev/80947d7fa565
http://hg.mozilla.org/mozilla-central/rev/a02e097367ab
http://hg.mozilla.org/mozilla-central/rev/d9bd4e76e241

attachment 375636 [details] [diff] [review] wasn't needed.

We still have the icon and description in a grid which isn't the most visually appealing thing.  However, dialogs generally fit in the screen space now, so I'm closing this bug out.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #375636 - Attachment is obsolete: true
(In reply to comment #50)
> We still have the icon and description in a grid which isn't the most visually
> appealing thing.
As per comment #40 I would have reviewed a patch based on attachment 372009 [details] [diff] [review].
(In reply to comment #52)
> 
> Would have liked to have you around!

Dude, chill. I've been offline due to some extremely annoying machine issues.

Thanks for being so helpful and understanding though.
Comment on attachment 373611 [details] [diff] [review]
softkb observer code v.2

>+        var left = (rect.left +  (width - window.innerWidth)/2);
You accidentally removed the second open bracket when you pushed this.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: