Closed Bug 1592389 Opened 5 years ago Closed 5 years ago

Support unprefixed field and fieldtext css colors

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: Sam, Assigned: Sam)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files)

Bug 1592389 - Rename Mozfield / Mozfieldtext to Field and Fieldtext
47 bytes, text/x-phabricator-request
Details | Review
strawman followup (but we probably don't want this?): restore original ordering
1.11 KB, text/plain
Details
No description provided.
Assignee: nobody → sam
Blocks: 1590894
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3

Split off of Bug 1590894
Rename these to support unprefixed version
Also add alias to keep compatibility

Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec25a8482342
Rename Mozfield / Mozfieldtext to Field and Fieldtext r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19987 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging

Backed out changeset ec25a8482342 (Bug 1592389) for mochitest failure at layout/style/test/test_value_computation.html.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=273627247&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=ec25a8482342f3e432f2fdfcddd42a6ed88bd249

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

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=273632731&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=c9873e002149c87f5888e45e4da7f6288df6975a

[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
Flags: needinfo?(sam)

(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.

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.

I've reverted property_database.js and went ahead and queued it for landing again.

Flags: needinfo?(sam)

(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

Flags: needinfo?(sam)

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...

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 to rgb(255, 255, 255) (white)
  • After this patch, these keywords (whether prefixed or not) instead compute to the bizarrely different values rgb(250, 209, 132) and rgb(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 again rgb(26, 26, 26) and rgb(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?

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).

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.

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).

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

Flags: needinfo?(sam)
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff9cd9a27579
Rename Mozfield / Mozfieldtext to Field and Fieldtext r=emilio
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

Please set keyword "dev-doc-needed" together wit reporting this kind of bugs. Thanks!

Keywords: dev-doc-needed
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: