Last Comment Bug 671243 - Empty text-field area goes outside of minimized group when hovering with mouse
: Empty text-field area goes outside of minimized group when hovering with mouse
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 10
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks: 589132 617901 619055 627228 671240
  Show dependency treegraph
 
Reported: 2011-07-13 05:40 PDT by George Carstoiu
Modified: 2016-04-12 14:00 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Screenshot (7.32 KB, image/png)
2011-07-13 05:40 PDT, George Carstoiu
no flags Details
possible patch (4.98 KB, patch)
2011-07-15 02:11 PDT, Teddy Ni
no flags Details | Diff | Splinter Review
better patch (6.44 KB, patch)
2011-07-16 10:38 PDT, Teddy Ni
no flags Details | Diff | Splinter Review
Based on Teddy's patch (7.50 KB, patch)
2011-10-04 00:17 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
Based on Teddy's patch (7.50 KB, patch)
2011-10-04 00:20 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
Patch for checkin (5.91 KB, patch)
2011-10-04 10:48 PDT, Raymond Lee [:raymondlee]
ttaubert: review-
Details | Diff | Splinter Review
v3 (5.93 KB, patch)
2011-10-05 20:31 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (6.21 KB, patch)
2011-10-09 22:55 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description George Carstoiu 2011-07-13 05:40:11 PDT
Created attachment 545649 [details]
Screenshot

Mozilla/5.0 (X11; Linux i686; rv:7.0a2) Gecko/20110712 Firefox/7.0a2

The text-field of an unnamed minimized group goes outside of the group when hovering with mouse - see screenshot.

Steps to reproduce:
1. Enter Panorama
2. Create a group and minimize it
3. Click in the background in order take focus away from textfield
4. Hover with mouse over the naming text-field

Actual results:
 - the text-field goes outside of the group

Expected results:
 - the text-field does not go outside of the group
Comment 1 Teddy Ni 2011-07-15 02:11:50 PDT
Created attachment 546121 [details] [diff] [review]
possible patch

Additional Question:

What function does div.title-shield serve? Is it necessary to also add to it the 20px offset we add to input.name?
Comment 2 Teddy Ni 2011-07-15 06:32:43 PDT
Oh, wait. Just reviewing this today, I realized I left something I was using for testing in there that shouldn't be. Namely, the color of the place holder should still be transparent. I can fix this when I get home tonight.
Comment 3 Teddy Ni 2011-07-16 10:38:46 PDT
Created attachment 546339 [details] [diff] [review]
better patch

I think this is a better patch. Let me know if anything needs improvement.
Comment 4 Tim Taubert [:ttaubert] 2011-07-17 07:48:45 PDT
Comment on attachment 546339 [details] [diff] [review]
better patch

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

Thanks for the patch! The approach looks good but we can do all this a little bit easier using -moz-calc():

1) Please add those changes to browser/base/content/tabview/tabview.css as that affects all themes.
2) Let's remove GroupItem.adjustTitleSize() and every call to it because we won't need it anymore.
3) input.name should always take the whole available space (because of [2]):

input.name { width: -moz-available; }

4) .title-container should do the same (because of [2]). And let's subtract another 6px to leave some space between the input field and the close button.

.title-container { width: -moz-calc(100% - 16px - 6px - 6px); }

5) The comment in the css file should be adjusted because we have a minimum group size - so there's no need to make sure the width doesn't become negative.

Sorry for the little scope creep... If you upload a new patch I'll be happy to review it again!
Comment 5 Teddy Ni 2011-07-17 15:08:11 PDT
No worries, I just want this patched :) I'm in the process of addressing these comments.

In conjunction with bug 589132, though, I don't know if this is enough to fix the text field expansion. I'm trying with all types of width settings, but it doesn't appear that the input tag can size itself based on its value attribute. And if it does not, JS like adjustTitleSize() seems unavoidable. I hope I'm missing something.

