Support unprefixed field and fieldtext css colors
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Tracking
()
People
(Reporter: Sam, Assigned: Sam)
References
Details
(Keywords: dev-doc-needed)
Attachments
(2 files)
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
Split off of Bug 1590894
Rename these to support unprefixed version
Also add alias to keep compatibility
Comment 6•5 years ago
|
||
Backed out changeset ec25a8482342 (Bug 1592389) for mochitest failure at layout/style/test/test_value_computation.html.
Failure log:
Mochitest failure:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=273632731&repo=autoland&lineNumber=2211
Reftest failure:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=273629066&repo=autoland&lineNumber=10015
analyzer: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/KASBi9FKSDqWms9bbxdm2w/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
[task 2019-10-30T08:09:40.056Z] 08:09:40 INFO - TEST-PASS | layout/style/test/test_value_computation.html | should not get empty value for 'color:fieldtext'
[task 2019-10-30T08:09:40.056Z] 08:09:40 INFO - Buffered messages finished
[task 2019-10-30T08:09:40.057Z] 08:09:40 INFO - TEST-UNEXPECTED-FAIL | layout/style/test/test_value_computation.html | should not get initial value for 'color:fieldtext' on elementn. - didn't expect "rgb(0, 0, 0)", but got it
[task 2019-10-30T08:09:40.057Z] 08:09:40 INFO - SimpleTest.isnot@SimpleTest/SimpleTest.js:334:16
[task 2019-10-30T08:09:40.057Z] 08:09:40 INFO - test_value@layout/style/test/test_value_computation.html:174:73
[task 2019-10-30T08:09:40.057Z] 08:09:40 INFO - test_property@layout/style/test/test_value_computation.html:206:15
[task 2019-10-30T08:09:40.058Z] 08:09:40 INFO - do_one@layout/style/test/test_value_computation.html:221:18
[task 2019-10-30T08:09:40.060Z] 08:09:40 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-10-30T08:09:40.061Z] 08:09:40 INFO - TEST-UNEXPECTED-FAIL | layout/style/test/test_value_computation.html | should not get initial value for 'color:fieldtext' on elementf. - didn't expect "rgb(0, 0, 0)", but got it
[task 2019-10-30T08:09:40.061Z] 08:09:40 INFO - SimpleTest.isnot@SimpleTest/SimpleTest.js:334:16
[task 2019-10-30T08:09:40.061Z] 08:09:40 INFO - test_value@layout/style/test/test_value_computation.html:178:86
[task 2019-10-30T08:09:40.061Z] 08:09:40 INFO - test_property@layout/style/test/test_value_computation.html:206:15
[task 2019-10-30T08:09:40.061Z] 08:09:40 INFO - do_one@layout/style/test/test_value_computation.html:221:18
Assignee | ||
Comment 7•5 years ago
|
||
(This is a new test introduced in this patch)
FieldText can be black (0, 0, 0) on some (all?) platforms, which means that it matches the initial value for the color test most or all of the time. It can't be in the other values section because of this, and I don't think it putting it in the list of colors that matches the initial value is the solution because of the possibility of a difference depending on a platform's differing LookAndFeel implementation...
Either we extend this test to allow for this somehow or just remove it altogether as it's a pretty simple thing, though it would be nice to have a test for it.
Comment 8•5 years ago
|
||
I'd recommend reverting the property_database.js changes entirely. I suspect this is fine to re-land once you've done that.
You don't know for sure whether field
fieldtext
compute to the same thing or a different thing as the initial value (since their colors are platform/OS-theme-specific), so we can't legitimately place them in either of the arrays there (initial_values
& other_values
).
We can test the values elsewhere (and it looks like you do in this patch, via the WPT test). We just can't use these particular property_database.js
constructs to test them.
Assignee | ||
Comment 9•5 years ago
|
||
I've reverted property_database.js
and went ahead and queued it for landing again.
Comment 10•5 years ago
|
||
(In reply to Sam Mauldin (:Sam) from comment #9)
I've reverted
property_database.js
and went ahead and queued it for landing again.
I removed the checkin-needed flag because I noticed there's one other failure that will need to be addressed before this can re-land (which probably hadn't completed yet when Comment 6 was posted).
The original push for this was:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ec25a8482342f3e432f2fdfcddd42a6ed88bd249&selectedJob=273631151
And there's a reliable "Android R2" test-failure at the bottom there. Link to log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=273629066&repo=autoland&lineNumber=10015
The failure viewed in reftest-analyzer:
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/KASBi9FKSDqWms9bbxdm2w/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
Comment 11•5 years ago
|
||
So looking at the reftest on desktop, it looks like the testcase's <input type="range">
slider-widgets are legitimately skinnier than the reference case's widgets.
However, their background is transparent (and the container is wide enough that it doesn't matter for sizing/positioning purposes), so you can't tell.
With that information and the reftest-analyzer visualization, it seems that the problem here is that we somehow started triggering a non-transparent default background for these sliders on Android (which then reveals the otherwise-hidden fact that their widths are different between the testcase and reference case).
That is semi-believable since this patch did touch widget/android/nsLookAndFeel.cpp
, but in a way that wouldn't be expected to have any effect...
Comment 12•5 years ago
|
||
I've done a bit of investigation on this locally using the android emulator (from ./mach bootstrap
and picking option 4), and I found the following (specifically on Android emulator, when running this particular test):
- Before this patch, -moz-Fieldtext computes to
rgb(26, 26, 26)
and -moz-Field computes torgb(255, 255, 255)
(white) - After this patch, these keywords (whether prefixed or not) instead compute to the bizarrely different values
rgb(250, 209, 132)
andrgb(177, 165, 152)
, respectively. - If I change the patch to simply restore the original ordering in
color.rs
(which presumably impacts the numeric value of the enum), then I get the original sane computed values againrgb(26, 26, 26)
andrgb(255, 255, 255)
.
So this has something to do with the enum ordering. Perhaps the enum is used as an index somewhere, or there's some other sort of ordering expectation that we're violating?
Comment 13•5 years ago
|
||
For reference, here's the strawman followup that seems to make the reftest pass again, by restoring the original keyword ordering (though I don't know yet why that matters).
Assignee | ||
Comment 14•5 years ago
|
||
The enum’s somewhat used as an index in ‘nsXPLookAndFeel.cpp’, I thought I preserved the correct ordering there, but perhaps not. I’ll look into it in a few hours.
Comment 15•5 years ago
|
||
Yeah, here's the array in nsXPLookAndFeel.cpp:
https://searchfox.org/mozilla-central/rev/1fe0cf575841dbf3b7e159e88ba03260cd1354c0/widget/nsXPLookAndFeel.cpp#102-109
Your patch isn't touching that array right now, and it needs to update the ordering there -- moving these lines:
"ui.-moz-field",
"ui.-moz-fieldtext",
Those prefs may want a rename at some point, too, though maybe that shouldn't happen here, to avoid breaking backwards-compat (AFAICT this is striving for backwards-compatibility with the prefixed keyword names at this point, so we probably want to continue supporting the prefixed pref name for the moment as well).
Assignee | ||
Comment 16•5 years ago
|
||
Ah, yes. I missed that when I grafted this patch over...
Rerunning web platform tests on android before I try to land again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5df7a732563f3ac838486ff0d6273d30c50b958d
Comment 17•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 19•5 years ago
|
||
bugherder |
Comment 20•5 years ago
|
||
Please set keyword "dev-doc-needed" together wit reporting this kind of bugs. Thanks!
Description
•