The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 10

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: George Carstoiu, Assigned: raymondlee)

Tracking

Trunk
Firefox 10
Dependency tree / graph

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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?
Attachment #546121 - Flags: review?(tim.taubert)

Comment 2

6 years ago
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.

Updated

6 years ago
Attachment #546121 - Flags: review?(tim.taubert)

Comment 3

6 years ago
Created attachment 546339 [details] [diff] [review]
better patch

I think this is a better patch. Let me know if anything needs improvement.
Attachment #546121 - Attachment is obsolete: true
Attachment #546339 - Flags: review?(tim.taubert)
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!
Attachment #546339 - Flags: review?(tim.taubert)
Blocks: 671240
Blocks: 617901

Comment 5

6 years ago
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?
(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

6 years ago
Please, put the text-overflow:ellipsis from bug 589132 in this patch, instead of two small patches patching the same thing...
(Assignee)

Updated

6 years ago
Blocks: 627228
(Assignee)

Comment 8

6 years ago
(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.
Keywords: uiwanted
(Assignee)

Updated

6 years ago
Blocks: 619055

Updated

6 years ago
No longer blocks: 627228
(Assignee)

Comment 9

6 years ago
Created attachment 564473 [details] [diff] [review]
Based on Teddy's patch

The group title takes up all the available space.
Attachment #564473 - Flags: review?(ttaubert)
(Assignee)

Comment 10

6 years ago
Created attachment 564475 [details] [diff] [review]
Based on Teddy's patch

A typo in the previous patch
Attachment #564473 - Attachment is obsolete: true
Attachment #564475 - Flags: review?
Attachment #564473 - Flags: review?(ttaubert)
(Assignee)

Updated

6 years ago
Attachment #564475 - Flags: review? → review?(ttaubert)
Blocks: 589132
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.
Attachment #564475 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 12

6 years ago
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
Attachment #564475 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
Passed Try!
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Attachment #546339 - Attachment is obsolete: true
Keywords: checkin-needed, uiwanted
Hardware: x86 → All
Attachment #564475 - Flags: review+
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.
Attachment #564605 - Flags: review-
(Assignee)

Comment 15

6 years ago
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
Attachment #564605 - Attachment is obsolete: true
Attachment #565120 - Flags: review?(ttaubert)
Comment on attachment 565120 [details] [diff] [review]
v3

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

Cool, thanks!
Attachment #565120 - Flags: review?(ttaubert) → review+
Blocks: 627228
(Assignee)

Comment 17

6 years ago
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
Attachment #565120 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Assignee: nobody → raymond

Updated

6 years ago
Attachment #565869 - Attachment is patch: true
(Assignee)

Comment 18

6 years ago
Passed Try!
Keywords: checkin-needed
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 :-)
Target Milestone: --- → Firefox 10
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9bf1262cc262
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.