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.

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

Transferring review to ntim, hope this is alright.

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

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

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

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

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.

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

(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

The Dom panel also has no clear button.

Comment on attachment 8782733 [details] Bug 1296187 - Don't overlap inspector-searchinput-clear with text. https://reviewboard.mozilla.org/r/72788/#review73492

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.

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.

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

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

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

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

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/a1e635fc39e2 Don't overlap inspector-searchinput-clear with text. r=gl

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