Don't overlap inspector-searchinput-clear with text

VERIFIED FIXED in Firefox 51

Status

defect
P1
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: magicp.jp, Assigned: steveck)

Tracking

(Blocks 1 bug)

Trunk
Firefox 51
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 verified)

Details

(Whiteboard: [reserve-html])

Attachments

(4 attachments, 1 obsolete attachment)

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.
Blocks: 1265759
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Flags: qe-verify?
Whiteboard: [devtools-html] [triage]
Assignee: nobody → schung
Status: NEW → ASSIGNED
add background-color for clear button can fix that as well
Priority: -- → P3
Whiteboard: [devtools-html] [triage] → [reserve-html]
Attachment #8782733 - Flags: review?(pbrosset) → review?(ntim.bugs)
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
Attachment #8782733 - Flags: review?(ntim.bugs) → review-
Posted image Odd shifting issue (obsolete) —
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.
Attachment #8782733 - Flags: review?(ntim.bugs) → review?(gl)
Searchboxes that don't have a clear button look quite odd with long text entered.
Attachment #8784090 - Attachment is obsolete: true
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.
Attachment #8782733 - Flags: review?(gl)
(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
Attachment #8782733 - Flags: review?(gl) → review+
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.
Attachment #8782733 - Flags: review+
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.
(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.
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
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."
Attachment #8782733 - Flags: review?(gl) → review+
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?
Keywords: checkin-needed
(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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a1e635fc39e2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Iteration: --- → 51.3 - Sep 12
Priority: P3 → P1
verified on latest Nightly Build ID (20160908030434)
Thanks all!
Status: RESOLVED → VERIFIED
Flags: qe-verify?
Blocks: 1305664
No longer blocks: 1305664
Depends on: 1305664
Blocks: 1305664
No longer depends on: 1305664
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.