Issue 4510 - Filter preferences: replace the built-in findbar widget by our own look-alike

Issue 4510 - Filter preferences: replace the built-in findbar widget by our own look-alike

[Wladimir Palant, 2016-10-11 09:00:20]

There is a description of the changes under
https://issues.adblockplus.org/ticket/4510#comment:1.

[Thomas Greiner, 2016-10-12 14:03:14]

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
File chrome/content/ui/filters-search.js (right):

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
chrome/content/ui/filters-search.js:117:
E("findbar-textbox").setAttribute("status", currentStatus);
Usually, what we do is set a "data-*" attribute on a parent element and
implement the logic in CSS. Thereby you'd avoid having to modify each of the
elements each time and you could even reuse it for the
`#findbar-textbox[status="notFound"]` rule.

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
chrome/content/ui/filters-search.js:121: return setStatus("");
Detail: `setStatus()` doesn't have a return value so I assume the reason you did
this is because the previous code actually returned a value.

Since this is no longer the case, however, I'd rather avoid this unnecessary
confusion and write it as two separate lines.

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
File chrome/content/ui/filters.xul (right):

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
chrome/content/ui/filters.xul:375: <hbox id="findbar" hidden="true"
align="center" onkeypress="FilterSearch.keyPress(event);">
Detail: "hidden" is a boolean attribute so it doesn't have a value. Therefore
valid variants are `hidden="hidden"`, `hidden=""` and `hidden` according to
https://www.w3.org/TR/html-markup/global-attributes.html#common.attrs.hidden.

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/skin/filters...
File chrome/skin/filters.css (right):

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/skin/filters...
chrome/skin/filters.css:242: filter: drop-shadow(0 0 4px red);
Detail: This looks quite strange due to the border color of the textbox element
when it is focused.

For reference, below are the styles from Firefox' stylesheet for invalid form
input fields. I'd suggest applying those instead.

/**
 * Set default style for invalid elements.
 */
:not(output):-moz-ui-invalid {
  box-shadow: 0 0 1.5px 1px red;
}

:not(output):-moz-ui-invalid:-moz-focusring {
  box-shadow: 0 0 2px 2px rgba(255,0,0,0.4);
}

output:-moz-ui-invalid {
  color: red;
}

See resource://gre-resources/forms.css

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/skin/filters...
chrome/skin/filters.css:259: display: -moz-box;
Sounds like it behaves the same way as `flex="1"` so I wonder why are we using
this instead?

[Wladimir Palant, 2016-10-12 15:22:01]

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
File chrome/content/ui/filters-search.js (right):

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
chrome/content/ui/filters-search.js:117:
E("findbar-textbox").setAttribute("status", currentStatus);
On 2016/10/12 14:03:13, Thomas Greiner wrote:
> Usually, what we do is set a "data-*" attribute on a parent element and
> implement the logic in CSS. Thereby you'd avoid having to modify each of the
> elements each time and you could even reuse it for the
> `#findbar-textbox[status="notFound"]` rule.

Done.

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
chrome/content/ui/filters-search.js:121: return setStatus("");
On 2016/10/12 14:03:13, Thomas Greiner wrote:
> Detail: `setStatus()` doesn't have a return value so I assume the reason you
did
> this is because the previous code actually returned a value.

Actually, I wanted to make sure that each return sets a status. Well, I can do
this differently...

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
File chrome/content/ui/filters.xul (right):

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
chrome/content/ui/filters.xul:375: <hbox id="findbar" hidden="true"
align="center" onkeypress="FilterSearch.keyPress(event);">
On 2016/10/12 14:03:14, Thomas Greiner wrote:
> Detail: "hidden" is a boolean attribute so it doesn't have a value. Therefore
> valid variants are `hidden="hidden"`, `hidden=""` and `hidden` according to
> https://www.w3.org/TR/html-markup/global-attributes.html#common.attrs.hidden.

This is not HTML ;)

See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/hidden

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/skin/filters...
File chrome/skin/filters.css (right):

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/skin/filters...
chrome/skin/filters.css:242: filter: drop-shadow(0 0 4px red);
On 2016/10/12 14:03:14, Thomas Greiner wrote:
> Detail: This looks quite strange due to the border color of the textbox
element
> when it is focused.
> 
> For reference, below are the styles from Firefox' stylesheet for invalid form
> input fields. I'd suggest applying those instead.

