1296187 - Don't overlap inspector-searchinput-clear with text

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[magicp]

Attachment: do-no-overlap-inspector-searchinput-clear-with-text.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160817030202

Steps to reproduce:

1. Start Nightly
2. Go to about:home
3. Open DevTools > Inspector
4. Enter "." in inspector-searchbox
5. Select ".contentSearchSuggestionsContainer"


Actual results:

inspector-searchinput-clear overlaps with value selected.

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=c9bbdb627b7804fee47aa6a6708647e6e589d09c&tochange=3269dd1a824d1b42cb021d1fb6858885179940b0


Expected results:

Don't overlap inspector-searchinput-clear with text.

Comment 1

[Steve Chung [:steveck]]

Attachment: Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

Review commit: https://reviewboard.mozilla.org/r/72788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72788/

Comment 2

[[e-mail]]

add background-color for clear button can fix that as well

Comment 3

[Patrick Brosset <:pbro>]

Transferring review to ntim, hope this is alright.

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8782733 [details]
Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

https://reviewboard.mozilla.org/r/72788/#review70982

r- because there's an odd shifting issue when typing in the input.

::: devtools/client/themes/toolbars.css:377
(Diff revision 1)
>    background-image: url(images/filter.svg#filterinput);
>  }
>  
> +.devtools-searchinput[filled],
> +.devtools-filterinput[filled] {
> +  padding-inline-end: 23px;

This is worth a comment

Comment 5

[Tim Nguyen :ntim]

Attachment: Odd shifting issue

Comment 6

[Steve Chung [:steveck]]

Comment on attachment 8782733 [details]
Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72788/diff/1-2/

Comment 7

[Steve Chung [:steveck]]

I set the search input for permanent padding end(and simpler rule because it might not necessary to have complex direction computation for 1 px). There will need more width and position changes for searchbox filled case, but I think we could simply set the padding now matter which state it is.

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8782733 [details]
Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

https://reviewboard.mozilla.org/r/72788/#review72622

Sorry for the delay, :gl would be more suited to review this change.

Comment 9

[Tim Nguyen :ntim]

Attachment: odd spacing in mem tool

Searchboxes that don't have a clear button look quite odd with long text entered.

Comment 10

[Tim Nguyen :ntim]

Also, the patch bitrotted, sorry about that. The searchbox code got moved to common.css

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8782733 [details]
Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

https://reviewboard.mozilla.org/r/72788/#review72694

Clearing review since it needs a rebase.

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8782733 [details]
Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72788/diff/2-3/

Comment 13

[Steve Chung [:steveck]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #9)
> Created attachment 8785678 [details]
> odd spacing in mem tool
> 
> Searchboxes that don't have a clear button look quite odd with long text
> entered.

Thanks for pointing out the exception. I thought all the search/filter would have the clear button. Is there any reason we skip the clear button only in memory panel?

Comment 14

[Tim Nguyen :ntim]

(In reply to Steve Chung [:steveck] from comment #13)
> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #9)
> > Created attachment 8785678 [details]
> > odd spacing in mem tool
> > 
> > Searchboxes that don't have a clear button look quite odd with long text
> > entered.
> 
> Thanks for pointing out the exception. I thought all the search/filter would
> have the clear button. Is there any reason we skip the clear button only in
> memory panel?

No specific reason, it just hasn't been implemented

Comment 15

[Tim Nguyen :ntim]

The Dom panel also has no clear button.

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8782733 [details]
Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

https://reviewboard.mozilla.org/r/72788/#review73492

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8782733 [details]
Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

https://reviewboard.mozilla.org/r/72788/#review73494

Actually, just tested this on the filter bar on the network panel, and these changes cause some issues there.

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Padding in the Network Panel RTL

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8782733 [details]
Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72788/diff/3-4/

Comment 20

[Steve Chung [:steveck]]

Ok, in this patch, I decided to leave the input which still applied xul elements unchanged and only adjust the padding only when clear button existed. It should still works even we replace the xul input with HTML elements.

Comment 21

[Tim Nguyen :ntim]

Does this rule still apply after your changes? https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.css#651

Comment 22

[Steve Chung [:steveck]]

Comment on attachment 8782733 [details]
Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72788/diff/4-5/

Comment 23

[Steve Chung [:steveck]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #21)
> Does this rule still apply after your changes?
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.
> css#651

No because the structure changed.. :/ So I updated the patch with additional "has-clear-btn" class for searchbox container(I think it would be less fragile then set in input)

Hi Gabriel, sorry for the inconvenience and it would need your review again.

Comment 24

[Tim Nguyen :ntim]

(In reply to Steve Chung [:steveck] from comment #23)
> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #21)
> > Does this rule still apply after your changes?
> > https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.
> > css#651
> 
> No because the structure changed.. :/ So I updated the patch with additional
> "has-clear-btn" class for searchbox container(I think it would be less
> fragile then set in input)
> 
> Hi Gabriel, sorry for the inconvenience and it would need your review again.

Sounds fine to me, I'll let Gabe have his say.

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8782733 [details]
Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

https://reviewboard.mozilla.org/r/72788/#review75350

This is very disappointing to see that we need to have a separate class to special case the searchinput-clear padding for the html searchinputs. This clearly highlights the problem with trying to maintain a xul/html implementation while using a common style classes. This also highlights that we need a common searchinput component implementation that is used in every panel. Namely, the DOM and memory panel currently doesn't implement the style of the searchipnut boxes for consistency and we need to do better. 

Steve, don't mind the rant above.

::: devtools/client/themes/common.css:530
(Diff revision 5)
>    background-size: 11px 11px;
>    background-repeat: no-repeat;
>    font-size: inherit;
>  }
>  
> +.has-clear-btn > .devtools-searchinput,

In the ideal solution, we would not want to keep a separate class for these html searchinput, and is a result of managing different inconsistent searchinput/filterinput in the devtools. Can you add the following notes in a common on top of this: 

"Bug 1296187 - Don't overlap clear button with html search and filter inputs. We should remmove this special class when we have a standardized search and filter input across the toolbox."

Comment 26

[Steve Chung [:steveck]]

Comment on attachment 8782733 [details]
Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72788/diff/5-6/

Comment 27

[Steve Chung [:steveck]]

Comment on attachment 8782733 [details]
Bug 1296187 - Don't overlap inspector-searchinput-clear with text.

https://reviewboard.mozilla.org/r/72788/#review75350

> In the ideal solution, we would not want to keep a separate class for these html searchinput, and is a result of managing different inconsistent searchinput/filterinput in the devtools. Can you add the following notes in a common on top of this: 
> 
> "Bug 1296187 - Don't overlap clear button with html search and filter inputs. We should remmove this special class when we have a standardized search and filter input across the toolbox."

Totally agree what you said and we'll need to remove this workaround once the inputs are unified. I added a comment for it, and maybe we can file a follow up for this?

Comment 28

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Steve Chung [:steveck](OOO 9/8 - 9/16) from comment #27)
> Comment on attachment 8782733 [details]
> Bug 1296187 - Don't overlap inspector-searchinput-clear with text.
> 
> https://reviewboard.mozilla.org/r/72788/#review75350
> 
> > In the ideal solution, we would not want to keep a separate class for these html searchinput, and is a result of managing different inconsistent searchinput/filterinput in the devtools. Can you add the following notes in a common on top of this: 
> > 
> > "Bug 1296187 - Don't overlap clear button with html search and filter inputs. We should remmove this special class when we have a standardized search and filter input across the toolbox."
> 
> Totally agree what you said and we'll need to remove this workaround once
> the inputs are unified. I added a comment for it, and maybe we can file a
> follow up for this?

Yes, filing follow ups for these would be a very good idea. 

1. To standardize the searchinput/filterinput across all of devtools (DOM, Memory). This can go in 2 directions. We already converted the inspector search into a component, but we need to take it one step further and fully make it a shareable component and reused everywhere else. Alternatively, just make everything look the same (not as desirable as an approach). We should probably be thinking in a react component and may have already been done in debugger.html/devtools.html with the new console and debugger projects. I would start there and figure out how we can share everything.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/a1e635fc39e2
Don't overlap inspector-searchinput-clear with text. r=gl

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a1e635fc39e2

Comment 31

[magicp]

verified on latest Nightly Build ID (20160908030434)
Thanks all!