1323404 - [Stylo] Stylo renders black text as grey

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Shing Lyu [:shinglyu]]

Check this reftest result:

https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/DWDN_wJjTlOG63eRyp3h6Q/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Select the "file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/reftest-sanity/default.html" test case.

The "Image 1 (TEST) is from Stylo, the text is grey; The "Image 2(REFERENCE)" is from Gecko's stock style system, the text is black. There are tons of reftest failures because of this color issue. (See https://treeherder.mozilla.org/#/jobs?repo=try&revision=82a813d1bd1df2a9c4e3a0733c6de62d08f27a44&selectedJob=32641575 for a full result)

Comment 1

[Bobby Holley (:bholley)]

Interesting! Curious to know what's causing this. Is it related to our hackiness here?

http://searchfox.org/mozilla-central/rev/36bfd0a8da5490274ca49c100957244253845e52/layout/style/StyleStructContext.h#81

If so, it might get fixed by bz's work in bug 1298588. But I'd be sorta surprised if

> LookAndFeel::GetColor(LookAndFeel::eColorID_WindowForeground, NS_RGB(0x00, 0x00, 0x00))

Returned gray?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Who knows what that returns, especially on different OSes...  What OS _is_ this on?  What does Gecko do on that OS if you set the "browser.display.use_system_colors" pref to true?

Comment 3

[Bobby Holley (:bholley)]

Oh, I see! I misread the code when implementing StyleStructContext.cpp. I thought that [1] was the default case, but we actually only hit that when usePrefColors is false, which is not the default.

So as a stopgap, we should change the code at [2] to just return NS_RGB(0x00, 0x00, 0x00) in the SERVO_DEFAULT case. Shing, can you see if that solves the problem?

[1] http://searchfox.org/mozilla-central/rev/36bfd0a8da5490274ca49c100957244253845e52/layout/base/nsPresContext.cpp#466

[2] http://searchfox.org/mozilla-central/rev/36bfd0a8da5490274ca49c100957244253845e52/layout/style/StyleStructContext.h#83

Comment 4

[Shing Lyu [:shinglyu]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> Who knows what that returns, especially on different OSes...  What OS _is_
> this on?  

% uname -a
Linux shinglyu-XPS-15-9550 4.4.0-53-generic #74-Ubuntu SMP Fri Dec 2 15:59:10 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

> What does Gecko do on that OS if you set the
> "browser.display.use_system_colors" pref to true?

I tried, it's still gery

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I tried, it's still gery

"still"?  I thought you said Gecko's styling was black?

To be clear, my question is what happens in Gecko _without_ stylo but with that pref set.

Comment 6

[Shing Lyu [:shinglyu]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #5)
> To be clear, my question is what happens in Gecko _without_ stylo but with
> that pref set.

Oh, Gecko without stylo with that perf on => grey text
It also happens on try server (platform = linux64-stylo), so probably not just my computer's issue



(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #3)
> So as a stopgap, we should change the code at [2] to just return
> NS_RGB(0x00, 0x00, 0x00) in the SERVO_DEFAULT case. Shing, can you see if
> that solves the problem?
Yes it fix the problem. Should I submit a patch as workaround or should we dig into the root cause?

Comment 7

[Bobby Holley (:bholley)]

Let's land that patch, bz is fixing the root cause.

Comment 8

[Shing Lyu [:shinglyu]]

This will reduce the failures from 4198 to 3044, that's 1154 test cases fixed.

Comment 9

[Shing Lyu [:shinglyu]]

Attachment: Bug 1323404 - Fixed Stylo renders black text as grey issue

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

Comment 10

[Shing Lyu [:shinglyu]]

Try push with the patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=501669dae26a902f18b8a6154201a9199520fd48&selectedJob=32802891

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8819194 [details]
Bug 1323404 - Fixed Stylo renders black text as grey issue

https://reviewboard.mozilla.org/r/99060/#review99302

Comment 12

[Shing Lyu [:shinglyu]]

Attachment: Awesomebar Selection background.png

One other thing I notice is that the URL bar selection background is grey too. This looks related but I'm not quite sure.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Oh, Gecko without stylo with that perf on => grey text

OK, that means that system color on those OSes is in fact gray.  ;)

Comment 14

[Shing Lyu [:shinglyu]]

I don't have Lv 3 access (maybe I should get one). Do you mind clicking the land button for me? Thank you. :)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2f24b4c8c2f
Fixed Stylo renders black text as grey issue r=bholley

Comment 16

[Emilio Cobos Álvarez (:emilio)]

I did it since Bobby is probably already or close to be in bed.

Comment 17

[Carsten Book [:Tomcat]]

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