671243 - Empty text-field area goes outside of minimized group when hovering with mouse

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[George Carstoiu]

Attachment: 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]

Attachment: 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]

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]

Attachment: better patch

I think this is a better patch. Let me know if anything needs improvement.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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]

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] (inactive)]

(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]

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]]

(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]]

Attachment: Based on Teddy's patch

The group title takes up all the available space.

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: Based on Teddy's patch

A typo in the previous patch

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: 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]]

Passed Try!

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: 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] (inactive)]

Comment on attachment 565120 [details] [diff] [review]
v3

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

Cool, thanks!

Comment 17

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Pushed to try and waiting for the results
https://tbpl.mozilla.org/?tree=Try&rev=be788c57619a

Comment 18

[Raymond Lee [:raymondlee]]

Passed Try!

Comment 19

[Ed Morley [:emorley]]

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]]

https://hg.mozilla.org/mozilla-central/rev/9bf1262cc262