Closed Bug 1373655 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Preferences, defect, P1)

56 Branch
All
Windows
defect

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox56 --- verified

People

(Reporter: hani.yacoub, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(2 files)

Attached image 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.
Blocks: 1357285
Whiteboard: [photon-preference]
Flags: qe-verify+
Priority: -- → P2
QA Contact: hani.yacoub
Target Milestone: --- → Firefox 56
Assignee: nobody → rchien
Priority: P2 → P1
Status: NEW → ASSIGNED
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?
Assignee: rchien → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mconley)
Priority: P1 → P2
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Priority: P2 → P1
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).
Flags: needinfo?(mconley) → needinfo?(dao+bmo)
High contrast mode intentionally disables background images, see also bug 1022553.
Flags: needinfo?(dao+bmo)
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
Flags: needinfo?(dao+bmo)
Flags: needinfo?(dao+bmo)
set about:config pref browser.display.document_color_use = 2 for testing.
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 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).
Attachment #8882033 - Flags: review-
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 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.
Attachment #8882033 - Flags: review?(dao+bmo) → review-
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;
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
(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
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 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.
Attachment #8882033 - Flags: review?(dao+bmo) → review-
Comment on attachment 8882033 [details]
Bug 1373655 - Fix search sign and arrow indicator in high contrast theme

https://reviewboard.mozilla.org/r/153108/#review158916
Attachment #8882033 - Flags: review?(dao+bmo) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b22febc81a56
Fix search sign and arrow indicator in high contrast theme r=dao
https://hg.mozilla.org/mozilla-central/rev/b22febc81a56
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.