I considered just marking the field as invalid instead but that looks *really*
weird - only the actual input field can be marked that way, and it's way smaller
than the <textbox> element.

box-shadow doesn't seem to look too different but fine with me. Not sure whether
a different shadow for focused elements is really a good idea...

Changing text color without being able to control the background color is
definitely a bad idea.

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/skin/filters...
chrome/skin/filters.css:259: display: -moz-box;
On 2016/10/12 14:03:14, Thomas Greiner wrote:
> Sounds like it behaves the same way as `flex="1"` so I wonder why are we using
> this instead?

This is merely overriding display:none that hidden="true" normally applies, but
this is no longer necessary now.

[Thomas Greiner, 2016-10-12 17:18:10]

LGTM

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
File chrome/content/ui/filters-search.js (right):

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
chrome/content/ui/filters-search.js:121: return setStatus("");
On 2016/10/12 15:22:00, Wladimir Palant wrote:
> On 2016/10/12 14:03:13, Thomas Greiner wrote:
> > Detail: `setStatus()` doesn't have a return value so I assume the reason you
> did
> > this is because the previous code actually returned a value.
> 
> Actually, I wanted to make sure that each return sets a status. Well, I can do
> this differently...

Thanks!

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
File chrome/content/ui/filters.xul (right):

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
chrome/content/ui/filters.xul:375: <hbox id="findbar" hidden="true"
align="center" onkeypress="FilterSearch.keyPress(event);">
On 2016/10/12 15:22:00, Wladimir Palant wrote:
> On 2016/10/12 14:03:14, Thomas Greiner wrote:
> > Detail: "hidden" is a boolean attribute so it doesn't have a value.
Therefore
> > valid variants are `hidden="hidden"`, `hidden=""` and `hidden` according to
> >
https://www.w3.org/TR/html-markup/global-attributes.html#common.attrs.hidden.
> 
> This is not HTML ;)
> 
> See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/hidden

Unfortunately, the page you mentioned is quite ambiguous about that but I get
your point.

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/skin/filters...
File chrome/skin/filters.css (right):

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/skin/filters...
chrome/skin/filters.css:242: filter: drop-shadow(0 0 4px red);
On 2016/10/12 15:22:00, Wladimir Palant wrote:
> On 2016/10/12 14:03:14, Thomas Greiner wrote:
> > Detail: This looks quite strange due to the border color of the textbox
> element
> > when it is focused.
> > 
> > For reference, below are the styles from Firefox' stylesheet for invalid
form
> > input fields. I'd suggest applying those instead.
> 
> I considered just marking the field as invalid instead but that looks *really*
> weird - only the actual input field can be marked that way, and it's way
smaller
> than the <textbox> element.
> 
> box-shadow doesn't seem to look too different but fine with me. Not sure
whether
> a different shadow for focused elements is really a good idea...
> 
> Changing text color without being able to control the background color is
> definitely a bad idea.

It's a pity that we can't just mark it as invalid, agreed. Alternatively, we
could use the `<html:input>` element if we want to have more control over its
design since that's what Firefox uses for the search bars in about:config and
about:preferences#applications.

But the way it is now should already be sufficient.

[Wladimir Palant, 2016-10-12 17:53:31]

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
File chrome/content/ui/filters.xul (right):

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/content/ui/f...
chrome/content/ui/filters.xul:375: <hbox id="findbar" hidden="true"
align="center" onkeypress="FilterSearch.keyPress(event);">
On 2016/10/12 17:18:10, Thomas Greiner wrote:
> Unfortunately, the page you mentioned is quite ambiguous about that

The text seems to have been imported from XULPlanet.com (© 1999 - 2004) and
never changed - it could definitely use some improvement. The same thing is more
obvious on
https://developer.mozilla.org/en-US/Add-ons/Overlay_Extensions/Firefox_addons...

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/skin/filters...
File chrome/skin/filters.css (right):

https://codereview.adblockplus.org/29356477/diff/29356483/chrome/skin/filters...
chrome/skin/filters.css:242: filter: drop-shadow(0 0 4px red);
On 2016/10/12 17:18:10, Thomas Greiner wrote:
> Alternatively, we
> could use the `<html:input>` element if we want to have more control over its
> design

Or we simply set `-moz-appearance: none` or drop `type="search"` which will also
give us full control over the design, but then we have to reimplement all the
styles and behavior. Mozilla doesn't mind doing that for the findbar, but for us
that would clearly be too much effort (particularly getting somewhat native
look&feel on all platforms).