613749 - Fix tab-modal prompt sizing (min/max height/width & overflow)

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

[Josh Tumath]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101120 Firefox/4.0b8pre
Build Identifier: 

When there is a lot of text in an alert, it should expand like the old ones did. To try this out, open the alert on the Acid3 test.

http://acid3.acidtests.org/

Reproducible: Always

Comment 2

[Jim Jeffery not reading bug-mail 1/2/11]

Confirmed - scrolling long text boxes is counter productive to seeing the big picture in one step.

Comment 3

[Justin Dolske [:Dolske]]

This is deliberate, to help with the long-standing problem of bad pages creating huge dialogs (which might overflow the screen). We can look at tweaking the limit, though (iirc it's 10 lines, currently).

Comment 4

[gadjo]

There could be option to increase the size of the box containing the message (as it is done with textarea) - alert box with a really long message doesn't have to be initially very big, but user should have possibility to enlarge it

Comment 5

[Justin Dolske [:Dolske]]

Hmm, I removed the max-height from tabprompts.css (and even sprinkled in some flex=1), but the text still gets limited to ~4 lines. 

I suspect what's happening is that the <row> is getting stuck at it's initial size (as determined by the height of the icon next to the text), and then when we set the dialog's text the height is just being eaten up by the |overflow-y: auto|. Indeed, if I also remove the overflow-y all the text shows up.

Not quite sure what to do about this.

Comment 6

[Frank Yan (:fryn)]

Dolske referred this to me. I have an idea for the text layout. Will hack away at it this or next week.

Comment 7

[Marat Tanalin]

It may make sense to not limit height in exact count of _lines_.

Instead, it makes sense to make max height of alert dialog _dynamic_ and depending on current client height. It would be acceptable enough to make max height of alert dialog equal to 90% of document client height or so. Thanks.

Comment 8

[Eric Shepherd [:sheppy]]

(In reply to comment #7)

> Instead, it makes sense to make max height of alert dialog _dynamic_ and
> depending on current client height. It would be acceptable enough to make max
> height of alert dialog equal to 90% of document client height or so. Thanks.

This seems like a good way to go to me. Currently even fairly simple alerts are getting forced to scroll.

Comment 9

[Justin Dolske [:Dolske]]

Marking as blocking+, the current overflow suppression (via scrolling) is rather broken, and fryn's got an improved POC for how to make this work as expected.

Comment 10

[Nikita Popov]

The best solution in my eyes would be to enlarge the alert as long as it doesn't fill the whole client area and only then display a scroll bar.

Comment 12

[Frank Yan (:fryn)]

Attachment: patch v1

I could not find a non-hacky solution to establish a maximum height of the prompt that is a _percentage_ of the content area's height. All solutions I wrote involved querying the <notificationbox/> that is the parent of the parent of the prompt node and doing some iffy calculations. Even after performing those calculations, it seems that as long as the prompt text contains a long substring contains no word separator characters, the prompt width will not shrink below the necessary width to fit that substring, even if a maximum width is set.

These issues aside, the patch I wrote gets the prompt to expand to up to 600px in width or 400px in height and overflows beyond that. Since this is already significantly better than our current sizing behavior, I would advocate for landing this and going from there.

Comment 13

[Frank Yan (:fryn)]

Attachment: patch v2

Comment 14

[Frank Yan (:fryn)]

Comment on attachment 499962 [details] [diff] [review]
patch v2

This addresses the long substring issue of the previous patch and allows the prompt to expand almost to the dimensions of the content area.

Comment 15

[Ekanan Ketunuti]

Attachment: screenshot after patched v2.

screenshot after patched v2. test on http://www.scribd.com/. it's too big.

Comment 16

[Ekanan Ketunuti]

Attachment: patch v3

this patch work like a charm

Comment 17

[Frank Yan (:fryn)]

(In reply to comment #16)
> Created attachment 500637 [details] [diff] [review]
> patch v3
> 
> this patch work like a charm

This allows the prompt to enlarge beyond the size of the content area, when the size of the alert string is extremely large. Then, it becomes non-obvious how to close the prompt.

I'll revise my patch to address cases like scribd's long alert strings without line breaks.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 499962 [details] [diff] [review]
patch v2

>+.infoContainer:not(.overflow) {

This is not how we usually use classes. Please use an attribute instead.

Comment 20

[Ed Morley [:emorley]]

Since the screenshot attached is not of a textbox prompt, not sure if this is already fixed by the patch - but from the bug that has just been made a dupe of this:

(From bug 620300 comment 2)
> Reduced STR:
> - Type the following into the address bar and hit enter:
> javascript:prompt('Test');
> 
> Expected:
> - A minimum textbox width that is comparable to 3.6.13's width.
> 
> Actual:
> - Image and margin on new tab modal prompts takes up a lot more room, meaning
> the textbox is much narrower for the same size prompt.
> - The size of the prompt's textbox seems dependant on the length of the prompt
> text. So therefore with short messages, the textbox is shown much narrower as
> the minimum textbox width is set too small.

ie - see screenshot of current vs 3.6.12:
https://bugzilla.mozilla.org/attachment.cgi?id=500737

Comment 21

[Daniel Kirsch]

Just for my understanding: Why was the dialog UI changed? People are very used to the OS dialogs. They have a titlebar which makes it clear and intuitive how to move the alert. Moving the dialog is an important feature as the message in the dialog might hide some content the dialog is referring to. 
The dialog was correctly sized depending on the displayed content.

For me, native (looking) dialogs are part of a good user experience. The new dialogs don't have a single advantage over the old ones as far as I can see but a couple of disadvantages. And they even look more unfriendly (but that's definitely a matter of taste).

Comment 22

[Ed Morley [:emorley]]

(In reply to comment #21)
> Just for my understanding: Why was the dialog UI changed? 
See the "remove modal dialogs" section here: http://limi.net/articles/papercuts/

Also see bug 59314 for both reasoning and for outstanding issues (eg request for them to be made movable) by looking at the dependency list.

Comment 24

[Frank Yan (:fryn)]

Attachment: patch v3

This patch enables the prompts to expand naturally based on size of their contents and dynamically resizes the prompt when the content area is resized. It caps the prompt dimensions at 60% width and 60% height of the content area. When the prompt text is too long to fit in that space, scrollbars are used.

The patch also removes the icons from the prompts. If you'd like me to separate the patch into 2 parts, I'm happy to do that.

>+if (info.clientWidth > maxWidth || info.clientHeight > maxHeight) {
>+    info.style.overflow = "auto";
>+    info.style.width = maxWidth + "px";
>+    info.style.height = maxHeight + "px";
>+}

This cannot be trivially split into:

if (info.clientWidth > maxWidth) {
    info.style.overflow = "auto";
    info.style.width = maxWidth + "px";
}
if (info.clientHeight > maxHeight) {
    info.style.overflow = "auto";
    info.style.height = maxHeight + "px";
}

because vertical scrollbars unnecessarily appear due to layout not taking into account line-height when auto-sizing the container, so it underestimates the height needed for text with line-height > 1. (line-height is 1.4 for the prompts, at least on OS X.)

Comment 25

[Frank Yan (:fryn)]

By the way, I initially considered rewriting this in HTML, but it turned out that it wouldn't easily solve the sizing problems anyway, so the patch still uses XUL.

Comment 26

[Justin Dolske [:Dolske]]

Comment on attachment 504893 [details] [diff] [review]
patch v3

r- on this per discussed http://dolske.net/mozilla/tests/prompt/sizes.html testcase (shrinking window for "Wide" test makes is jump in height when the line starts to wrap).

Patch otherwise looks basically fine. A pure-CSS solution that didn't need JS/onresize would be ideal, but this'll do.

Comment 27

[Frank Yan (:fryn)]

Attachment: patch v4

Rounding was only half the problem. It turns out that <description/> has strange margins (top: 1px, right: 5px, bottom: 4px, left: 6px). I just zeroed those margins for <description class="info.body"/>, which made calculations easier, and the patch now works perfectly on my Mac and Windows machines.

Comment 28

[Frank Yan (:fryn)]

Attachment: patch v5

A bit of cleanup. No behavior change from v4.

Comment 29

[Frank Yan (:fryn)]

Attachment: patch v6

This revision enables the prompt to expand independently along each axis as necessary. It also ensures that the onResize handler doesn't recursively trigger itself (which broke the scrollbars in v5).

Comment 30

[Frank Yan (:fryn)]

Attachment: patch v7

unbitrotted!

Comment 31

[Justin Dolske [:Dolske]]

Comment on attachment 506982 [details] [diff] [review]
patch v7

>+++ b/toolkit/components/prompts/content/tabprompts.xml
>                 infoTitle          : getElement("info.title"),
>-                infoIcon           : getElement("info.icon"),
>                 checkbox           : getElement("checkbox"),

Instead of removing this line, just set infoIcon to null (that way it's more obvious that something is missing here, compared to commonDialog.js).


>+                this.onResize();
>+                window.addEventListener("resize", this, false);

Group the addEL call with the others a few lines up.


>+        <method name="onResize">
>+            <body>
>+            <![CDATA[
>+                let container = this.parentNode.parentNode;

... // the <tabpanel>

>+                let availWidth = container.clientWidth;
>+                let availHeight = container.clientHeight;
>+                if (availWidth == this.availWidth && availHeight == this.availHeight)
>+                    return;

Why cache and check these? (I think you mentioned getting multiple resize events? Sure you're not just getting bubbling events from a child of the prompt?)

>+                let self = this;
>+                function getElement(anonid) {
>+                    return document.getAnonymousElementByAttribute(self, "anonid", anonid);
>+                }

Hmm. I guess we could now do |function helper() { ... }.bind(this);|, but let's just stay with common syntax for now (as you have).

>+                let main = getElement("mainContainer");
>+                let top = getElement("topContainer");
>+                let info = getElement("infoContainer");

One more space and these line up in a pleasing way. ;)


>+                // cap prompt dimensions at 60% width and 60% height of content area
>+                let maxWidth = Math.floor(availWidth * 0.6 + top.clientWidth - main.clientWidth);
>+                let maxHeight = Math.floor(availHeight * 0.6 + top.clientHeight - main.clientHeight);

Why adjust to 60% + prompt overhead? Why not just 60%?

r+ with nits/explanations fixed. :)

Comment 32

[Frank Yan (:fryn)]

(In reply to comment #31)
> Comment on attachment 506982 [details] [diff] [review]
> patch v7
>
> >+        <method name="onResize">
> >+            <body>
> >+            <![CDATA[
> >+                let container = this.parentNode.parentNode;
> 
> ... // the <tabpanel>

It's the <notificationbox>. I'll add the comment. This isn't ideal, since toolkit/ shouldn't need to know this about browser/, but it's late in the game. (/toolkit/components/console/hudservice/ has this type of assumption too.) We can address these later.

> >+                let availWidth = container.clientWidth;
> >+                let availHeight = container.clientHeight;
> >+                if (availWidth == this.availWidth && availHeight == this.availHeight)
> >+                    return;
> 
> Why cache and check these? (I think you mentioned getting multiple resize
> events? Sure you're not just getting bubbling events from a child of the
> prompt?)

It seems to fire an event on the chrome window and another event on the content window and sometimes several on each when resizing quickly.

> >+                // cap prompt dimensions at 60% width and 60% height of content area
> >+                let maxWidth = Math.floor(availWidth * 0.6 + top.clientWidth - main.clientWidth);
> >+                let maxHeight = Math.floor(availHeight * 0.6 + top.clientHeight - main.clientHeight);
> 
> Why adjust to 60% + prompt overhead? Why not just 60%?

It actually adjusts to 60% without prompt overhead by accounting for the prompt overhead outside of the <vbox/> that contains the <description/>.
In other words, top.clientWidth - main.clientWidth is less than zero.

Comment 33

[Justin Dolske [:Dolske]]

Comment on attachment 506982 [details] [diff] [review]
patch v7

Oops! Still has weird resizing jumps when the window shrinks, Frank can repro now and is looking into it. :)

Comment 34

[Frank Yan (:fryn)]

Attachment: patch v8

Addressed review comments from v7 and fixed sizing issue.

Comment 35

[Frank Yan (:fryn)]

Attachment: patch v9

Removed unnecessary, bogus |if|.

Comment 36

[Justin Dolske [:Dolske]]

Comment on attachment 507356 [details] [diff] [review]
patch v9

>+        <method name="onResize">
>+            <body>
>+            <![CDATA[
>+                // XXX the <notificationbox/>; to be made app-agnostic later
>+                let container = this.parentNode.parentNode;

File a followup bug and list it here.

\o/

Comment 37

[Frank Yan (:fryn)]

Pushed.

http://hg.mozilla.org/mozilla-central/rev/b5314bc1a926

(In reply to comment #36)
> File a followup bug and list it here.

Filed and listed.

Comment 38

[Alfred Kayser]

Why is the icon removed?

Comment 39

[Justin Dolske [:Dolske]]

Removing the icon is part of the updated design plan (bug 613704), and it simplified things to just do it here.

Comment 40

[Aleksei Perepelkin [:chaos]]

Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre

Comment 41

[mcdavis941 (sporadically reading bugmail)]

>  .info\.body {
> -  max-width: 45em;
> +  margin: 0 !important;

If you remember, could you say why you needed to use !important to set the margin?  Does that have to do with some stylesheets coming from the tabprompt binding but global.css coming from a different source?