1373655 - Search sign and arrow indicator aren't displayed with high contrast theme

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Hani Yacoub, Desktop QA]

Attachment: highcontrast.png

[Affected versions]: 
Nightly 56.0a1

[Affected platforms]:
Platforms: Windows 10 x 64

[Steps to reproduce]:
1. Activate a high contrast theme.
- "Windows: Go to Personalize> Themes> Theme Settings and activate a High Contrast Theme.
2. Launch Firefox, go to about:config and search for "browser.preferences.search" and set it value to true.
3. Go to "about:preferences".
4. Search for "yahoo" in the search field.

[Expected result]:
Search sign and arrow indicator should be displayed with high contrast theme.

[Actual result]:
Search sign and arrow indicator aren't displayed with high contrast theme.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Both search sign and arrow indicator are using background-image to render the search-textbox.svg. Find a randomly xul:image and replace its image with search-textbox.svg will show the image correctly.

I suspect that might be a platform issue when rendering background-image in high contrast mode.

Mike, do you have any clue about about using background-image in high contrast mode? Is there any workaround we can fix it easily?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Redirecting to dao who I think has much more experience than I dealing with high-contrast mode (especially due to his work in the dependencies of bug 1016556).

Comment 3

[Dão Gottwald [:dao]]

High contrast mode intentionally disables background images, see also bug 1022553.

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Loading UA stylesheet with loadSheetUsingURIString mechanism [1] works and it can display background-image for menulist. But search input doesn't work.

Dao, do you have any idea why injecting UA into in-content doesn't work for search input?

[1] http://searchfox.org/mozilla-central/source/toolkit/content/datepicker.xhtml#40-47

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1373655 - Fix search sign and arrow indicator in high contrast theme

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

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8882033 [details]
Bug 1373655 - Fix search sign and arrow indicator in high contrast theme

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

Comment 7

[Ricky Chien [:rickychien] (inactive)]

set about:config pref browser.display.document_color_use = 2 for testing.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8882033 [details]
Bug 1373655 - Fix search sign and arrow indicator in high contrast theme

https://reviewboard.mozilla.org/r/153108/#review158334

::: browser/components/preferences/in-content-new/preferences.js:27
(Diff revision 2)
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/AppConstants.jsm");
>  
> +var domWinUtls = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)

You might want to write a comment above this line & leave a comment on the original stylesheet.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8882033 [details]
Bug 1373655 - Fix search sign and arrow indicator in high contrast theme

https://reviewboard.mozilla.org/r/153108/#review158394

::: browser/components/preferences/in-content-new/preferences.js:30
(Diff revision 2)
>  Cu.import("resource://gre/modules/AppConstants.jsm");
>  
> +var domWinUtls = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                       .getInterface(Components.interfaces.nsIDOMWindowUtils);
> +domWinUtls.loadSheetUsingURIString("data:text/css,textbox[type='search'] > .textbox-input-box { background-image: url(chrome://global/skin/icons/search-textbox.svg); }", domWinUtls.AGENT_SHEET);
> +domWinUtls.loadSheetUsingURIString("data:text/css,menulist[indicator=true] > menupopup menuitem[indicator=true]:not([image]) > .menu-iconic-left { background-image: url(chrome://global/skin/icons/search-arrow-indicator.svg); }", domWinUtls.AGENT_SHEET);

Ugh. Let's not do this. Instead, let's move the <image> in textbox.xml (except for the searchbutton case, where the image needs to keep its current location). We can do this now since we replaced the "Search:" label with placeholder text in the history and bookmarks sidebars, which makes these search boxes wider (so they can display both the search icon and the clear icon simultaneously).

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8882033 [details]
Bug 1373655 - Fix search sign and arrow indicator in high contrast theme

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

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Dao,

Your approach of <image> in textbox.xml helps a lots! I've updated my patch and got rid of all those ugly loadSheetUsingURIString hacks.

Right now, both arrow indicators in menulist and search sign can be visible correctly in high contrast theme with setting `browser.display.document_color_use = 2`.

Can you review the patch again? thanks

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8882033 [details]
Bug 1373655 - Fix search sign and arrow indicator in high contrast theme

https://reviewboard.mozilla.org/r/153108/#review158600

::: toolkit/themes/shared/in-content/common.inc.css
(Diff revision 3)
>    padding-right: unset;
>    padding-left: unset;
>  }
>  
> -xul|textbox[type="search"] > .textbox-input-box {
> -  background: url(chrome://global/skin/icons/search-textbox.svg) no-repeat center left;

We still need this in the "searchbutton" case where the icon acts as a submit button rather than just an indicator.

Comment 13

[Ricky Chien [:rickychien] (inactive)]

Dao, I'm not very clear what's your comment saying. Can you give me a concrete example for "searchbutton" case?

If we put below back, there will be two search sign icons showing in the same time. 

> -xul|textbox[type="search"] > .textbox-input-box {
> -  background: url(chrome://global/skin/icons/search-textbox.svg) no-repeat center left;

Comment 14

[Dão Gottwald [:dao]]

(In reply to Ricky Chien [:rickychien] from comment #13)
> Dao, I'm not very clear what's your comment saying. Can you give me a
> concrete example for "searchbutton" case?

about:addons has such a search box, but the button is currently broken by your changes from bug 1369640.

> If we put below back, there will be two search sign icons showing in the
> same time.

You'll need to show only one of the two based on the searchbutton attribute.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #14)
> (In reply to Ricky Chien [:rickychien] from comment #13)
> > Dao, I'm not very clear what's your comment saying. Can you give me a
> > concrete example for "searchbutton" case?
> 
> about:addons has such a search box, but the button is currently broken by
> your changes from bug 1369640.

Bug 1360491 even

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8882033 [details]
Bug 1373655 - Fix search sign and arrow indicator in high contrast theme

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

Comment 17

[Ricky Chien [:rickychien] (inactive)]

IIRC, my patch should be tested and passed in the case when searchbutton="true" and searchbutton="false".

But what I saw both of above cases work as expected. I can see search-textbox.svg icon renders properly at the left side of textbox.

Dao, do I misunderstand anything?

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8882033 [details]
Bug 1373655 - Fix search sign and arrow indicator in high contrast theme

https://reviewboard.mozilla.org/r/153108/#review158760

I don't see the search button displayed in about:addons with this patch applied.

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8882033 [details]
Bug 1373655 - Fix search sign and arrow indicator in high contrast theme

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

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 8882033 [details]
Bug 1373655 - Fix search sign and arrow indicator in high contrast theme

https://reviewboard.mozilla.org/r/153108/#review158916

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b22febc81a56
Fix search sign and arrow indicator in high contrast theme r=dao

Comment 22

[Phil Ringnalda (:philor)]

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

Comment 23

[Hani Yacoub, Desktop QA]

Build ID: 20170702030204
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

Verified as fixed on Firefox Nightly 56.0a1 on Windows 10 x 64.