I've tried some stuff with a textarea and a content editable div, but neither works very well on the whole.

What do you think?
Comment 6 Tim Taubert [:ttaubert] 2011-07-17 15:56:51 PDT
(In reply to comment #5)
> In conjunction with bug 589132, though, I don't know if this is enough to
> fix the text field expansion. I'm trying with all types of width settings,
> but it doesn't appear that the input tag can size itself based on its value
> attribute. And if it does not, JS like adjustTitleSize() seems unavoidable.

I don't think we need the automatic text field expansion. Why shouldn't the input field just take up all the available space? I think we can clean up the old behavior here - but let's ask UX.

@Limi: Do you think we should have smaller group title input fields that auto-expand when typing? Or should these rather always take up all available space?
Comment 7 Alfred Kayser 2011-07-25 07:43:52 PDT
Please, put the text-overflow:ellipsis from bug 589132 in this patch, instead of two small patches patching the same thing...
Comment 8 Raymond Lee [:raymondlee] 2011-08-18 20:35:45 PDT
(In reply to Tim Taubert [:ttaubert] from comment #6)
> (In reply to comment #5)
> > In conjunction with bug 589132, though, I don't know if this is enough to
> > fix the text field expansion. I'm trying with all types of width settings,
> > but it doesn't appear that the input tag can size itself based on its value
> > attribute. And if it does not, JS like adjustTitleSize() seems unavoidable.
> 
> I don't think we need the automatic text field expansion. Why shouldn't the
> input field just take up all the available space? I think we can clean up
> the old behavior here - but let's ask UX.
> 
> @Limi: Do you think we should have smaller group title input fields that
> auto-expand when typing? Or should these rather always take up all available
> space?

IMO, take up all available should be better.
Comment 9 Raymond Lee [:raymondlee] 2011-10-04 00:17:48 PDT
Created attachment 564473 [details] [diff] [review]
Based on Teddy's patch

The group title takes up all the available space.
Comment 10 Raymond Lee [:raymondlee] 2011-10-04 00:20:42 PDT
Created attachment 564475 [details] [diff] [review]
Based on Teddy's patch

A typo in the previous patch
Comment 11 Tim Taubert [:ttaubert] 2011-10-04 05:12:04 PDT
Comment on attachment 564475 [details] [diff] [review]
Based on Teddy's patch

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

Thank you for finishing that! r=me with the style definitions moved to the "global" tabview.css.

::: browser/themes/gnomestripe/browser/tabview/tabview.css
@@ +421,5 @@
> +.title-container {
> +  /* We want the title container to leave out the width and position of the
> +   * .close button. Keep an eye on LTR and RTL differences in .close. */
> +  width: -moz-calc(100% - 16px - 6px);
> +}

Please add those changes to browser/base/content/tabview/tabview.css, they are the same for all themes.

@@ +432,5 @@
>    -moz-margin-end: 0;
>    margin-bottom: 0;
>    -moz-margin-start: 3px;
>    padding: 1px;
> +  width: -moz-available;

Same here.
Comment 12 Raymond Lee [:raymondlee] 2011-10-04 10:48:18 PDT
Created attachment 564605 [details] [diff] [review]
Patch for checkin

(In reply to Tim Taubert [:ttaubert] from comment #11)
> Thank you for finishing that! r=me with the style definitions moved to the
> "global" tabview.css.
> 
> ::: browser/themes/gnomestripe/browser/tabview/tabview.css
> @@ +421,5 @@
> > +.title-container {
> > +  /* We want the title container to leave out the width and position of the
> > +   * .close button. Keep an eye on LTR and RTL differences in .close. */
> > +  width: -moz-calc(100% - 16px - 6px);
> > +}
> 
> Please add those changes to browser/base/content/tabview/tabview.css, they
> are the same for all themes.
> 
> @@ +432,5 @@
> >    -moz-margin-end: 0;
> >    margin-bottom: 0;
> >    -moz-margin-start: 3px;
> >    padding: 1px;
> > +  width: -moz-available;
> 
> Same here.

Updated

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=74a2cd60578c
Comment 13 Raymond Lee [:raymondlee] 2011-10-04 22:36:43 PDT
Passed Try!
Comment 14 Tim Taubert [:ttaubert] 2011-10-05 02:22:00 PDT
Comment on attachment 564605 [details] [diff] [review]
Patch for checkin

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

I just tried the patch locally and it's not quite ready, yet. We'd need to fix those issues before it's ready to land.

Another thing: if you enter a long group title, press "End" (if you're not already at the end of the text field) and then click out of the input field we have the ellipsis at the front.

To correct that we can add "self.$title[0].setSelectionRange(0, 0);" to the groupItem.$title's blur-handler.

::: browser/base/content/tabview/tabview.css
@@ +111,5 @@
>  
> +.title-container {
> +  /* We want the title container to leave out the width and position of the
> +   * .close button. Keep an eye on LTR and RTL differences in .close. */
> +  width: -moz-calc(100% - 16px - 6px);

That should be "100% - 16px - 6px - 6px" to leave some space between the input field and the close button.

@@ +117,5 @@
> +
> +input.name {
> +  text-overflow: ellipsis;
> +  width: -moz-available;
> +}

We should add a "input.name:focus { text-overflow: clip }" otherwise there is a strange effect when the text contained in the input field is too long and you select all of the text.
Comment 15 Raymond Lee [:raymondlee] 2011-10-05 20:31:34 PDT
Created attachment 565120 [details] [diff] [review]
v3

(In reply to Tim Taubert [:ttaubert] from comment #14)
> Comment on attachment 564605 [details] [diff] [review] [diff] [details] [review]
> Patch for checkin
> 
> Review of attachment 564605 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I just tried the patch locally and it's not quite ready, yet. We'd need to
> fix those issues before it's ready to land.
> 
> Another thing: if you enter a long group title, press "End" (if you're not
> already at the end of the text field) and then click out of the input field
> we have the ellipsis at the front.
> 
> To correct that we can add "self.$title[0].setSelectionRange(0, 0);" to the
> groupItem.$title's blur-handler.
> 

Added

> ::: browser/base/content/tabview/tabview.css
> @@ +111,5 @@
> >  
> > +.title-container {
> > +  /* We want the title container to leave out the width and position of the
> > +   * .close button. Keep an eye on LTR and RTL differences in .close. */
> > +  width: -moz-calc(100% - 16px - 6px);
> 
> That should be "100% - 16px - 6px - 6px" to leave some space between the
> input field and the close button.

Updated

> 
> @@ +117,5 @@
> > +
> > +input.name {
> > +  text-overflow: ellipsis;
> > +  width: -moz-available;
> > +}
> 
> We should add a "input.name:focus { text-overflow: clip }" otherwise there
> is a strange effect when the text contained in the input field is too long
> and you select all of the text.

Added
Comment 16 Tim Taubert [:ttaubert] 2011-10-08 08:39:18 PDT
Comment on attachment 565120 [details] [diff] [review]
v3

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

Cool, thanks!
Comment 17 Raymond Lee [:raymondlee] 2011-10-09 22:55:35 PDT
Created attachment 565869 [details] [diff] [review]
Patch for checkin

Pushed to try and waiting for the results
https://tbpl.mozilla.org/?tree=Try&rev=be788c57619a
Comment 18 Raymond Lee [:raymondlee] 2011-10-10 02:44:29 PDT
Passed Try!
Comment 19 Ed Morley [:emorley] 2011-10-10 07:45:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bf1262cc262

Thanks for the try runs + nicely formatted patches on all these bugs, makes doing checkin-neededs a lot less time consuming :-)
Comment 20 Marco Bonardo [::mak] 2011-10-11 02:26:14 PDT
https://hg.mozilla.org/mozilla-central/rev/9bf1262cc262

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