Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in mozilla2.0b11

Status

()

Toolkit
General
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: Josh Tumath, Assigned: fryn)

Tracking

Trunk
mozilla2.0b11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [hardblocker][fx4-fixed-bugday])

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

7 years ago
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
Duplicate of this bug: 613753
Confirmed - scrolling long text boxes is counter productive to seeing the big picture in one step.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

7 years ago
Blocks: 59314
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

7 years ago
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
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.
(Assignee)

Comment 6

7 years ago
Dolske referred this to me. I have an idea for the text layout. Will hack away at it this or next week.
Assignee: nobody → fryn
Status: NEW → ASSIGNED
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.
(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.
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.
blocking2.0: --- → final+

Comment 10

7 years ago
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.
Duplicate of this bug: 615253
(Assignee)

Comment 12

7 years ago
Created attachment 499935 [details] [diff] [review]
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.
Attachment #499935 - Flags: review?(dolske)
(Assignee)

Comment 13

7 years ago
Created attachment 499962 [details] [diff] [review]
patch v2
Attachment #499962 - Flags: review?
(Assignee)

Comment 14

7 years ago
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.
Attachment #499962 - Flags: review? → review?(dolske)
(Assignee)

Updated

7 years ago
Attachment #499935 - Attachment is obsolete: true
Attachment #499935 - Flags: review?(dolske)

Comment 15

7 years ago
Created attachment 500636 [details]
screenshot after patched v2.

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

Comment 16

7 years ago
Created attachment 500637 [details] [diff] [review]
patch v3

this patch work like a charm
(Assignee)

Comment 17

7 years ago
(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 on attachment 499962 [details] [diff] [review]
patch v2

>+.infoContainer:not(.overflow) {

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

Updated

7 years ago
Summary: Tab-modal alerts do not increase height when text overflows vertically → Fix tab-modal prompt sizing (min/max height/width & overflow)
(Assignee)

Updated

7 years ago
Attachment #499962 - Flags: review?(dolske)
(Assignee)

Updated

7 years ago
Duplicate of this bug: 620300
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

7 years ago
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).
(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.
Whiteboard: [hard blocker]
Whiteboard: [hard blocker] → [hardblocker]
(Assignee)

Updated

7 years ago
Blocks: 613704
Duplicate of this bug: 625726
(Assignee)

Comment 24

7 years ago
Created attachment 504893 [details] [diff] [review]
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.)
Attachment #499962 - Attachment is obsolete: true
Attachment #500636 - Attachment is obsolete: true
Attachment #500637 - Attachment is obsolete: true
Attachment #504893 - Flags: review?(dolske)
(Assignee)

Comment 25

7 years ago
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 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.
Attachment #504893 - Flags: review?(dolske) → review-
(Assignee)

Comment 27

7 years ago
Created attachment 505982 [details] [diff] [review]
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.
Attachment #504893 - Attachment is obsolete: true
Attachment #505982 - Flags: review?(dolske)
(Assignee)

Comment 28

7 years ago
Created attachment 506042 [details] [diff] [review]
patch v5

A bit of cleanup. No behavior change from v4.
Attachment #505982 - Attachment is obsolete: true
Attachment #506042 - Flags: review?(dolske)
Attachment #505982 - Flags: review?(dolske)
(Assignee)

Updated

7 years ago
Whiteboard: [hardblocker] → [hardblocker][needs review dolske]
(Assignee)

Comment 29

7 years ago
Created attachment 506964 [details] [diff] [review]
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).
Attachment #506042 - Attachment is obsolete: true
Attachment #506964 - Flags: review?(dolske)
Attachment #506042 - Flags: review?(dolske)
(Assignee)

Comment 30

7 years ago
Created attachment 506982 [details] [diff] [review]
patch v7

unbitrotted!
Attachment #506964 - Attachment is obsolete: true
Attachment #506982 - Flags: review?(dolske)
Attachment #506964 - Flags: review?(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. :)
Attachment #506982 - Flags: review?(dolske) → review+
(Assignee)

Comment 32

7 years ago
(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 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. :)
Attachment #506982 - Flags: review+ → review-
(Assignee)

Comment 34

7 years ago
Created attachment 507355 [details] [diff] [review]
patch v8

Addressed review comments from v7 and fixed sizing issue.
Attachment #506982 - Attachment is obsolete: true
Attachment #507355 - Flags: review?(dolske)
(Assignee)

Comment 35

7 years ago
Created attachment 507356 [details] [diff] [review]
patch v9

Removed unnecessary, bogus |if|.
Attachment #507355 - Attachment is obsolete: true
Attachment #507356 - Flags: review?(dolske)
Attachment #507355 - Flags: review?(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/
Attachment #507356 - Flags: review?(dolske) → review+
Whiteboard: [hardblocker][needs review dolske] → [hardblocker]
(Assignee)

Comment 37

7 years ago
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.
Blocks: 629318
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b11
Version: unspecified → Trunk

Comment 38

7 years ago
Why is the icon removed?
Removing the icon is part of the updated design plan (bug 613704), and it simplified things to just do it here.
Status: RESOLVED → VERIFIED
Whiteboard: [hardblocker] → [hardblocker][fx4-fixed-bugday]
Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
>  .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?
You need to log in before you can comment on or make changes to this bug.