Closed Bug 1216615 Opened 9 years ago Closed 9 years ago

Filter field in ruleview and computed view produces unnecessary movement during sidebar resizing

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox44 verified)

VERIFIED FIXED
Firefox 44
Tracking Status
firefox44 --- verified

People

(Reporter: arni2033, Assigned: bgrins)

References

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(5 files)

STR:   (Win7_64, Nightly 44, 32bit, ID 20151020031317, new profile)
1. Switch window to fullscreen mode
2. Open devtools -> inspector, open ruleview/computed view
3. Resize sidebar so that it was as wide as possible
4. Slowly resize sidebar so that it was as narrow as possible

Result:       Right (end) border of filter field moves to the right by ~6px
Expectations: Right (end) border of filter field should be stationary

Note_1:   Currently inner input (.devtools-searchinput) has "margin: 1px 3px;", and .devtools-searchbox (parent element) has "padding:0px;".   Removing margin-left/right from search input and adding the same padding to searchbox solves the issue
Example of CSS which works for me:
>   .devtools-searchbox   { -moz-padding-start: 3px !important; }
>   .devtools-searchbox   { -moz-padding-end:   3px !important; }
>   .devtools-searchinput { -moz-margin-start:  0px !important; }
>   .devtools-searchinput { -moz-margin-end:    0px !important; }

Note_2:   The solution introduced in bug 1200073 only cures the symptoms (by adding -moz-margin-start:5px to the checkbox in computed view); I believe that code is unnecessary and should be removed
See Also: → 1200073
Whiteboard: [polish-backlog][difficulty=easy]
See Also: → 1216569
Blocks: 1216569
See Also: 1216569
Bug 1216615 - Set margin-left and margin-right for devtools-searchinput to prevent movement during resize;r=pbrosset
Attachment #8676564 - Flags: review?(pbrosset)
Thanks for the find, I think this fix does the trick - where the margin start / end is 0 on the input.  I don't think the additional 3px padding on the container is needed since the toolbar has 3px padding.  Patrick, can you confirm this fixes it for you?
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
I made another research.
1) This patch changes visual indent before search field (according to mockup? Change in bug 1200073 was also done according to a mockup? If not, should it be backed out since it doesn't fix anything?). 
There's many inputs in devtools - this change can break a lot of things (like it always happens). Maybe margin should be removed only for '.devtools-searchbox > .devtools-searchinput'?
2) This patch changes visual indent before search field in tabs "Rules" and "Computed", but doesn't change visual indent before input field in tab "Font". Were you going to do the same for input fields?
(In reply to arni2033 from comment #3)
> I made another research.
> 1) This patch changes visual indent before search field (according to
> mockup? Change in bug 1200073 was also done according to a mockup? If not,
> should it be backed out since it doesn't fix anything?). 
> There's many inputs in devtools - this change can break a lot of things
> (like it always happens). Maybe margin should be removed only for
> '.devtools-searchbox > .devtools-searchinput'?
> 2) This patch changes visual indent before search field in tabs "Rules" and
> "Computed", but doesn't change visual indent before input field in tab
> "Font". Were you going to do the same for input fields?

Hi, good catch and thanks for doing this research.  Looks like font panel uses the .devtools-textinput class (and really only a couple of other things: https://dxr.mozilla.org/mozilla-central/search?q=devtools-textinput&redirect=true&case=false).  I guess these are for boxes that don't have the 'clear' button.

I think we can safely modify the rule with `.devtools-textinput, .devtools-searchinput` to be margin: 1px 0 and get rid of the special case for .devtools-searchinput.  Even though this does affect other search boxes in the tools, it seems to get rid of double 3px margins that they have when butted up against the edge of a toolbar (which I believe they are in all cases).  This also fixes the issue where it's different for the fonts panel.
Comment on attachment 8676564 [details]
MozReview Request: Bug 1216615 - Set margin-left and margin-right for devtools-searchinput to prevent movement during resize;r=pbrosset

Bug 1216615 - Set margin-left and margin-right for devtools-searchinput to prevent movement during resize;r=pbrosset
I've checked this out locally in the toolbox and all of the text inputs looked fine to me.  The main one that really changed noticeably was the netmonitor's filter box at the bottom, but it gave it extra px of spacing so it wasn't directly rubbing against the bottom toolbar.
Attached image network-field.png
I've applied the patch locally and can confirm that I'm not seeing the problem described in comment 0.
I had a look at other fields in devtools and think they all look good.
One thing about the network panel filter field though, it now looks like it's closer to the left and right borders around it. See attached screenshot.
I'm still going to R+ the patch because this fixes the original issue and it's such a simple CSS change. Feel free to fix the network panel problem without re-requesting review, or to file another bug for it.
Attachment #8676564 - Flags: review?(pbrosset) → review+
Comment on attachment 8676564 [details]
MozReview Request: Bug 1216615 - Set margin-left and margin-right for devtools-searchinput to prevent movement during resize;r=pbrosset

https://reviewboard.mozilla.org/r/22755/#review20459
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #7)
> One thing about the network panel filter field though, it now looks like
> it's closer to the left and right borders around it. See attached screenshot.
By the way, that's on Windows 10.
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #7)
> Created attachment 8677491 [details]
> network-field.png
> 
> I've applied the patch locally and can confirm that I'm not seeing the
> problem described in comment 0.
> I had a look at other fields in devtools and think they all look good.
> One thing about the network panel filter field though, it now looks like
> it's closer to the left and right borders around it. See attached screenshot.
> I'm still going to R+ the patch because this fixes the original issue and
> it's such a simple CSS change. Feel free to fix the network panel problem
> without re-requesting review, or to file another bug for it.

Good catch - thanks for checking that.  I'll re-add the 3px left/right margin directly to #requests-menu-filter-freetext-text in the netmonitor css.
(In reply to Brian Grinstead [:bgrins] from comment #10)
> I'll re-add the 3px left/right margin directly to #requests-menu-filter-freetext-text
So, I'm glad that you finally noticed the field in Netmonitor, but that's not the only broken place
> screenshot:   https://dl.dropboxusercontent.com/s/4rkqyljv0gna7eg/del%20bug%201216615%20-%20Focused%20button%20%2B%20broken%20add-on%20styling%201.png?dl=0
I still don't understand the way of development "To fix it in one place, and break in some other places. Then fix it in other places with special-case rules" instead of just "To fix it in this place w/ special-case rule and don't break other things at all". The last one refers to my suggestion:
> .devtools-searchbox > .devtools-searchinput{
>   -moz-margin-start: 0px;
>   -moz-margin-end: 0px;
> }
> .devtools-searchbox{
>   -moz-padding-start: 3px;
>   -moz-padding-end: 3px;
> }

It's also unclear for me why you ignored that simple solution in favor to (sic!) change visual appearance of all inputs (while it's not necessary in this bug).
(In reply to arni2033 from comment #12)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> > I'll re-add the 3px left/right margin directly to #requests-menu-filter-freetext-text
> So, I'm glad that you finally noticed the field in Netmonitor, but that's
> not the only broken place
> > screenshot:   https://dl.dropboxusercontent.com/s/4rkqyljv0gna7eg/del%20bug%201216615%20-%20Focused%20button%20%2B%20broken%20add-on%20styling%201.png?dl=0
> I still don't understand the way of development "To fix it in one place, and
> break in some other places. Then fix it in other places with special-case
> rules" instead of just "To fix it in this place w/ special-case rule and
> don't break other things at all". The last one refers to my suggestion:
> > .devtools-searchbox > .devtools-searchinput{
> >   -moz-margin-start: 0px;
> >   -moz-margin-end: 0px;
> > }
> > .devtools-searchbox{
> >   -moz-padding-start: 3px;
> >   -moz-padding-end: 3px;
> > }
> 
> It's also unclear for me why you ignored that simple solution in favor to
> (sic!) change visual appearance of all inputs (while it's not necessary in
> this bug).

The fix is an attempt to address the problem while also simplifying the CSS used throughout the theme for the shared elements.  The thing with the other proposed fix is that there are devtools-searchinput's that aren't children of searchboxes, so they wouldn't have changes applied by this.  Maybe the answer is to not allow that setup, but that's getting even further off into changing everything.

Also, there's currently an unintentional double padding in the common case of a search input that starts or ends a devtools-toolbar, so this addresses that while also fixing the original problem.  I hadn't considered the addon use-case, so maybe we need to be more specific with the selectors (only doing the 0 margin on start / end if the searchinput / searchbox / textinput is a first / last child of a toolbar).  I'll look into that.
Keywords: leave-open
Can you please check with the patch applied (on top of the one that already landed) and see if it fixes the issues you were seeing?
Flags: needinfo?(arni2033)
(In reply to Brian Grinstead [:bgrins] from comment #14)
> Created attachment 8677586 [details] [diff] [review]
> input-spacing-2.patch
> 
> Can you please check with the patch applied (on top of the one that already
> landed) and see if it fixes the issues you were seeing?

If this works I'll probably break it into 2 patches - 1 that just updates the spacing and a second for a new bug that takes care of the 'double spacing' issue with textboxes and toolbars
(In reply to Brian Grinstead [:bgrins] from comment #14)
> Can you please check with the patch applied and see if it fixes the issues you were seeing?
Yes, it fixes everything if I add/modify selector in your patch like this:
> +   /* The spacing is accomplished via a padding on the searchbox */
> +++ .devtools-searchbox > .devtools-textinput,
> +   .devtools-searchbox > .devtools-searchinput {
> +     margin-left: 0;
> +     margin-right: 0;
> +   }
Or like this:
> +   /* The spacing is accomplished via a padding on the searchbox */
> +✱  .devtools-searchbox > input {           /* changed to "input" */
> +     margin-left: 0;
> +     margin-right: 0;
> +   }
That's required because fonts panel also uses .devtools-searchbox (weird). I checked - there's only 3 .devtools-searchbox there so no extra elements would be affected by this patch.
>   http://mxr.mozilla.org/mozilla-central/search?find=/&string=devtools-searchbox

But I also found out that this bug could've been fixed with
>   .devtools-searchbox > input{
>     width: calc(100% - 3px - 3px); /* 3px for left margin, 3px for right margin */
>   }
OR
>   .devtools-searchbox > input{
>     min-width: 0px; /* currently it has -moz-min-content because
>                        parent element has "display:flex" and acts weird
>                        maybe there's a bug with -moz-min-content and margin */
>   }
This looks like more 'clever' solution than just expecting using margins... But, as I said, your last patch fixes everything after small change, so there's no need to look for better solution (probably)
Flags: needinfo?(arni2033)
Sorry about the extra review Patrick, but this will fix up a couple of remaining issues.  I was hoping we wouldn't need to add new rules for this but it seems we do.  I will file a new bug to get rid of the 'double spacing' in the toolbar that the previous approach was fixing
Attachment #8678164 - Flags: review?(pbrosset)
Comment on attachment 8678164 [details] [diff] [review]
input-spacing.patch

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

Looks good to me.
Attachment #8678164 - Flags: review?(pbrosset) → review+
Keywords: leave-open
Blocks: 1217884
https://hg.mozilla.org/mozilla-central/rev/0a5d8b46660d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Thanks all!
I have successfully reproduce this bug on firefox nightly 44.0a1 (2015-10-20)
with windows 7 (64 bit)
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0

I found this fix on latest nightly 44.0a1 (2015-10-27)
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID : 20151027030239 

[bugday-20151028]
Reproduced this bug on Nightly 44.0a1 (2015-10-20); (Build ID: 20151020031317) in Linux,64 bit

This Bug is now verified as fixed on Latest Firefox Nightly 45.0a1 (2015-10-29)

Build ID: 20151029045227
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0

As it is also verified on Windows (Comment 23), Marking it as verified!
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20151028]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: